[Sim Qian Hui] iP #58

qianz-z · 2022-08-29T16:01:50Z

No description provided.

AaaaaronC

Good coding practices, only a few minor naming issues

AaaaaronC · 2022-09-01T09:49:30Z

src/main/java/Duke.java

+                printAddItemText(newTodo);
+                break;
+            case "deadline":
+                String[] taskWithDeadline = inputWords[1].split("/by ", 2);


collections can be named as plural to distinguish from Strings instead of String[]

AaaaaronC · 2022-09-01T09:50:33Z

src/main/java/Duke.java

+                printAddItemText(newDeadlineTask);
+                break;
+            case "event":
+                String[] eventTask = inputWords[1].split("/at ", 2);


same thing here, the naming for eventTask follows that of other Strings, which make it slightly confusing

AaaaaronC · 2022-09-01T09:52:00Z

src/main/java/Task.java

+        return taskType;
+    }
+
+    public String printTask() {


code here can be condensed more

AaaaaronC · 2022-09-01T09:53:45Z

src/main/java/Duke.java

+        System.out.println("____________________________________________________________");
+    }
+
+    public static void printGreeting() {


good that helper functions are used to make code neater subsequently

src/main/java/Deadline.java

shengiv

In general, code quality was well-maintained. Good job.

shengiv · 2022-09-01T09:59:47Z

src/main/java/Duke.java

+        System.out.println("____________________________________________________________");
+    }
+
+    public static void printGreeting() {


Consider adding comments to describe what the method does

shengiv · 2022-09-01T10:09:58Z

src/main/java/Duke.java

+        if (previousIcon == "X") {
+            if (isDone) {
+                System.out.println("This task has already been marked!");
+            } else {
+                System.out.println("OK, I've marked this task as not done yet:");
+            }
+        } else {
+            if (!isDone) {
+                System.out.println("This task has already been unmarked!");
+            } else {
+                System.out.println("Nice! I've marked this task as done:");
+            }
+        }


The code can be restructured to make the happy path unindented as much as possible.

shengiv · 2022-09-01T10:18:10Z

src/main/java/Duke.java

+    public static void printAddItemText(Task input) {
+        System.out.println("Got it. I've added this task:");
+        System.out.println(input.printTask());
+        System.out.println("Now you have " + taskSize + " tasks in the list.");
+        printHorizontalLine();
+    }


Good job on extracting out the print statements to print methods

shengiv · 2022-09-01T10:22:30Z

src/main/java/Duke.java

+            case "deadline":
+                String[] taskWithDeadline = inputWords[1].split("/by ", 2);
+                String taskDescription = taskWithDeadline[0];
+                String taskDeadline = taskWithDeadline[1];
+                Deadline newDeadlineTask = new Deadline(taskDescription, 'D', taskDeadline);
+                addItem(newDeadlineTask);
+                printAddItemText(newDeadlineTask);


Avoid varying levels of abstraction to improve readability

hughjazzman

Keep coding! 👍

hughjazzman · 2022-09-12T15:24:16Z

src/main/java/Duke.java

+                printAddItemText(newTodo);
+                break;
+            case "deadline":
+                String[] taskWithDeadline = inputWords[1].split("/by ", 2);


Could 2 be a magic number here?

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

hughjazzman

Here are some comments after your commits 👍

hughjazzman · 2022-09-18T14:53:29Z

src/main/java/Duke/Duke.java

@@ -0,0 +1,213 @@
+package Duke;
+
+import Duke.Tasks.*;


Do list imported classes explicitly, you might have to change IntelliJ settings.

https://se-education.org/guides/conventions/java/basic.html#statements

hughjazzman · 2022-09-18T14:55:20Z

src/main/java/Duke/Duke.java

+    }
+
+    private static void loadTasksToTasksList() {
+        try {


Perhaps you can limit the scope of try-except?

https://nus-cs2113-ay2223s1.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-deep-nesting

hughjazzman · 2022-09-18T14:56:32Z

src/main/java/Duke/Duke.java

+        }
+    }
+
+    public static void main(String[] args) throws EmptyArgumentException, InvalidCommandFormatException, TaskListEmptyException, TaskNumberOutOfBoundsException, IOException, TaskNumberNotNumberException {


Maybe you can break up this long method?

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

hughjazzman · 2022-09-18T14:57:09Z

src/main/java/Duke/Exceptions/DukeException.java

@@ -0,0 +1,8 @@
+package Duke.Exceptions;
+public class DukeException extends Exception {
+//    protected final String MESSAGE = "☹ OOPS!!! I'm sorry, but I don't know what that means :-(";


Consider deleting this dead code.

https://nus-cs2113-ay2223s1.github.io/website/se-book-adapted/chapters/codeQuality.html#delete-dead-code

hughjazzman · 2022-09-18T14:58:07Z

src/main/java/Duke/Exceptions/TaskAlreadyMarkedException.java

+        if (markStatus.equals("marked")) {
+            return ERROR_MESSAGE + markStatus + "!";
+        } else {
+            return ERROR_MESSAGE + markStatus + "!";
+        }


Seems the if-else does not do anything?

Suggested change

if (markStatus.equals("marked")) {

return ERROR_MESSAGE + markStatus + "!";

} else {

return ERROR_MESSAGE + markStatus + "!";

}

return ERROR_MESSAGE + markStatus + "!";

hughjazzman · 2022-09-18T14:59:39Z

src/main/java/Duke/Tasks/TasksList.java

+        } else {
+            System.out.println("Here are the tasks in your list:");
+            for (int i = 0; i < getTasksListSize(); i++) {
+                System.out.println((i+1) + "." + tasksList.get(i).taskStatusWithDescriptionText());


Consider adding space between operators.

Suggested change

System.out.println((i+1) + "." + tasksList.get(i).taskStatusWithDescriptionText());

System.out.println((i + 1) + "." + tasksList.get(i).taskStatusWithDescriptionText());

Operators should be surrounded by a space character.

https://se-education.org/guides/conventions/java/index.html#layout

Add a find function

qian added 16 commits August 25, 2022 20:17

Created printGreeting()

ec14505

Echo and Exit functions

2b4ae68

Created add item function

4a5daca

Update numberOfTexts and created print list function

c47b566

Added Task class

87b26e2

Update print task format with checkbox

1c75097

Update exiting of Duke

ef2f4d6

Creating mark functions and update taskList for 1-index

4516b80

Update markTask to also include unmarking functions

8cbe414

Tweak to comply coding standard

aaf22a2

Check taskSize before printing task list

02c686e

Update for coding standard and check if task has been marked before

6813e6e

Change to switch cases instead of if-else

7226c7c

Added Todo function

0eb754d

Add Deadline function

790f4f9

Add Events function

bc1e013

AaaaaronC reviewed Sep 1, 2022

View reviewed changes

shengiv reviewed Sep 1, 2022

View reviewed changes

qian added 4 commits September 2, 2022 00:29

Improve code quality

fbae349

Fixed bugs in the code

85b650b

Remove package and added constant

1626396

Create new TasksList class

542406f

hughjazzman reviewed Sep 12, 2022

View reviewed changes

qian added 7 commits September 14, 2022 12:04

TaskListEmpty, TaskNumberOutOfBounds, EmptyArgument Exceptions

56d1923

Moved try-catch code from Duke to TasksList

3c222d7

Invalid Command Format for todo, deadline and event

f88e676

Exception for tasks that has already been marked

12593c4

Divide classes into packages (Tasks and Exceptions)

a5272ba

Code cleanup

6ab214b

Delete task function

afc36dc

hughjazzman reviewed Sep 18, 2022

View reviewed changes

qian and others added 29 commits September 28, 2022 00:11

Code cleanup

b27d7cb

Added Storage and Ui Classes

67270f7

Added Parser and Tasklist classes

583f228

Add in reading of input in Ui

ab71c49

Add a find function

7fc9636

Merge pull request #1 from qianz-z/branch-Level-9

e2a2c3e

Add a find function

User Guide and Quick Start on README

4bde99b

Important thing to note for features

2000879

Update features in README

db096e2

Command Summary in README

17d79e5

Updated Ui print functions

285195a

Added Command classes

bcb8def

Update Storage print functions in Ui

571122f

Add Command Words

6322116

Gives Bob personality

c275058

Add JavaDoc comments to the code

c51a211

Error catching and update on JavaDoc

e81f37b

Code cleanup (removal of unused imports)

a70b07e

Update with Bob's Personality

d2ec93e

Swap the order of list and mark and unmark

bc3ab57

Minor code cleanup

8c4748e

Merge branch 'master' of https://github.com/qianz-z/ip

ae70367

Updated command summary

6660b32

Code cleanup with coding standards followed

23f1f34

Merge branch 'master' of https://github.com/qianz-z/ip

22cc366

Update find function

e96f8c2

Update Command Summary

efa7eb8

More error catching

4dd7c14

Merge branch 'master' of https://github.com/qianz-z/ip

8ae5944

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sim Qian Hui] iP #58

[Sim Qian Hui] iP #58

qianz-z commented Aug 29, 2022

AaaaaronC left a comment

AaaaaronC Sep 1, 2022

AaaaaronC Sep 1, 2022

AaaaaronC Sep 1, 2022

AaaaaronC Sep 1, 2022

shengiv left a comment

shengiv Sep 1, 2022

shengiv Sep 1, 2022

shengiv Sep 1, 2022

shengiv Sep 1, 2022

hughjazzman left a comment

hughjazzman Sep 12, 2022

hughjazzman left a comment

hughjazzman Sep 18, 2022

hughjazzman Sep 18, 2022

hughjazzman Sep 18, 2022

hughjazzman Sep 18, 2022

hughjazzman Sep 18, 2022

hughjazzman Sep 18, 2022

	System.out.println((i+1) + "." + tasksList.get(i).taskStatusWithDescriptionText());
	System.out.println((i + 1) + "." + tasksList.get(i).taskStatusWithDescriptionText());

[Sim Qian Hui] iP #58

Are you sure you want to change the base?

[Sim Qian Hui] iP #58

Conversation

qianz-z commented Aug 29, 2022

AaaaaronC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shengiv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughjazzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughjazzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment