[Shawn Tan Jinhui] iP by GitPancaked · Pull Request #56 · nus-cs2113-AY2223S1/ip

GitPancaked · 2022-08-29T15:50:30Z

No description provided.

bdthanh

Looks good! You should check the names of constants

bdthanh · 2022-08-31T04:47:00Z

-                + "| |_| | |_| |   <  __/\n"
-                + "|____/ \\__,_|_|\\_\\___|\n";
+
+    static String logo = " ____        _        \n"


Should add private to be clearer and constant name should be writen in uppercase LOGO

bdthanh · 2022-08-31T04:48:52Z

+        System.out.println(linebreak);
+    }
+
+    static void markTasks(ArrayList<Task> Tasks, String text) {


This function should be writen in class Task

abhityakrishnaraj

Overall good job, but I think some things are overcomplicated

abhityakrishnaraj · 2022-08-31T05:37:20Z

+        else if (result[0].equals("event")) {
+            newEvent = new Event(result3[0].substring(6), result3[1]);
+            Tasks.add(newEvent);
+        }


Have an else statement to catch any incorrect inputs here

abhityakrishnaraj · 2022-08-31T05:38:59Z

+        if (result[0].equals("todo")) {
+            System.out.println(newTodo);
+        }
+        else if (result[0].equals("deadline")) {
+            System.out.println(newDeadline);
+        }
+        else if (result[0].equals("event")) {
+            System.out.println(newEvent);
+        }


You should be able to just print out tasks using a to string method instead of rechecking.

abhityakrishnaraj · 2022-08-31T05:40:04Z

        int count = 1;
        for (Task i : Tasks) {
-            System.out.println(String.valueOf(count) + "." + "[" + i.getStatusIcon() + "] " + i.getDescription());
+            System.out.println(String.valueOf(count) + "." + i);


You don't need valueOf, you can just do count+"."+i

abhityakrishnaraj · 2022-08-31T05:43:05Z

    }
+
+    public String toString() {
+        return "[" + this.getStatusIcon() + "] " + this.getDescription();


You can just use description instead of this.getDescription

dZhei98

Good job so far!

dZhei98 · 2022-08-31T05:49:53Z

-                + "| |_| | |_| |   <  __/\n"
-                + "|____/ \\__,_|_|\\_\\___|\n";
+
+    static String logo = " ____        _        \n"


You may want to consider setting the logo to be of type private static final. Constant names should be in capital letters.

dZhei98 · 2022-08-31T05:52:32Z

+            + "| | | | | | | |/ / _ \\\n"
+            + "| |_| | |_| |   <  __/\n"
+            + "|____/ \\__,_|_|\\_\\___|\n";
+    static String linebreak = "____________________________________________________________";


Same as above for this, you can consider making the type private static final and capitalise "linebreak"

dZhei98 · 2022-08-31T05:53:15Z

+        line = in.nextLine();
+
+        while (!line.equals("bye")) {
+            if (line.length() > 3 && line.substring(0, 4).equals("mark")) {


You should avoid complicated expressions as per https://nus-cs2113-ay2223s1.github.io/website/se-book-adapted/chapters/codeQuality.html

dZhei98 · 2022-08-31T05:53:55Z

+        while (!line.equals("bye")) {
+            if (line.length() > 3 && line.substring(0, 4).equals("mark")) {
+                markTasks(Tasks, line);
+            } else if (line.length() > 4 && line.substring(0, 6).equals("unmark")) {


You can consider removing the magic number 4 and making it a named constant

dZhei98 · 2022-08-31T05:54:36Z

+    static void storeTasks(ArrayList<Task> Tasks, String text) {
+        System.out.println(linebreak);
+        String[] result = text.split(" ");
+        String[] result2 = text.split("/by ");
+        String[] result3 = text.split("/at ");
+        Todo newTodo = null;
+        Deadline newDeadline = null;
+        Event newEvent = null;
+        if (result[0].equals("todo")) {
+            newTodo = new Todo(result[1]);
+            Tasks.add(newTodo);
+        }
+        else if (result[0].equals("deadline")) {
+            newDeadline = new Deadline(result2[0].substring(9), result2[1]);
+            Tasks.add(newDeadline);
+        }
+        else if (result[0].equals("event")) {
+            newEvent = new Event(result3[0].substring(6), result3[1]);
+            Tasks.add(newEvent);
+        }


You may want to adhere to SLAP and shift this to a new class

okkhoy

There are a few issues related to coding standards and code readability as indicated in individual comments. Try to improve on those aspects.

okkhoy · 2022-09-18T11:30:58Z

@@ -0,0 +1,14 @@
+public class Deadline extends Task {


Consider adding header comments to non-trivial classes and methods

okkhoy · 2022-09-18T11:31:20Z

@@ -0,0 +1,14 @@
+public class Deadline extends Task {
+
+    protected String by;


Why is this protected?

okkhoy · 2022-09-18T11:32:07Z

+            if (line.length() > 3 && line.startsWith("mark")) {
+
+                Manager.markTasks(line);
+            } else if (line.length() > 4 && line.startsWith("unmark")) {
+                Manager.unmarkTasks(line);
+            } else if (line.equals("list")) {
+                Manager.printTasks();
+            } else {
+                Manager.storeTasks(line);
+            }
+            line = in.nextLine();


You can improve SLAP here by extracting out some of the parsing to a different class (or minimally different method)

okkhoy · 2022-09-18T11:33:16Z

@@ -0,0 +1,2 @@
+public abstract class DukeExceptions {


Hmmm... just a DukeExceptions may not mean anything much. If you are intending to write custom exceptions, consider extending the Exceptions class.

okkhoy · 2022-09-18T11:33:39Z

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


Similar comments as Deadline class

okkhoy · 2022-09-18T11:34:42Z

+        }
+        else if (result[0].equals("deadline")) {
+            try {
+                String x=result[1];
+            }
+            catch (IndexOutOfBoundsException e) {
+                System.out.println("Deadlines, that's all life's about, but you gotta tell me which!");
+                return;
+            }
+            newDeadline = new Deadline(result2[0].substring(9), result2[1]);
+            Tasks.add(newDeadline);
+        }
+        else if (result[0].equals("event")) {
+            try {
+                String x=result[1];
+            }
+            catch (IndexOutOfBoundsException e) {
+                System.out.println("Which event? Personally I find an uneventful life to be the key to longevity");


if ... else and try ... catch violate our coding standards

okkhoy · 2022-09-18T11:35:18Z

+        if (result[0].equals("todo")) {
+            try {
+                String x=result[1];
+            }
+            catch (IndexOutOfBoundsException e) {
+                System.out.println("Much ado about nothing");
+                return;
+            }
+            newTodo = new Todo(result[1]);


You can try to extract out parsing things to a separate method and improve SLAP here

okkhoy · 2022-09-18T11:36:11Z

+        String[] result = text.split(" ");
+        String[] result2 = text.split("/by ");
+        String[] result3 = text.split("/at ");


bad SLAP here; remember the usage of specific methods in the TaskS repo used in Lecture 6? that should give you an idea how to improve SLAP.

okkhoy · 2022-09-18T11:36:50Z

+                System.out.println("Deadlines, that's all life's about, but you gotta tell me which!");
+                return;
+            }
+            newDeadline = new Deadline(result2[0].substring(9), result2[1]);


always better to have clean, named variables for things you are passing as parameters; try to improve readability.

Level-9

branch-A-JavaDoc

…nputted by the user

GitPancaked added 5 commits August 19, 2022 00:45

Level-0

d0359f7

Level-1

c9f2e1e

Level-2

fde8640

Level-3

56588df

Level-4

f0667c3

bdthanh reviewed Aug 31, 2022

View reviewed changes

abhityakrishnaraj reviewed Aug 31, 2022

View reviewed changes

dZhei98 reviewed Aug 31, 2022

View reviewed changes

GitPancaked added 5 commits September 2, 2022 01:49

A-TextUiTesting

f3e4b39

A-CodeQuality

78191b9

Level-5

dde20aa

A-Exceptions

080fcc4

Merge branch 'branch-Level-5'

e998f99

okkhoy reviewed Sep 18, 2022

View reviewed changes

GitPancaked and others added 16 commits September 23, 2022 22:34

A-Packages

2fe4cbe

Level-6

38c43c5

Level-7

abbe7f1

Bug fixes on Storage file check

795a1c0

A-MoreOOP

fddcaf2

Level-9

f9427dd

Merge pull request #1 from GitPancaked/heads/Level-9

9fa95e9

Level-9

A-JavaDoc

66a27e2

bug fixes for find

762d6a2

Improvements

3224818

Formatting

1cda75f

More JavaDoc

14b0945

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

8859db0

branch-A-JavaDoc

Improvements and UG

b12b193

UG fix

88f220a

Improved SLAP and reduced long methods, fixed minor formatting

23dde3d

Improved find functionality to include searching for the whole text i…

4a95f15

…nputted by the user

		@@ -0,0 +1,14 @@
		public class Deadline extends Task {

		protected String by;

Uh oh!

Conversation

GitPancaked commented Aug 29, 2022

Uh oh!

bdthanh left a comment

Choose a reason for hiding this comment

Uh oh!

bdthanh Aug 31, 2022 • 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!

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

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bdthanh Aug 31, 2022 •

edited

Loading