[Xue Yushan] iP by Mist0919 · Pull Request #197 · nus-cs2113-AY2122S1/ip

Mist0919 · 2021-09-02T03:18:56Z

No description provided.

itsleeqian · 2021-09-02T04:32:43Z

src/main/java/list.java

+public class List {
+    private static String[] currentList = new String[100];
+    public static int listSize = 0;
+    private static int[] isDone = new int[100];


name of this array "int[] isDone" sounds like a boolean name.
maybe change the name to better represent what you are trying to store

itsleeqian · 2021-09-02T04:37:05Z

src/main/java/List.java

+        isDone[number-1] = 1;
+    }
+
+    public static void markedItems() {


name of this function "markedItems()" sounds like a list. maybe should make it sound like a verb instead

theodorekwok

Overall good job following most of the code guidelines. Do consider refactoring the functionalities into methods to reduce the number of conditionals

theodorekwok · 2021-09-02T04:53:45Z

src/main/java/Duke.java

+                        + "____________________________________________________________\n");
+                break;
+            }
+            else if (str.equals("list")) {


The else if should be on line 25, to follow the K&R egyption style bracketing, likewise for line 32.
It was done correctly on line 52

theodorekwok · 2021-09-02T04:54:53Z

src/main/java/Duke.java

+import java.lang.String;
+
 public class Duke {
    public static void main(String[] args) {


The main method exceeds 30 lines, consider refactoring the console input and output into separate methods

theodorekwok · 2021-09-02T04:57:55Z

src/main/java/List.java

+public class List {
+    private static String[] currentList = new String[100];
+    public static int listSize = 0;
+    private static int[] isDone = new int[100];


The value 100 is a magic number, consider extracting it as a constant variable

Agreed! Also make sure the constants are named using proper naming conventions.

theodorekwok · 2021-09-02T05:03:37Z

src/main/java/Duke.java

+                String nums = "";
+                for (int i = 0; i < str.length(); i++) {
+                    char c = str.charAt(i);
+                    if (Character.isDigit(c) == true) {
+                        nums += c;
+                    }
+                }
+                int index = Integer.parseInt(nums);
+                if (index > List.listSize) {
+                    System.out.println("____________________________________________________________\n"
+                            + "The index is out of range.\n"
+                            + "____________________________________________________________\n");
+                } else {
+                    List.mark(index);
+                    System.out.println("____________________________________________________________\n"
+                            + "Nice! I've marked this task as done: ");
+                    List.markedItems();
+                    System.out.println("____________________________________________________________");
+                }


The abstraction levels here are different violating the single level of abstraction principle (SLAP) from the for loop in line 34 to 39 and the method calling in line 46 and 49 respectively.
Consider abstracting the for loop in line 34 to 39 as a separate method that is responsible for the console input.

theodorekwok · 2021-09-02T05:06:28Z

src/main/java/list.java

+
+    public static void printItems() {
+        if (listSize == 0) {
+            System.out.println("The list is empty.");


The if and else makes the happy path not prominent.

Suggested change

System.out.println("The list is empty.");

System.out.println("The list is empty.");

return;

theodorekwok · 2021-09-02T05:10:31Z

src/main/java/List.java

+        }
+    }
+
+    public static void mark(int number) {


Consider renaming this method to make it more clear on what it does. For eg setTaskAsCompleted

nguyin · 2021-09-02T08:53:14Z

src/main/java/Duke.java

+                        + "Here are the tasks in your list: ");
+                List.printItems();
                System.out.println("____________________________________________________________");
            }


The bracket for the if-else statement should be on the same line as else, so the line should be :
} else {

nguyin · 2021-09-02T08:55:07Z

src/main/java/Duke.java

+                + "____________________________________________________________\n");
        Scanner input = new Scanner(System.in);
-        while (true){
+        while (true) {


I like that you added the spacing after for the while loop and the { bracket

A codeQuality

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-19T02:00:56Z

src/main/java/Duke.java

+        greeting();
+        ArrayList<Task> tasks = getDatafromFile("tasks");
+
+        if (tasks.size() == 0) {


You can also do something like:

if (!tasks.size()) { }

iamabhishek98 · 2021-09-19T02:01:40Z

src/main/java/Duke.java

+                try {
+                    printList(tasks);
+                } catch (DukeException d) {
+                    System.out.println(line
+                            + d.getMessage() + "\n" + line);
+                }
+            } else if (inputString.contains("done") == true) {
+                try {
+                    setDone(inputString, tasks);
+                } catch (IndexOutOfBoundsException i) {
+                    System.out.println(line
+                            + "☹ OOPS!!! The index is out of range.\n"
+                            + line);
+                } catch (NumberFormatException n) {
+                    System.out.println(line
+                            + "☹ OOPS!!! The index of done indstruction should be a number.\n"
+                            + line);
+                } catch (NullPointerException p) {
+                    System.out.println(line
+                            + "☹ OOPS!!! The index of done indstruction cannot be empty.\n"
+                            + line);
+                }
+            } else if (inputString.contains("delete") == true) {
+                try {
+                    deleteTask(inputString, tasks);
+                } catch (IndexOutOfBoundsException i) {
+                    System.out.println(line
+                            + "☹ OOPS!!! The index is out of range.\n"
+                            + line);
+                } catch (NumberFormatException n) {
+                    System.out.println(line
+                            + "☹ OOPS!!! The index of delete indstruction should be a number.\n"
+                            + line);
+                }
+            } else if (inputString.contains("clear") == true) {
+                clearList(tasks);
+            } else {
+                try {
+                    addTask(inputString, tasks);
+                }
+                catch(IndexOutOfBoundsException i) {
+                    String type = "";
+                    if (inputString.contains("todo")) {
+                        type = "todo";
+                    } else if (inputString.contains("deadline")) {
+                        type = "deadline";
+                    } else if (inputString.contains("event")) {
+                        type = "event";
+                    }
+                    if (type == "") {
+                        System.out.println(line
+                                + "☹ OOPS!!! I'm sorry, but I don't know what that means :-(\n"
+                                + line);
+                    } else {
+                        System.out.println(line
+                                + "☹ OOPS!!! The description of a " + type + " cannot be empty.\n"
+                                + line);
+                    }


Avoid using so many nested try catch blocks. Please abstract out the logic into different functions

iamabhishek98 · 2021-09-19T02:03:04Z

src/main/java/Duke.java

+                    System.out.println(line
+                            + d.getMessage() + "\n" + line);
+                }
+            } else if (inputString.contains("done") == true) {


There is no need to explicitly check if it is equal to true because the contains method returns a boolean value. You can change this to:

else if (inputString.contains("done")) { }

iamabhishek98 · 2021-09-19T02:05:40Z

src/main/java/Duke.java

+        ArrayList<Task> tasks = new ArrayList<Task>();
+        String path = "C:\\Users\\XUEY0013\\Documents\\ip\\src\\main\\" + fileName+ ".txt";
+        File file = new File(path);
+        if (!file.exists()) {
+            return new ArrayList<Task> ();
+        }
+        BufferedReader reader = null;
+        try {
+            FileInputStream fileInputStream = new FileInputStream(path);
+            InputStreamReader inputStreamReader = new InputStreamReader(fileInputStream, "UTF-8");
+            reader = new BufferedReader(inputStreamReader);
+            String tempString = "";
+            Task task = new Task("null");
+
+            while ((tempString = reader.readLine()) != null) {
+                if (tempString.charAt(TYPE_POS) == 'T') {
+                    task = (new Todo(tempString.substring(TASK_POS)));
+                } else if (tempString.charAt(TYPE_POS) == 'E') {
+                    String event = tempString.substring(TASK_POS,
+                            tempString.indexOf("(") - 1);
+                    String at = tempString.substring(tempString.indexOf(":") + 2,
+                            tempString.indexOf(")"));
+                    task = (new Event(event, at));
+                } else if (tempString.charAt(TYPE_POS) == 'D') {
+                    String deadline = tempString.substring(TASK_POS,
+                            tempString.indexOf("(") - 1);
+                    String by = tempString.substring(tempString.indexOf(":") + 2,
+                            tempString.indexOf(")"));
+                    task = (new Deadline(deadline, by));
+                }
+
+                if (tempString.charAt(7) == 'X') {
+                    task.markedAsDone();
+                }
+
+                tasks.add(task);
+            }
+            reader.close();
+        } catch (IOException e) {
+            e.printStackTrace();
+        } finally {
+            if (reader != null) {
+                try {
+                    reader.close();
+                } catch (IOException e) {
+                    e.printStackTrace();
+                }
+            }
+        }
+        return tasks;
+    }


Having such long functions can be hard to read. Please consider abstracting out repeated code in line with the DRY (Don't Repeat Yourself) principle. Also add comments to improve the readability of your code

iamabhishek98 · 2021-09-19T02:06:09Z

src/main/java/List.java

+public class List {
+    private static String[] currentList = new String[100];
+    public static int listSize = 0;
+    private static int[] isDone = new int[100];


Agreed! Also make sure the constants are named using proper naming conventions.

iamabhishek98 · 2021-09-19T02:07:01Z

src/main/java/List.java

+            for (int i = 1; i <= listSize; i++) {
+                System.out.print(i + ". " );
+                System.out.print("[");
+                if (isDone[i-1] == 1) {


Please maintain spacing between the operators and the operands

iamabhishek98 · 2021-09-19T02:08:09Z

src/main/java/Task.java

+    }
+
+    public final void markedAsDone() {
+        this.isDone = "X";


As mentioned earlier, avoid using magic strings - please extract them out as constants.

iamabhishek98 · 2021-09-19T02:09:55Z

src/main/java/list.java

+            type[listSize] = "T";
+        } else if (input.contains("deadline")) {
+            String by = input.substring(input.indexOf("/") + 4);
+            l[listSize] = input.substring(9, input.indexOf("/")) + " (by: " + by


Avoid having complex nested logic like this. Either separate it out into different lines or abstract it out into different functions

commit

Mist0919 added 5 commits August 26, 2021 22:51

Level 1

85d1982

Level 2

cac3d8e

Level 3

1fd8497

A-CodingStandard

6466eb7

commit

6141721

itsleeqian reviewed Sep 2, 2021

View reviewed changes

theodorekwok reviewed Sep 2, 2021

View reviewed changes

nguyin reviewed Sep 2, 2021

View reviewed changes

Mist0919 added 13 commits September 4, 2021 11:19

level4

babe161

A codeQuality

Level 5

d198914

Level 5

1a6a0ac

Add branch

9f131cc

Add DukeException

7cac7da

Add exceptions

6be3ec0

Add exception

5aa495e

Add exceptions

ecee40d

Level 6

1b59f78

Level 7

8cfb82a

Changes.

e060cf4

Changed level7

0cf473f

change

b8d0935

iamabhishek98 reviewed Sep 19, 2021

View reviewed changes

Mist0919 and others added 7 commits September 22, 2021 23:18

Level 9

d69ddd3

Find

97a99f5

More OOP

3516ed4

More OOP final version

0d92829

A-JavaDoc

6c1b940

commit

27ea764

Merge pull request #1 from Mist0919/branch-A-JavaDoc

3a00f4e

commit

Mist0919 added 3 commits October 1, 2021 10:25

Update User Guide and correct some minor errors

329a47e

Merge branch 'master' of github.com:Mist0919/ip

de1b451

Final correction

22393ec

	System.out.println("The list is empty.");
	System.out.println("The list is empty.");
	return;

Conversation

Mist0919 commented Sep 2, 2021

Uh oh!

itsleeqian Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

itsleeqian Sep 2, 2021 •

edited

Loading