[Devesh Logendran] iP #63

deveshl · 2022-08-29T19:39:21Z

No description provided.

jorellesee

Overall great work! But consider using lambdas in different lines instead to make the code more readable

jorellesee · 2022-08-31T04:47:48Z

src/main/java/Duke.java

+
+            Task task = tasks.get(taskIndex);
+            task.setDone(isDone);
+            System.out.println("Okay, dude, I've marked this task as " + (isDone ? "done" : "not done yet") + ": ");


Perhaps the use of a lambda here makes the code slightly less readable

jorellesee · 2022-08-31T04:48:32Z

src/main/java/Duke.java

+            "$$ |  $$ |$$ |  $$ |$$ |  $$ |$$   ____|\n" +
+            "$$$$$$$  |\\$$$$$$  |\\$$$$$$$ |\\$$$$$$$\\ \n" +
+            "\\_______/  \\______/  \\_______| \\_______|";
+    static final String GREET = "I am Dude.\n" +


I like the use of all caps here to tell the reader that this is a constant. Good job!

jorellesee · 2022-08-31T04:48:42Z

src/main/java/Duke.java

+            "\\_______/  \\______/  \\_______| \\_______|";
+    static final String GREET = "I am Dude.\n" +
+            "Type something in, dude.";
+    static final String BYE = "Catch you later, dude.";


jorellesee · 2022-08-31T04:49:33Z

src/main/java/Duke.java

+        case "list":
+            for (int i = 0; i < tasks.size(); i++) {
+                Task task = tasks.get(i);
+                System.out.printf("%d.[%s] %s\n", i + 1, (task.isDone() ? "X" : " "), task.getDescription());


Perhaps more could be done to make this line more readable? Consider not using the lambda or putting it in a different line.

I agree, it would be great to make this line more understandable

alfred-leong

Looks good overall, maybe some minor changes to some variables and code to make it more readable would be aweomse!

alfred-leong · 2022-08-31T05:39:10Z

src/main/java/Duke.java

+            tasks.add(task);
+            System.out.println("added: " + command);
+            break;
+        }


hey I like how clean and readable your code is!

alfred-leong · 2022-08-31T05:40:25Z

src/main/java/Duke.java

+        case "list":
+            for (int i = 0; i < tasks.size(); i++) {
+                Task task = tasks.get(i);
+                System.out.printf("%d.[%s] %s\n", i + 1, (task.isDone() ? "X" : " "), task.getDescription());


I agree, it would be great to make this line more understandable

alfred-leong · 2022-08-31T05:41:04Z

src/main/java/Duke.java

+        case "bye":
+            System.out.println(BYE);
+            System.exit(0);
+            // no break needed, the code has already exited


Great comment to help the reader understand what happens after the exit command 👍

alfred-leong · 2022-08-31T05:42:22Z

src/main/java/Duke.java

+
+    public static void runCommand(String command) {
+        String[] args = command.split(" ");
+        switch (args[0]) {


It would be great to rename "args[0]" to a name that is easy for the reader to understand the variable

ongzhihong · 2022-08-31T14:09:17Z

src/main/java/Duke.java

+    public static void runCommand(String command) {
+        String[] args = command.split(" ");
+        switch (args[0]) {
+        case "bye":
+            System.out.println(BYE);
+            System.exit(0);
+            // no break needed, the code has already exited
+        case "list":
+            for (int i = 0; i < tasks.size(); i++) {
+                Task task = tasks.get(i);
+                System.out.printf("%d.[%s] %s\n", i + 1, (task.isDone() ? "X" : " "), task.getDescription());
+            }
+            break;
+        case "mark":
+        case "unmark": {
+            boolean isDone = args[0].equals("mark");
+            int taskIndex = Integer.parseInt(args[1]) - 1; // add code to handle oob, missing argument
+
+            Task task = tasks.get(taskIndex);
+            task.setDone(isDone);
+            System.out.println("Okay, dude, I've marked this task as " + (isDone ? "done" : "not done yet") + ": ");
+            System.out.println(task.getDescription());
+            break;
+        }
+        default: {
+            Task task = new Task(command);
+            tasks.add(task);
+            System.out.println("added: " + command);
+            break;
+        }
+        }
+    }


This method runCommand is 31 lines long, excluding the ending curly braces it is still 29. A rather lengthy method and you can possibly improve code quality if you take corrective action.

ongzhihong · 2022-08-31T14:09:55Z

src/main/java/Duke.java

+
+    static List<Task> tasks = new ArrayList<>();
+
+    public static void runCommand(String command) {


The naming of this method could be better, runCommand does not immediately explain the method to the reader accurately and at a sufficient level of detail.

ongzhihong · 2022-08-31T14:15:04Z

src/main/java/Duke.java

+    static final String LOGO = "$$$$$$$\\                  $$\\           \n" +
+            "$$  __$$\\                 $$ |          \n" +
+            "$$ |  $$ |$$\\   $$\\  $$$$$$$ | $$$$$$\\  \n" +
+            "$$ |  $$ |$$ |  $$ |$$  __$$ |$$  __$$\\ \n" +
+            "$$ |  $$ |$$ |  $$ |$$ /  $$ |$$$$$$$$ |\n" +
+            "$$ |  $$ |$$ |  $$ |$$ |  $$ |$$   ____|\n" +
+            "$$$$$$$  |\\$$$$$$  |\\$$$$$$$ |\\$$$$$$$\\ \n" +
+            "\\_______/  \\______/  \\_______| \\_______|";
+    static final String GREET = "I am Dude.\n" +
+            "Type something in, dude.";
+    static final String BYE = "Catch you later, dude.";


LOGO is okay but GREET and BYE in your context are verbs and they are a constant variable so they should be named like nouns. For eg, GREETING and GOODBYE

ongzhihong · 2022-08-31T14:21:27Z

src/main/java/Task.java

+    public void setDone(boolean isDone) {
+        this.isDone = isDone;
+    }
+
+    public boolean isDone() {


Good appropriate naming for boolean methods and setter for boolean variable. Makes the code very readable.

okkhoy

The code is generally quite well written. However, I do notice that you haven't completed some of the more recent tasks. Please catch up on the iP activities soon; do let me know if you need any help with the topics.

okkhoy · 2022-09-18T14:09:58Z

src/main/java/Duke.java

+            } catch (NumberFormatException e) {
+                // user either didn't enter a number or number was too big for integer
+                // not a nice way to use exceptions but alternatives are cumbersome
+                System.out.println("Invalid argument, dude.");


You could explain what you have in the comment in the error message.

okkhoy · 2022-09-18T14:10:49Z

src/main/java/Duke.java

+        case "todo":
+        case "deadline":
+        case "event": {


Need to include explicit fallthrough comment

okkhoy · 2022-09-18T14:11:56Z

src/main/java/Duke.java

+        case "deadline":
+        case "event": {
+            if (args.length < 2) {
+                System.out.println("Not enough arguments, dude."); // repeated code, can this be collapsed?


wrt comment:

if yes, it should be. not left as is.

it is better to use comments like this one on Github to record such comments rather than include it in the code.

Such comments can become stale very soon

okkhoy · 2022-09-18T14:12:36Z

src/main/java/Duke.java

+        case "todo": {
+            task = createTodo(taskData);
+            break;
+        }


there are no braces around case statements

If I were to declare a variable with the same name in two different cases, the braces are necessary to distinguish them as being as different scopes, or else Java complains about variable redefinition within the same scope. In this case I have fortuitously avoided that scenario, but this will probably come up as I add more code, and I'm not sure what the best way to handle this apart from putting braces around every case statement (using if statements instead? defining common variables in case statements just outside the switch statement?)

okkhoy · 2022-09-18T14:12:56Z

src/main/java/Duke.java

+        if (!taskData.contains("/at")) {
+            System.out.println("Missing /at parameter");
+            return null;
+        }


better handled as exceptions

okkhoy · 2022-09-18T14:16:05Z

src/main/java/Task.java

@@ -0,0 +1,27 @@
+public class Task {


All non-trivial classes and methods should have header comments

# Conflicts: # src/main/java/duke/Duke.java

Add Level 9

deveshl added 5 commits August 14, 2022 22:54

Update logo, add greet and exit message

73ae6e0

Add command processor, user input routine

ec00982

Add code to store and list tasks

29bbe64

Add Task class, code to mark and unmark tasks as done

a79dbc8

Add comment to comply with standard

c5cc698

jorellesee reviewed Aug 31, 2022

View reviewed changes

alfred-leong reviewed Aug 31, 2022

View reviewed changes

ongzhihong reviewed Aug 31, 2022

View reviewed changes

deveshl added 5 commits September 1, 2022 14:14

Duke: incorporate suggestions from peer review

513c910

Duke: break out runCommand into functions, add some error handling

25899fd

Add Deadline, Event and Todo classes; meet level 4 requirements

90277bc

Clean up addTask and split object creation to functions

2db8797

Add test cases for automated testing

2110043

okkhoy reviewed Sep 18, 2022

View reviewed changes

deveshl added 13 commits September 19, 2022 18:14

small code changes

a456ebd

Add InvalidArgumentException, change code to use it

6a19d6c

Merge branch 'branch-Level-5'

5ed17d4

Organize into packages

81439f1

Add delete task command and tests

e867ade

Add file storage capability

38c35da

Merge branch 'branch-Level-6'

d1bca03

Merge branch 'branch-Level-7'

ca70c15

# Conflicts: # src/main/java/duke/Duke.java

Make delete operation update disk

605ad1f

ignore all program output files

af34b82

bugfix: make reading from file work

ed997b2

bugfix: delete duke output before testing

3bf4e58

"More OOP", whatever that means

5d7a1f0

deveshl added 6 commits September 26, 2022 05:17

add MANIFEST.MF

e6f67c8

add find command

0014ba1

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

ec5b66b

Add Level 9

Add JavaDocs for public methods

728d21f

Merge branch 'branch-A-JavaDoc'

5e269fd

Add user guide

4a31242

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Devesh Logendran] iP #63

[Devesh Logendran] iP #63

deveshl commented Aug 29, 2022

jorellesee left a comment

jorellesee Aug 31, 2022

jorellesee Aug 31, 2022

jorellesee Aug 31, 2022

jorellesee Aug 31, 2022

alfred-leong Aug 31, 2022

alfred-leong left a comment

alfred-leong Aug 31, 2022

alfred-leong Aug 31, 2022

alfred-leong Aug 31, 2022

alfred-leong Aug 31, 2022

ongzhihong Aug 31, 2022

ongzhihong Aug 31, 2022

ongzhihong Aug 31, 2022

ongzhihong Aug 31, 2022

okkhoy left a comment

okkhoy Sep 18, 2022

okkhoy Sep 18, 2022

okkhoy Sep 18, 2022

okkhoy Sep 18, 2022

deveshl Sep 19, 2022

okkhoy Sep 18, 2022

okkhoy Sep 18, 2022


		static List<Task> tasks = new ArrayList<>();

		public static void runCommand(String command) {

[Devesh Logendran] iP #63

Are you sure you want to change the base?

[Devesh Logendran] iP #63

Conversation

deveshl commented Aug 29, 2022

jorellesee 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

alfred-leong 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okkhoy 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

Choose a reason for hiding this comment