[Tan Jia Qi] iP #249

jiaqi20 · 2020-08-23T14:01:33Z

No description provided.

# Conflicts: # src/main/java/Deadline.java # src/main/java/Duke.java # src/main/java/Event.java

shermz-lim

I think your code is very well-organized and clean in general! Nice one 👍 Just left my comments on some small coding standard and code quality issues :)

shermz-lim · 2020-09-01T12:28:17Z

src/main/java/DukeException.java

+        if (this.type.equals("other")) {
+            return "\uD83D\uDE41" + " OOPS!!! I'm sorry, but I don't know what that means :-(";
+        } else if (this.type.equals("done")) {
+            return "\uD83D\uDE41" + " OOPS!!! Please indicate the numbering of task that has to be marked as done.";
+        } else if (this.type.equals("delete")) {
+            return "\uD83D\uDE41" + " OOPS!!! Please indicate the numbering of task that has to be deleted.";
+        } else if (this.type.equals("date")) {
+            return "\uD83D\uDE41" + "Please format the time in YYYY-MM-DD.";
+        }
+        return "\uD83D\uDE41" + " OOPS!!! The description of a " + this.type + " cannot be empty.";


I like how you have defined the error messages in a single place: it makes it much easier to change!

shermz-lim · 2020-09-01T12:31:40Z

src/main/java/Parser.java

+     * @param duke Duke object.
+     * @return Duke response message.
+     */
+    public String parse(String userInput, Duke duke) {


I think you can consider shortening this method (as it seems quite long) by creating other private methods as helper functions! 👍

I agree that it's quite long hahaha I'll work on it! thanks :))

shermz-lim · 2020-09-01T12:33:11Z

src/main/java/Parser.java

+                return duke.getUi().printDeleteTask(t, duke.getTasks());
+            } else {
+                if (strArr[0].equals("todo")) {
+                    String sd = "";


Perhaps you can use a more descriptive variable name :) I notice this in other places too in this method

shermz-lim · 2020-09-01T12:34:31Z

src/main/java/Storage.java

@@ -0,0 +1,65 @@
+import java.io.*;


You can consider listing explicitly the classes that you are importing!

shermz-lim · 2020-09-01T12:37:50Z

src/main/java/Ui.java

+     * Prints bye message to user.
+     * @return Duke response message for bye.
+     */
+    public String bye() {


Maybe you can name this as "printBye"? I think it will be more consistent with the rest of the method names, and it also sounds slightly more like an action :)

Ohh seems like I missed out this. Alright, I'll edit it! thanks!

shermz-lim · 2020-09-01T12:48:19Z

src/main/java/Duke.java

+import javafx.scene.shape.Rectangle;
+import javafx.stage.Stage;
+
+import java.io.*;


I think you can consider explicitly listing all the classes you are importing! :)

galvinleow · 2020-09-02T01:54:09Z

src/main/java/Storage.java

+import java.io.*;
+import java.time.LocalDate;
+import java.util.ArrayList;


I agree with shermz-lim, try to configure your IntelliJ so that it will remove this problem going forward

galvinleow · 2020-09-02T02:45:46Z

src/test/java/TaskTest.java

+    @Test
+    public void testDoneState(){
+        assertEquals(0, new Task("read book").doneState());
+    }


Maybe can include the expected result in the method name

Ohh okayy I'll take note of that! thanks :))

galvinleow · 2020-09-02T03:45:40Z

src/main/java/Deadline.java

+        this.isDone = done.equals("1");
+    }
+
+    public String toString() {


Missing @OverRide, your other task subclass have.

Oopss okayy I'll change this.

galvinleow

I think if you use checkstyle will have some error at the import statement cause they will have a fixed order.

LinkedInk

In general, LGTM. The parse(String userInput, Duke duke) under the Parser class goes against having long methods and deep nesting. Some readiblity issues here and there with variables (like others mentioned) and complicated expessions that can be easily fixed.

LinkedInk · 2020-09-05T08:31:33Z

src/main/java/Task.java

+/**
+ * Represents a task entered by user.
+ */
+public class Task {


Maybe making Task class abstract is better so that there is no need for default methods and not every one of the subclasses need to use @Override for saveString() or any other possible future overlapping methods that are implemented differently across the different subclasses.

LinkedInk · 2020-09-05T08:48:35Z

src/main/java/Parser.java

+        String[] strArr = userInput.split(" ");
+        try {
+            check(strArr);
+            if (userInput.equals("bye")) {
+                return duke.getUi().bye();
+                //System.exit(0);
+            } else if (userInput.equals("list")) {
+                return duke.getUi().printTaskList(duke.getTasks());
+            } else if (strArr[0].equals("done")) {
+                duke.getTasks().getTask(Integer.parseInt(strArr[1]) - 1).markAsDone();
+                duke.getStorage().writeFile(duke.getTasks().getTaskList());
+                return duke.getUi().printDoneTask(duke.getTasks().getTask(Integer.parseInt(strArr[1]) - 1));
+            } else if (strArr[0].equals("delete")) {
+                int i = Integer.parseInt(strArr[1]) - 1;
+                Task t = duke.getTasks().getTask(Integer.parseInt(strArr[1]) - 1);
+                duke.getTasks().deleteTask(i);
+                duke.getStorage().writeFile(duke.getTasks().getTaskList());
+                return duke.getUi().printDeleteTask(t, duke.getTasks());
+            } else {


Feels really messy. Since strArr already has the command string in the 0 position. Can consider using a switch statement to check for command for readibility purposes. Instead of a mix of userInput.equals and strArr[0].equals. The logic after each else if/else block (or switch statement if u choose to use it) can be broken out into private methods like others mentioned for readibility.

LinkedInk · 2020-09-05T09:00:54Z

src/main/java/Parser.java

+    public void check(String[] arr) throws DukeException {
+        if (arr.length == 1 && (arr[0].equals("todo") || arr[0].equals("deadline") || arr[0].equals("event") ||
+                arr[0].equals("done") || arr[0].equals("delete") || arr[0].equals("find"))) {
+            throw new DukeException(arr[0]);
+        } else if (!arr[0].equals("todo") && !arr[0].equals("deadline") && !arr[0].equals("event") &&
+                !arr[0].equals("list") && !arr[0].equals("bye") && !arr[0].equals("done") 
+                && !arr[0].equals("delete") && !arr[0].equals("find")) {
+            throw new DukeException("other");


Readiblity is quite poor here :(

Switching the if else checks around would allow you to skip && (arr[0].equals("todo") || arr[0].equals("deadline") || arr[0].equals("event") || arr[0].equals("done") || arr[0].equals("delete") || arr[0].equals("find")) just checking for length instead.

So:
if statement 1) command I don't understand? (ignoring description)
if statement 2) command without description? (all commands here would be understood)

Instead of:
if statement 1) command without description? && command I understand?
if statement 2) command with description but I don't understand?

Comments would be nice to describe what is being checked at each if statement.

Do also consider using the logic under public String parse(String userInput, Duke duke) to avoid doing checks on the command string twice to keep maintaining simple (ie don't have to update 2 places when commands change).

LinkedInk · 2020-09-05T09:25:56Z

src/main/java/Task.java

+    public String getStatusIcon() {
+        return (isDone ? "\u2713" : "\u2718");
+    }


I assume these two values are the 'tick' and 'cross'. Consider using a named constant as part of keeping code quality, avoiding magic numbers.

Use Assertions

Improve Code Quality

damithc and others added 25 commits July 23, 2020 23:27

Add Gradle support

3b19ba1

Greet, Echo, Exit

7b32d61

Level-1: Greet, Echo, Exit

e8b8ce8

Merge branch 'master' of https://github.com/jiaqi20/ip

b402d42

Level-2: Add, List

b0c275c

Level-3: Mark As Done

401a9cd

Level-4: ToDos, Events, Deadlines

ce27701

Automated Text UI Testing

e1567f3

Level-5: Handle Errors

505270f

Level-6: Delete

8e5887b

Adds indentation and spacing

9bc0943

Level-7: Save

bc80550

Level-8: Dates and Times

f987b1a

Merge branch 'branch-Level-8'

31406c4

# Conflicts: # src/main/java/Deadline.java # src/main/java/Duke.java # src/main/java/Event.java

Use More OOP

d3877b0

Add JUnit Tests

e95d1eb

Add JavaDoc comments

210f53e

Tweak the code to comply with a coding standard

1049fd6

Add find feature

0242c07

Merge branch 'branch-A-CodingStandard'

6e31a0f

Merge branch 'branch-Level-9'

3625ce5

Merge remote-tracking branch 'origin/add-gradle-support'

f890634

Use Gradle

521d9ac

Add GUI

cd89bd4

Add some comments

dcf5656

shermz-lim reviewed Sep 1, 2020

View reviewed changes

galvinleow reviewed Sep 2, 2020

View reviewed changes

galvinleow reviewed Sep 3, 2020

View reviewed changes

LinkedInk reviewed Sep 5, 2020

View reviewed changes

jiaqi20 and others added 15 commits September 7, 2020 14:14

Use Assertions

beaf37c

Merge pull request #1 from jiaqi20/branch-A-Assertions

564c34d

Use Assertions

Improve Code Quality

3a35713

Merge pull request #2 from jiaqi20/branch-A-CodeQuality

3691ce2

Improve Code Quality

Add extension

8501f68

Improve functionality (Detect Duplicates)

c6444db

Update README

f436f16

Update README.md

7c1458e

Set theme jekyll-theme-hacker

6dea25c

Do minor changes

8b58da0

Merge branch 'master' of https://github.com/jiaqi20/ip

4ef0c36

Set theme jekyll-theme-time-machine

9c6fda0

Modify README

5e45b65

Merge branch 'master' of https://github.com/jiaqi20/ip

b5919cb

Set theme jekyll-theme-hacker

b43629c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tan Jia Qi] iP #249

[Tan Jia Qi] iP #249

jiaqi20 commented Aug 23, 2020

shermz-lim left a comment

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

shermz-lim Sep 1, 2020

jiaqi20 Sep 3, 2020

galvinleow Sep 2, 2020

jiaqi20 Sep 3, 2020

galvinleow Sep 2, 2020 •

edited

jiaqi20 Sep 3, 2020

galvinleow Sep 2, 2020

jiaqi20 Sep 3, 2020

galvinleow left a comment

LinkedInk left a comment

LinkedInk Sep 5, 2020

LinkedInk Sep 5, 2020 •

edited

LinkedInk Sep 5, 2020

LinkedInk Sep 5, 2020 •

edited

[Tan Jia Qi] iP #249

Are you sure you want to change the base?

[Tan Jia Qi] iP #249

Conversation

jiaqi20 commented Aug 23, 2020

shermz-lim 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

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

galvinleow Sep 2, 2020 • edited

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

galvinleow left a comment

Choose a reason for hiding this comment

LinkedInk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinkedInk Sep 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LinkedInk Sep 5, 2020 • edited

Choose a reason for hiding this comment

galvinleow Sep 2, 2020 •

edited

LinkedInk Sep 5, 2020 •

edited

LinkedInk Sep 5, 2020 •

edited