[Yan Lyn] iP #469

yanlynnnnn · 2020-08-31T06:53:06Z

No description provided.

This reverts commit 652a853.

This reverts commit 2c4ba83.

This reverts commit 6f3abf9.

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

* branch-Level-8: Level 8

* branch-Level-7: no message Level 8 Level 7 # Conflicts: # listStore.ser # src/main/java/Duke.java

This reverts commit 49a8bb8.

* branch-A-CodingStandard: A-CodingStandard # Conflicts: # src/main/java/Duke.java

* branch-Level-9: Level-9 # Conflicts: # src/main/java/Duke.java

* commit 'a75fceeb74708da6ac524c08ef526d38e60ee56b': build.gradle: Update version to 8.29 Add Gradle support

hogantan

Overall pretty good!
However one important thing to note is that, I realized that all classes were nested inside the Duke class which could get clunky and messy as the project gets bigger. You should try to distribute and package each class separately to make it neater. Also, I realized that most if not all the classes are static as well which should be removed they are redundant to the class.
For the JavaDocs, just do not forget about the fullstops!

hogantan · 2020-08-31T13:23:10Z

src/main/java/Duke.java

 public class Duke {
-    
+
    private static Storage storage;
    private TaskList inputs;


Could the name "inputs" be a little more precise when it comes to referring a TaskList?

hogantan · 2020-08-31T13:25:29Z

src/main/java/Duke.java

@@ -7,12 +7,18 @@
 import java.time.format.DateTimeFormatter;
 import java.time.temporal.ChronoUnit;

+


Do you think adding a JavaDoc description here would allow other users to better understand what exactly Duke does?

hogantan · 2020-08-31T13:26:39Z

src/main/java/Duke.java

-
+
+    /*
+     * Constructs Duke Object, constructs UI, storage and TaskList object to initialize


Missing full stop for JavaDoc here!

hogantan · 2020-08-31T13:27:27Z

src/main/java/Duke.java

+    }
+
+    // Input class represents user inputted tasks
+    public static class Input implements Serializable {


Interesting use of static classes and interfaces here!

hogantan · 2020-08-31T13:28:56Z

src/main/java/Duke.java

+
+    // Input class represents user inputted tasks
+    public static class Input implements Serializable {
+        boolean done;


You could try naming this boolean more like a boolean, something like isDone.

hogantan · 2020-08-31T13:35:06Z

src/main/java/Duke.java

            }
+            Todo todo = new Todo(nextLine.substring(5));


I realised the use of substring to get specific keywords from the user input. An alternative to this would be to use the split() function of String to split the user input up!

hogantan · 2020-08-31T13:37:44Z

src/main/java/Duke.java

+            int len = inputs.size();
+            for(int i = 0; i < len; i++) {
+                Input input = inputs.get(i);
+                if (input.content.contains(keyword)) {


Maybe you can consider making this more succint. In other words, try to avoid nesting conditionals which could get quite messy, though I am guilty of this too!

Would the code more readable if you use extra method to handle the conditions? 🤔

hogantan · 2020-08-31T13:39:22Z

src/main/java/Duke.java

+
+    //Parser class deals with making sense of user command
+    public static class Parser {
+        String nextLine;


Are these missing their access modifiers? It would be best to make them private!

hogantan · 2020-08-31T13:40:44Z

src/main/java/Duke.java

+                    inputs.taskEvent(nextLine);
+                } else if (nextLine.startsWith("find")) {
+                    inputs.taskFind(nextLine);
+                } else if (nextLine.equals("list")) {


You could try making these code more succint by nesting lesser conditionals together!

hogantan · 2020-08-31T13:43:45Z

src/main/java/Duke.java

+            System.out.println("Noted. I've removed this task:");
+            Input inputType = inputs.get(numTaskDone - 1);
+            if (inputType.done) {
+                System.out.println("  " + inputType.id + "[/] " + inputType.content + inputType.printTime);


You could try using the tick symbol instead of a '/'. The four-digit hex code for the tick symbol is \u2713.

tanwayne890

Just some nits to fix for more readability.

tanwayne890 · 2020-09-01T04:59:51Z

src/main/java/Duke.java

+         * @param nextLine, represents user input
+         * @throws DukeException if user does not specify which task done, or if task specified is not in list
+         */
+        void taskDone(String nextLine) throws DukeException {


I notice you are trying to mark a task as done. Would it be better if you rename your method to verb? For example, markTaskDone.

tanwayne890 · 2020-09-01T05:06:07Z

src/main/java/Duke.java

+        }
+    }
+
+    public static void main (String[]args) throws DukeException {


Is there any spacing errors?

tanwayne890 · 2020-09-01T05:13:10Z

src/main/java/Duke.java

+        String content;
+        String id;
+        LocalDate time;
+        String printTime;


Is it better to understand a variable or method if you use noun for variable/class and verb for methods/functions?
🤔

tanwayne890 · 2020-09-01T05:20:38Z

src/main/java/Duke.java

+            this.id = "[E]";
+            this.printTime = "(" + time.format(DateTimeFormatter.ofPattern("MMM d yyyy")) + ")";
+        }
+    }


I am glad that all of your code is maintained below the line length limit which is below 120 char, so the code view is better. 👍

tanwayne890 · 2020-09-01T05:22:35Z

src/main/java/Duke.java

+            inputs.add(deadline);
+            int count = inputs.size();
+            System.out.println("Got it. I've added this task: \n" + "  [D][x] " + deadline.content +
+                    deadline.printTime + "\n Now you have " + count + " tasks in the list");


Would the readability improve if you break the line before operator? 🤔

tanwayne890 · 2020-09-01T05:26:11Z

src/main/java/Duke.java

+            int len = inputs.size();
+            for(int i = 0; i < len; i++) {
+                Input input = inputs.get(i);
+                if (input.content.contains(keyword)) {


Would the code more readable if you use extra method to handle the conditions? 🤔

This reverts commit 311532c.

* branch-A-CodeQuality: A-CodeQuality

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

Add Gradle support

3b19ba1

no message

652a853

Revert "no message"

2c4ba83

This reverts commit 652a853.

Revert "Revert "no message""

6f3abf9

This reverts commit 2c4ba83.

Revert "Revert "Revert "no message"""

58cbb14

This reverts commit 6f3abf9.

Level 1

993e55a

Level 2

c73a6d6

Level 3

51a3fbe

no message

03191ac

Merge branch 'master' into HEAD

382e15f

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

no message

b81969c

no message

98ae8d3

Level 7

c7c32f0

Level 8

853e69f

no message

3f6e141

A-OOP

b6b321f

A-JUnit

2ec6df1

Merge branch 'branch-Level-8' into branch-Level-7

6f9552b

* branch-Level-8: Level 8

Merge branch 'branch-Level-7'

1434336

* branch-Level-7: no message Level 8 Level 7 # Conflicts: # listStore.ser # src/main/java/Duke.java

A-JavaDoc

49a8bb8

Revert "A-JavaDoc"

0c03fe6

This reverts commit 49a8bb8.

A-JavaDoc

3a58acb

A-CodingStandard

e58cf6f

Level-9

507ebfa

Merge branch 'branch-A-CodingStandard'

a7eb991

* branch-A-CodingStandard: A-CodingStandard # Conflicts: # src/main/java/Duke.java

Merge branch 'branch-Level-9'

cc5f84a

* branch-Level-9: Level-9 # Conflicts: # src/main/java/Duke.java

Level-9

6b8d8bb

Week3

44216fa

build.gradle: Update version to 8.29

a75fcee

Merge commit 'a75fceeb74708da6ac524c08ef526d38e60ee56b'

f31911a

* commit 'a75fceeb74708da6ac524c08ef526d38e60ee56b': build.gradle: Update version to 8.29 Add Gradle support

hogantan reviewed Aug 31, 2020

View reviewed changes

tanwayne890 reviewed Sep 1, 2020

View reviewed changes

yanlynnnnn added 17 commits September 3, 2020 15:59

Level-10

9205c0b

GUI for Duke

4fc5c2d

Edits based on pull request review

311532c

Revert "Edits based on pull request review"

829c423

This reverts commit 311532c.

A-CodeQuality

1046ae3

Merge branch 'branch-A-CodeQuality'

398c649

* branch-A-CodeQuality: A-CodeQuality

Fixed bug in 'findTask' method

5ff3a7e

Improved Documentation

c06c2fa

BCD-extension

918b69b

A-UserGuide

ee08b5a

Improve GUI and UG

26cedef

Add Highlights on UG

7bb129f

Improve UG and GUI

625bac9

improve ug

fe835e9

finalize ug

40967de

no message

dd4fe02

Empty Commit

0c02323

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yan Lyn] iP #469

[Yan Lyn] iP #469

yanlynnnnn commented Aug 31, 2020

hogantan left a comment

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

tanwayne890 Sep 1, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

hogantan Aug 31, 2020

tanwayne890 left a comment

tanwayne890 Sep 1, 2020

tanwayne890 Sep 1, 2020

tanwayne890 Sep 1, 2020

tanwayne890 Sep 1, 2020

tanwayne890 Sep 1, 2020

tanwayne890 Sep 1, 2020

		@@ -7,12 +7,18 @@
		import java.time.format.DateTimeFormatter;
		import java.time.temporal.ChronoUnit;



		/*
		* Constructs Duke Object, constructs UI, storage and TaskList object to initialize

[Yan Lyn] iP #469

Are you sure you want to change the base?

[Yan Lyn] iP #469

Conversation

yanlynnnnn commented Aug 31, 2020

hogantan 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

tanwayne890 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