[Silin Chen] iP by silinche · Pull Request #192 · nus-cs2113-AY2122S1/ip

silinche · 2021-09-01T14:54:38Z

No description provided.

kelvneo

Nice job on the readability of the code, but the main thing to look out for is the method names.

kelvneo · 2021-09-02T02:22:52Z

src/main/java/Task.java

+    public void setDone() {
+        this.isDone = true;
+    }


Good job following naming conventions, however, I think you may have to place a parameter for the boolean instead.

kelvneo · 2021-09-02T02:28:41Z

src/main/java/Duke.java

+            if (line.equals("list")) {
+                for (int i = 0; i < taskNumber; i++) {
+                    System.out.println((i + 1) + "." + tasks[i]);
+                }


Good work on following bracketing conventions:
https://se-education.org/guides/conventions/java/basic.html#conditionals

kelvneo · 2021-09-02T02:30:40Z

src/main/java/Duke.java

+        Task[] tasks= new Task[100];
+        int taskNumber = 0;
+        String line;
+        Scanner in = new Scanner(System.in);


Perhaps you can name the in here inputScanner instead?

kelvneo · 2021-09-02T02:33:01Z

src/main/java/Duke.java

+
+
 public class Duke {
+    public static void hi() {


The name of the method have to be a verb instead:
https://se-education.org/guides/conventions/java/basic.html#naming

kelvneo · 2021-09-02T02:33:18Z

src/main/java/Duke.java

+        System.out.println("What can I do for you?");
+    }
+
+    public static void bye() {


The name of the method have to be a verb instead:
https://se-education.org/guides/conventions/java/basic.html#naming

kelvneo · 2021-09-02T02:33:49Z

src/main/java/Duke.java

+        System.out.println("Bye. Hope to see you again soon!");
+    }
+
+    public static void running() {


The name of the method have to be a verb instead:
https://se-education.org/guides/conventions/java/basic.html#naming

AnShengLee

Overall, readability is there. Main issue is the long running() method that takes up multiple lines of code. Do try to keep a method within 30 lines of code.

AnShengLee · 2021-09-02T04:48:39Z

src/main/java/Duke.java

+    public static void running() {
+        Task[] tasks= new Task[100];
+        int taskNumber = 0;
+        String line;
+        Scanner in = new Scanner(System.in);
+        while (true) {
+            //new scanner
+            line = in.nextLine();
+            //if goodbye
+            if (line.equals("bye")) {
+                return;
+            }
+            //if want to list out tasks or mark as done or add items
+            if (line.equals("list")) {
+                for (int i = 0; i < taskNumber; i++) {
+                    System.out.println((i + 1) + "." + tasks[i]);
+                }
+            } else if (line.startsWith("done")) {
+                int index = Integer.parseInt(line.substring(5)) - 1;
+                tasks[index].setDone();
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  " + tasks[index].getStatusIcon() + " " + tasks[index].getDescription());
+            } else if (line.startsWith("todo")) {
+                tasks[taskNumber] = new ToDo(line.substring(5));
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            } else if (line.startsWith("deadline")) {
+                String[] words = line.split(" ");
+                int index = 0;
+                String deadlineDescription = "";
+                String by = "";
+                for (int i = 0; i < words.length; i++) {
+                    if (words[i].equals("/by")) {
+                        index = i;
+                        break;
+                    }
+                }
+                for (int i = 1; i < index; i++) {
+                    deadlineDescription = deadlineDescription + words[i] + " ";
+                }
+                by = words[index + 1];
+                tasks[taskNumber] = new Deadline(deadlineDescription, by);
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            } else if (line.startsWith("event")) {
+                String[] words = line.split(" ");
+                int index = 0;
+                String eventDescription = "";
+                String at = "";
+                for (int i = 0; i < words.length; i++) {
+                    if (words[i].equals("/at")) {
+                        index = i;
+                        break;
+                    }
+                }
+                for (int i = 1; i < index; i++) {
+                    eventDescription = eventDescription + words[i] + " ";
+                }
+                for (int i = index + 1; i < words.length; i++) {
+                    at = at + words[i] + " ";
+                }
+                tasks[taskNumber] = new Event(eventDescription, at);
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            }
+        }
+    }


Method is over 30 lines of code. It would not fit into the computer screen making it hard to debug and read. Maybe split the method into smaller methods.

AnShengLee · 2021-09-02T04:51:08Z

src/main/java/Deadline.java

+public class Deadline extends Task {
+
+    protected String by;
+
+    public Deadline(String description, String by) {
+        super(description);
+        this.by = by;
+    }
+
+    @Override
+    public String toString() {
+        return "[D]" + super.toString() + "(by: " + by + ")";
+    }
+}


Clear! Reader would not be confused.

AnShengLee · 2021-09-02T04:52:15Z

src/main/java/ToDo.java

+public class ToDo extends Task{
+    public ToDo(String description) {
+        super(description);
+    }
+
+    @Override
+    public String toString() {
+        return "[T]" + super.toString();
+    }
+}


Clear! Reader would not be confused.

AnShengLee · 2021-09-02T04:55:24Z

src/main/java/Task.java

+public class Task {
+    protected String description;
+    protected boolean isDone;
+
+    public Task(String description) {
+        this.description = description;
+        this.isDone = false;
+    }
+
+    public void setDone() {
+        this.isDone = true;
+    }
+
+    public String getDescription() {
+        return this.description;
+    }
+
+    public String getStatusIcon() {
+        return (isDone ? "[X]" : "[ ]"); // mark done task with X
+    }
+
+    @Override
+    public String toString() {
+        return this.getStatusIcon() + " " + this.getDescription();
+    }
+}


Clear! Reader would not be confused.

AnShengLee · 2021-09-02T04:57:33Z

src/main/java/Duke.java

+            if (line.equals("list")) {
+                for (int i = 0; i < taskNumber; i++) {
+                    System.out.println((i + 1) + "." + tasks[i]);
+                }
+            } else if (line.startsWith("done")) {
+                int index = Integer.parseInt(line.substring(5)) - 1;
+                tasks[index].setDone();
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  " + tasks[index].getStatusIcon() + " " + tasks[index].getDescription());
+            } else if (line.startsWith("todo")) {
+                tasks[taskNumber] = new ToDo(line.substring(5));
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            } else if (line.startsWith("deadline")) {
+                String[] words = line.split(" ");
+                int index = 0;
+                String deadlineDescription = "";
+                String by = "";
+                for (int i = 0; i < words.length; i++) {
+                    if (words[i].equals("/by")) {
+                        index = i;
+                        break;
+                    }
+                }
+                for (int i = 1; i < index; i++) {
+                    deadlineDescription = deadlineDescription + words[i] + " ";
+                }
+                by = words[index + 1];
+                tasks[taskNumber] = new Deadline(deadlineDescription, by);
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            } else if (line.startsWith("event")) {
+                String[] words = line.split(" ");
+                int index = 0;
+                String eventDescription = "";
+                String at = "";
+                for (int i = 0; i < words.length; i++) {
+                    if (words[i].equals("/at")) {
+                        index = i;
+                        break;
+                    }
+                }
+                for (int i = 1; i < index; i++) {
+                    eventDescription = eventDescription + words[i] + " ";
+                }
+                for (int i = index + 1; i < words.length; i++) {
+                    at = at + words[i] + " ";
+                }
+                tasks[taskNumber] = new Event(eventDescription, at);
+                taskNumber++;
+                System.out.println("Got it. I've added this task:");
+                System.out.println("  " + tasks[taskNumber - 1]);
+                System.out.println("Now you have " + taskNumber + " tasks in the list");
+            }


Good work on avoiding deep nested conditionals!

kewenlok

In general, I think you did a great job in complying with the coding standards! I have pointed out some minor changes that you could make to enhance the quality of your code. Keep up the good work!

kewenlok · 2021-09-17T12:20:07Z

src/main/java/duke/command/Duke.java

+        while (true) {
+            line = in.nextLine();
+            if (line.equals("bye")) {
+                return;
+            }
+            if (line.equals("list")) {
+                listOut(tasks, taskNumber);
+            } else if (line.startsWith("done")) {
+                markDone(line, tasks);
+            } else if (line.startsWith("todo")) {
+                if (addTodo(line, tasks, taskNumber)) {
+                    taskNumber++;
+                }
+            } else if (line.startsWith("deadline")) {
+                if (addDeadline(line, tasks, taskNumber)) {
+                    taskNumber++;
+                }
+            } else if (line.startsWith("event")) {
+                if (addEvent(line, tasks, taskNumber)) {
+                    taskNumber++;
+                }
+            } else {
+                System.out.println("☹ OOPS!!! I'm sorry, but I don't know what that means :-(");
+            }
+        }


You could consider splitting this functionality into another method for better readability and reusability of your code.

kewenlok · 2021-09-17T12:21:37Z

src/main/java/duke/command/Duke.java

+        tasks[taskNumber] = new Event(eventDescription, at);
+        taskNumber++;
+        System.out.println("Got it. I've added this task:");
+        System.out.println("  " + tasks[taskNumber - 1]);


Should the magic literal 1 be declared as a constant instead so that it is easier for you and other programmers to refer to your code in the future? This applies to all magic literals across the code.

kewenlok · 2021-09-17T12:24:31Z

src/main/java/duke/command/Duke.java

+        String[] words = line.split(" ");
+        int index = 0;
+        String deadlineDescription = "";
+        String by = "";
+        for (int i = 0; i < words.length; i++) {
+            if (words[i].equals("/by")) {
+                index = i;
+                break;
+            }
+        }
+        if (index == 0) {
+            System.out.println("OOPS!!! The description of a deadline cannot be empty.");
+            return false;
+        }
+        for (int i = 1; i < index; i++) {
+            deadlineDescription = deadlineDescription + words[i] + " ";
+        }
+        for (int i = index + 1; i < words.length; i++) {
+            by = by + words[i] + " ";
+        }
+        tasks[taskNumber] = new Deadline(deadlineDescription, by);
+        taskNumber++;
+        System.out.println("Got it. I've added this task:");
+        System.out.println("  " + tasks[taskNumber - 1]);
+        System.out.println("Now you have " + taskNumber + " tasks in the list");
+        return true;


You may want to consider splitting this method into multiple smaller methods that do smaller tasks and call it from this method. This goes the same for the other tasks-related methods.

kewenlok · 2021-09-17T12:25:42Z

src/main/java/duke/command/Duke.java

+
+
+public class Duke {
+    public static void listOut(Task[] tasks, int taskNumber) {


Consider using a clearer naming convention for your method such as printTasks().

kewenlok · 2021-09-17T12:27:01Z

src/main/java/duke/task/Deadline.java

+        this.by = by;
+    }
+
+    @Override


Good use of the override statement. Well done!

kewenlok · 2021-09-17T12:29:15Z

src/main/java/duke/task/ToDo.java

@@ -0,0 +1,12 @@
+package duke.task;
+
+public class ToDo extends Task{


Do take note of the spacing for each bracket and curly braces for the consistency of the whole program.

kewenlok · 2021-09-17T12:30:56Z

src/main/java/duke/command/Duke.java

+        while (true) {
+            line = in.nextLine();
+            if (line.equals("bye")) {
+                return;


In general, we do not use a return statement in a void function. If you want to break out of the current loop, use the "break" statement instead.

add the find feature (level 9)

# Conflicts: # src/main/java/duke/command/TaskList.java

Branch a java doc

silinche added 9 commits August 20, 2021 15:20

add increment level 0

8bf8991

added increment 1

5876d3a

added increment 2

02672d8

added increment 3

d75ba2c

fix minor bug for increment 3

c77d04f

update on A-CodingStandard

308eacf

Increment 4

97a6781

fixed bugs for level-4

64e85b9

fixed minor bugs for increment level 4

48ce882

kelvneo reviewed Sep 2, 2021

View reviewed changes

AnShengLee reviewed Sep 2, 2021

View reviewed changes

silinche added 6 commits September 8, 2021 23:44

commit text ui testing

653a21d

create more functions to comply with code quality standards

7f61f2d

added dealing with errors

dfcf04b

Merge branch 'branch-Level-5'

ccd2556

added packaging

b4c1c68

added level 6 deletion function

7f4ad3f

kewenlok reviewed Sep 17, 2021

View reviewed changes

silinche and others added 12 commits September 17, 2021 21:32

added the save to file feature

06d40a3

Merge branch 'branch-Level-6'

d308886

Merge branch 'branch-Level-7'

2c8103d

created Storage class to achieve more oop

2a89619

added ui class to achieve more oop

d41de47

added more classes to achieve more oop

3e52e94

added the find feature (level 9)

8320297

added javadoc

fd35808

Merge pull request #1 from silinche/branch-Level-9

c492a3b

add the find feature (level 9)

Merge branch 'master' into branch-A-JavaDoc

e61603d

# Conflicts: # src/main/java/duke/command/TaskList.java

Merge pull request #2 from silinche/branch-A-JavaDoc

1e115d9

Branch a java doc

Merge branch 'master' of https://github.com/silinche/ip

2faad66

silinche and others added 4 commits October 1, 2021 15:23

Added user guide

bb57228

Set theme jekyll-theme-cayman

ebba67b

fixed issues with saving in files

999995d

Merge branch 'master' of https://github.com/silinche/ip

4168e98



		public class Duke {
		public static void listOut(Task[] tasks, int taskNumber) {

		@@ -0,0 +1,12 @@
		package duke.task;

		public class ToDo extends Task{

Conversation

silinche commented Sep 1, 2021

Uh oh!

kelvneo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnShengLee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kewenlok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants