[Liu Ze Hui] iP by liuzehui03 · Pull Request #192 · nus-cs2113-AY2324S2/ip

liuzehui03 · 2024-02-08T16:39:15Z

Added Level-0, Level-1, Level-2, Level-3

yeozhishen

LGTM! Your code mostly seems to follow the coding standards, all but one line appears to need changing!

yeozhishen · 2024-02-09T01:38:51Z

@@ -0,0 +1,32 @@
+public class Task {
+    protected static String[] tasks = new String[100];
+    protected static boolean[] tasksCompleted = new boolean[100];


whichTasksCompleted sounds like a better variable name as the variable name tasksCompleted sounds like an integer

yeozhishen

suggested extra comments for cleaner code

yeozhishen · 2024-02-09T05:29:17Z

-                + "| | | | | | | |/ / _ \\\n"
-                + "| |_| | |_| |   <  __/\n"
-                + "|____/ \\__,_|_|\\_\\___|\n";
+    public static void printLine() {


Should try to rename it to something that indicates it is also printing a newline ie)printlnLine

yeozhishen · 2024-02-09T05:29:39Z

+    public static void printLine() {
+        System.out.println("________________________________________");
+    }
+    public static void printShortLine() {


Should try to rename it to something that indicates it is also printing a newline ie)printlnShortLine

yeozhishen · 2024-02-09T05:32:46Z

+            printLine();
+            if (userInput.equals("list")) {
+                Task.printList();
+            } else if(userInput.matches("mark \\d+")) {


may want to set constants to identify the regex expression, as regex may not be intuitively readable

yeozhishen · 2024-02-09T05:32:57Z

+                Task.markDone(index-1);
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  [X] " + Task.tasks[index-1]);
+            } else if(userInput.matches("unmark \\d+")) {


may want to set constants to identify the regex expression, as regex may not be intuitively readable

runxinghuan

Generally quite good. However, I noted a few minor coding quality violations which I have recommended solutions for in the comments.

runxinghuan · 2024-02-09T09:59:10Z

+import java.util.Scanner;
+//so scanner class can be used
+
 public class Duke {


Consider changing the class name to Yoj? Since you named your bot to be this.

runxinghuan · 2024-02-09T10:04:26Z

+                String[] taskIndex = userInput.split(" ");
+                int index = Integer.parseInt(taskIndex[1]);


Consider changing these two lines to a method? So that you can use again in "unmark" condition as well.

runxinghuan · 2024-02-09T10:08:01Z

+                int index = Integer.parseInt(taskIndex[1]);
+                Task.markDone(index-1);
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  [X] " + Task.tasks[index-1]);


Since you already have the getStatusIcon method, why not use it here?

runxinghuan · 2024-02-09T10:24:25Z

+            } else if(userInput.matches("mark \\d+")) {
+                String[] taskIndex = userInput.split(" ");
+                int index = Integer.parseInt(taskIndex[1]);
+                Task.markDone(index-1);
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  [X] " + Task.tasks[index-1]);
+            } else if(userInput.matches("unmark \\d+")) {
+                String[] taskIndex = userInput.split(" ");
+                int index = Integer.parseInt(taskIndex[1]);
+                Task.markUndone(index-1);
+                System.out.println("OK, I've marked this task as not done yet:");
+                System.out.println("  [ ] " + Task.tasks[index-1]);


Consider extracting these lines to a method called markTask? So that you can have the same level of abstraction within a code fragment.

runxinghuan · 2024-02-09T10:41:07Z

+        taskNumber = 0;
+    }
+
+    public static void addTask(String userInput) {


I like the way you named your methods, which are all in camalCase. Good job!

Jai2501

Hi Ze Hui!

Good job on the work done so far!

I have reviewed your code and have left some comments. Please go through them and get back to me for any clarifications.

Thanks!

Jai2501 · 2024-02-25T22:05:36Z

+    public Deadline (String description, String by){
+        super(description);
+        this.by = by;
+    }


Advisable to have a uniform spacing strategy, to enhance code readability.

Suggested change

public Deadline (String description, String by){

super(description);

this.by = by;

}

public Deadline (String description, String by) {

super(description);

this.by = by;

}

Jai2501 · 2024-02-25T22:07:43Z

+    public String getAt(){
+        return at;
+    }
+
+    public String toString() {
+        return "[E] " + super.toString() + description + " (at: " + at + ")";
+    }
+}


It is usually a practise to use this.method() or this.variable when that method or variable belongs to the Class.

This change can be made at other places as well :)

Suggested change

public String getAt(){

return at;

}

public String toString() {

return "[E] " + super.toString() + description + " (at: " + at + ")";

}

}

public String getAt(){

return this.at;

}

public String toString() {

return "[E] " + super.toString() + this.description + " (at: " + this.at + ")";

}

}

Jai2501 · 2024-02-25T22:09:45Z

+    protected static String[] tasks = new String[100];
+    protected static boolean[] tasksCompleted = new boolean[100];


