[Wang Chuhan] ip by Vincent6896 · Pull Request #179 · nus-cs2113-AY2122S1/ip

Vincent6896 · 2021-09-01T01:50:42Z

No description provided.

yeezao · 2021-09-02T04:58:51Z

@@ -0,0 +1,81 @@
+import java.lang.String;
+
+public class List {


I wouldn't recommend calling your class List - there's already a provided class within Java called List, and this could prove very confusing to readers. You might want to consider changing the name of this class to something else.

yeezao · 2021-09-02T05:00:18Z

+                + task.substring(task.indexOf(" "), task.indexOf("/"))
+                + "(" + task.substring(task.indexOf("/") + 1, task.indexOf("/") + 3)
+                + ": " + task.substring(task.indexOf("/") + 4) + ")";


This section looks quite confusing - what are the +1 and +3 and +4 doing there - what do they mean? You might want to define these magic variables before using them.

yeezao · 2021-09-02T05:01:30Z

+    public static void doneTask(String task) {
+        task = task.trim();
+        String str = "";
+        if (task != null && !"".equals(task)) {


You could consider replacing "".equals(task) with task.isEmpty() - it reads more sensibly.

yeezao · 2021-09-02T05:02:34Z

+        System.out.println("    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n     "
+                + "Here are the tasks in your list:");
+        for (int i = 0; i < count; i++) {
+            System.out.println("     " + list[i]);


You could consider defining some variable to deal with the huge block of spaces inside your println.

MaifengNg · 2021-09-02T06:31:45Z

+public class List {
+
+    public static String[] list = new String[100];
+    public static String[] done = new String[100];


Would be good to rename done to something like doneList

MaifengNg · 2021-09-02T06:41:19Z

+                + "    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~");
+        while (true) {
+            Scanner in = new Scanner(System.in);
+            String order;


variable name order does not break the coding standard but it seems very unintuitive to me, a better name would be maybe input or task

MaifengNg · 2021-09-02T06:42:01Z

+            List.addTask(order);
+            if (order.equals("list")) {
+                List.printTask();
+            }


Can consider using case inside of multiple ifs

MaifengNg · 2021-09-02T06:45:41Z

+                + task.substring(task.indexOf(" "));
+        }
+        count++;
+        if (count == 1) {


can consider putting this whole chunk in a method to reduce repeating statements

Could also use variables to represent the repeated print statements

iamabhishek98

I have added my suggestions. Please note that although I have not repeated myself, what I have commented in one place applies to the rest of the codebase as well.

iamabhishek98 · 2021-09-19T01:26:18Z

+                }
+                System.out.println(SPLIT_LINE);
+                System.out.println("    Got it. I've added this task:");
+                System.out.println("     " + list.get(list.size() - 1));


Avoid having such nested logic in print statements as the purpose of print statements is only to print and not do anything else. Seperate this out into separate lines

iamabhishek98 · 2021-09-19T01:27:20Z

+                            if (tasks[i].contains("X")) {
+                                Todo todo = new Todo(description);
+                                todo.description = todo.description.replaceFirst(" ", "X");
+                                todo.isDone = true;
+                                list.add(todo);
+                            } else {
+                                list.add(new Todo(description));
+                            }
+                            continue;
+                        case "D":
+                            String by = tasks[i].substring(tasks[i].indexOf(":") + 2, tasks[i].indexOf(")"));
+                            if (tasks[i].contains("X")) {
+                                Deadline deadline = new Deadline(description.substring(0, description.indexOf("(") - 1), by);
+                                deadline.description = deadline.description.replaceFirst(" ", "X");
+                                deadline.isDone = true;
+                                list.add(deadline);
+                            } else {
+                                list.add(new Deadline(description.substring(0, description.indexOf("(") - 1), by));
+                            }
+                            continue;
+                        case "E":
+                            String at = tasks[i].substring(tasks[i].indexOf(":") + 2, tasks[i].indexOf(")"));
+                            if (tasks[i].contains("X")) {
+                                Event event = new Event(description.substring(0, description.indexOf("(") - 1), at);
+                                event.description = event.description.replaceFirst(" ", "X");
+                                event.isDone = true;
+                                list.add(event);
+                            } else {
+                                list.add(new Event(description.substring(0, description.indexOf("(") - 1), at));
+                            }


Consider abstracting out the logic of each switch case statement into separate functions

iamabhishek98 · 2021-09-19T01:28:02Z

+                                event.isDone = true;
+                                list.add(event);
+                            } else {
+                                list.add(new Event(description.substring(0, description.indexOf("(") - 1), at));


As mentioned before, do not use nested logic like this as it makes code extremely hard to read.

iamabhishek98 · 2021-09-19T01:28:48Z

+                        switch (tasks[i].substring(2, 3)) {
+                        case "T":
+                            Todo todo = new Todo(description);
+                            todo.description = todo.description.replaceFirst(" ", "X");


Avoid magic strings like "X" - extract them out as constants and name them appropriately using proper naming conventions

iamabhishek98 · 2021-09-19T01:29:50Z

+    }
+
+    public static void printDoneList() {
+        if (doneList.size() == 0) {


You can also do something like:
if (!doneList.size()) {
}

iamabhishek98 · 2021-09-19T01:30:29Z

+    protected boolean isDone;
+
+    public List(String description) {
+        this.description = "[ ] " + description;


As mentioned earlier, avoid magic strings like "[ ] "

iamabhishek98 · 2021-09-19T01:31:37Z

+        if (task != null && !"".equals(task)) {
+            for (int i = 0; i < task.length(); i++) {
+                if (task.charAt(i) >= 48 && task.charAt(i) <= 57) {
+                    str = str + task.charAt(i);
+                }
+            }


Having so many nested if statements and loops can be hard to read. Consider abstracting it out into a different function

merge PR for findCommand

Merge PR for java doc

Vincent6896 and others added 6 commits August 26, 2021 22:13

Level-1

dddd0f9

List part

1d1da66

ip_new_version

cf34648

A-CodingStandard

e107d0f

Level-4

c43ed4d

A-CodeQuality

019d151

yeezao reviewed Sep 2, 2021

View reviewed changes

MaifengNg reviewed Sep 2, 2021

View reviewed changes

chuhan wang added 7 commits September 9, 2021 00:30

Level 5 and change the structure of the whole program

b4f3fee

Create two package: duke and duke.task

6f01f09

Get the changes on the code.(ArrayList)

7c994f7

Add save function

a58c319

Update the code styles

8da55c4

improve the code style

6970bce

Change the path of creating and reading files, improve the jar file

68001db

iamabhishek98 reviewed Sep 19, 2021

View reviewed changes

chuhan wang and others added 14 commits September 25, 2021 21:30

Make the project more OOP

6f20f32

Add the java doc to most public method

90cef46

merge PR for findCommand

4e69d8f

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

a2eb9e9

merge PR for findCommand

Merge PR for java doc

19e80e5

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

1418f66

Merge PR for java doc

Final update for IP

f731a33

update readme

5150571

update

456c48e

update

99a8451

final update

3c6b18e

update

426987c

update

9f49177

update jar

bd6823f

chuhan wang added 2 commits October 1, 2021 12:14

update

0634d16

update

81a33a6

Conversation

Vincent6896 commented Sep 1, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamabhishek98 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