Please avoid the use of Magic Numbers. Instead declare a constant with the value you deem is right, and use that constant.

Or better, you may choose to use ArrayList which dynamically resizes as per need, and you may not require such constants. However, everything comes at a tradeoff, so you may do some research into Array vs ArrayList and choose what fits the best for your needs.

Reference: https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Jai2501 · 2024-02-25T22:12:44Z

+    protected static String[] tasks = new String[100];
+    protected static boolean[] tasksCompleted = new boolean[100];


For this line, I may suggest you to refactor Task Class to have a boolean variable isDone. As a new Task is created, that isDone variable is unique to that task and may be updated as required. This is also efficient as you don't have to maintain an extra Array.

Sometimes it may happen that, you delete a Task and you may forget to update it in this other Array, making your code fragile and more bug prone.

protected boolean isTaskDone;

Jai2501 · 2024-02-25T22:16:03Z

+
+        System.out.println("] " + description);
+    }
+}


Missing EOF. Add an extra empty line at the end of file (EOF), as this indicates the end of the file.

You may configure IntelliJ to add this automatically for you when hit CTRL/CMD + S.

Jai2501 · 2024-02-25T22:19:41Z

+            printLine();
+            if (userInput.equals("list")) {
+                Task.printList();
+            } else if(userInput.matches("mark \\d+")) {


You may want to declare all the regex expressions being used as constants. This also allows you to just update the constant and reuse that at multiple places, without needing to update it at every place you declared it initially.

Jai2501 · 2024-02-25T22:20:15Z

+        while(!userInput.equals("bye")) {
+            printShortLine();
+            System.out.println("added: " + userInput);
+            printLine();
+            if (userInput.equals("list")) {
+                Task.printList();
+            } else if(userInput.matches("mark \\d+")) {
+                String[] taskIndex = userInput.split(" ");
+                int index = Integer.parseInt(taskIndex[1]);
+                Task.markDone(index-1);
+                System.out.println("Nice! I've marked this task as done:");
+                System.out.println("  [X] " + Task.tasks[index-1]);
+            } else if(userInput.matches("unmark \\d+")) {
+                String[] taskIndex = userInput.split(" ");
+                int index = Integer.parseInt(taskIndex[1]);
+                Task.markUndone(index-1);
+                System.out.println("OK, I've marked this task as not done yet:");
+                System.out.println("  [ ] " + Task.tasks[index-1]);
+            } else {
+                Task.addTask(userInput);
+            }
+            userInput = in.nextLine();
+        }


This code block can be abstracted to improve the code readability. Usually, methods should not be longer than 30 Lines of Code.

Reference: https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods

# Conflicts: # src/main/java/Yoj/List.java

This reverts commit 0612989.

find task feature

liuzehui03 added 4 commits February 2, 2024 14:22

add Duke.java level 0

215dbb8

added level-1

da5bbbb

Level-2

5b9ead6

Level-3

bec25f4

yeozhishen reviewed Feb 9, 2024

View reviewed changes

runxinghuan reviewed Feb 9, 2024

View reviewed changes

liuzehui03 added 2 commits February 22, 2024 23:45

no message

6c1ed56

Level-5

5769d82

Jai2501 reviewed Feb 25, 2024

View reviewed changes

liuzehui03 added 20 commits March 6, 2024 03:09

changes to Deadline event and Todo

52ab112

testing

09dad06

handle exceptions

8934c19

package

5667b2a

Merge branch 'branch-Level-5'

4c010a4

rename package

1163469

rename package

ca4c4c6

Merge branch 'trial'

755cb31

created List.java

75014ee

changed to ArrayList

e3469f4

delete function fixed

96d7ab0

fixing save data

6e92e2d

Merge branch 'branch-Level-6'

058b7df

Merge branch 'branch-Level-7'

1f259ca

# Conflicts: # src/main/java/Yoj/List.java

fix save function

666aaa8

JAR file

12b1ddd

added Storage class

09c7795

added Ui class

ed304d3

added taskList and parser

a5c5aed

added find task feature

0612989

liuzehui03 and others added 13 commits March 8, 2024 10:30

Revert "added find task feature"

2df056b

This reverts commit 0612989.

find task feature

db07c45

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

f540c91

find task feature

fix some bugs

df0007c

added comments

6a14058

more comments

8c65295

Merge branch 'branch-A-JavaDoc'

84af7ed

finallsing documentation READme

306396e

updated link

674d503

no message

017bdec

no message

302e239

comment

5491c80

Merge remote-tracking branch 'origin/master'

5fd1291

		String[] taskIndex = userInput.split(" ");
		int index = Integer.parseInt(taskIndex[1]);

		protected static String[] tasks = new String[100];
		protected static boolean[] tasksCompleted = new boolean[100];

Uh oh!

Conversation

liuzehui03 commented Feb 8, 2024

Uh oh!

yeozhishen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeozhishen 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!

runxinghuan 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!

Jai2501 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