[Goh Wei Kiat] iP #270

mrweikiat · 2021-02-06T04:53:07Z

Reopening my current PR based on feedbacks from Prof

danielonges

Overall, it was a pleasant experience reading your code! No major coding standard violations were found, just some minor tweaks that might need to be done here and there. Otherwise, LGTM! ☺️

danielonges · 2021-02-06T05:48:47Z

src/main/justin/Justin.java

+        ui.showHelpMessage();
+
+        // Condition for Duke to stop
+        boolean terminate = false;


Should this be canTerminate? Might be better to change to reflect boolean variable name

danielonges · 2021-02-06T05:51:18Z

src/main/justin/Storage.java

+                    }
+                } else if (task.getList().get(i) instanceof Deadline) { // is a deadline class
+                    if (task.getList().get(i).isDone) {
+                        holder = "D" + "|" + "1" + "|" + task.getList().get(i).description + "|" + ((Deadline) task.getList().get(i)).by;


Might have exceeded line limit

danielonges · 2021-02-06T05:54:16Z

src/main/justin/Ui.java

+        printSpace();
+        System.out.println("To add a todo: use command todo<space>taskName");
+        System.out.println("To add a deadline: use command deadline<space>taskName<space>/by<space>YYYY-MM-DD");
+        System.out.println("To add a event: use command event<space>taskName<space>/at<space>YYYY-MM-DD<space>HH:MM");


Might have exceeded line limit

danielonges · 2021-02-06T05:54:33Z

src/main/justin/Ui.java

+
+    static String logo =
+
+            "     ,--.                    ,--.   ,--.          \n" +


Might want to consider shifting "+" operator to begin on new line

danielonges · 2021-02-06T05:55:28Z

src/main/justin/Event.java

+
+    @Override
+    public String toString() {
+        return "[E]" + super.toString() + " (at: " + date1.format(DateTimeFormatter.ofPattern("MMM d yyyy")) + " " +


Might have exceeded line limit

Update JUnit testing

weixue123

Hi Wei Kiat, I feel that your code is quite well segmented. Great work!

weixue123 · 2021-02-06T12:25:27Z

src/main/java/justin/Justin.java

+
+                    case "DONE":
+
+                        String num = text.substring(5); // take out the int value of the task to be completed


Would it be clearer if a named constant or method was used to extract the task number instead? (instead of using a "magic" number)

weixue123 · 2021-02-06T12:28:36Z

src/main/java/justin/Justin.java

+        ui.showHelpMessage();
+
+        // Condition for Duke to stop
+        boolean terminate = false;


I feel that the code is clear enough for certain comments to be omitted!

weixue123 · 2021-02-06T12:30:59Z

src/main/java/justin/Storage.java

+                    }
+                } else if (task.getList().get(i) instanceof Deadline) { // is a deadline class
+                    if (task.getList().get(i).isDone) {
+                        holder = "D" + "|" + "1" + "|" + task.getList().get(i).description + "|" + ((Deadline) task.getList().get(i)).by;


Perhaps use a method to reduce the code duplication here?

weixue123 · 2021-02-06T12:42:09Z

src/main/java/justin/Parser.java

+        if (text.length() < 5 && text.contains("todo") ) { // case 1
+            throw new JustinException("☹ OOPS!!! The description of a todo cannot be empty.");
+        }
+        else if (text.contains("blah")) { // case 2


According to convention and for better readability, maybe consider indenting the trailing comments to the same tab setting?

weixue123 · 2021-02-06T12:43:36Z

src/main/java/justin/TaskList.java

+
+    }
+
+


I think there are times when it is not really necessary to have multiple blank lines in between parts of your code?

Added C-DetectDuplicates

PR from branch-A-CodeQuality to master

PR from branch-A-JavaDoc to master

PR branch-A-Assertions

PR branch-Level-10

PR branch-A-CodingStandard

PR branch-Level-9

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

Add Gradle support

3b19ba1

build.gradle: Update version to 8.29

a75fcee

Update Duke.java

7020044

level2

8d73734

level3

fc455aa

level 4

f286b75

level5

238b224

level6

98bc1c3

Update JAVADOCS comment

d1475b3

Update Duke.java

50e718d

A-TextUiTesting

3ba369a

Level 6

4c53e33

Update A-TextUiTesting

1e149a7

feature1

a1e2149

Branch testing

2348e76

Merge branch 'feature1'

2687090

Level 7

9e92011

Level8

67efef3

A-MoreOOP

1eb74dd

A-Packages

5a823a8

Level 9

4f103fe

danielonges reviewed Feb 6, 2021

View reviewed changes

Package Justin

ec6e328

Update JUnit testing

weixue123 reviewed Feb 6, 2021

View reviewed changes

mrweikiat added 6 commits February 8, 2021 00:24

A-JavaDoc

3141a6a

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

46a8cc9

A-Gradle

6c45111

Level 10 GUI

bc42771

Added help taskbar

6f08404

Added C-DetectDuplicates

Add C-Extensions

8207336

mrweikiat and others added 30 commits February 18, 2021 19:45

Update README.md

5d86cbf

Update README.md

ae164d8

Improve on Ui

6f07dc9

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

516ac7c

Update README.md

6d88cb0

Update README.md

ac112f7

Update README.md

ef5247a

Update README.md

c735e63

Update Ui.png

df6b25e

branch-A-CodeQuality

0fc6025

branch-A-CodingStandard

06271ce

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

3e9e0bd

PR from branch-A-CodeQuality to master

branch-A-JavaDoc

14fe879

Merge pull request #3 from mrweikiat/branch-A-JavaDoc

5aabbc7

PR from branch-A-JavaDoc to master

branch-A-Assertions

3748d06

Merge pull request #4 from mrweikiat/branch-A-Assertions

2ea782f

PR branch-A-Assertions

branch-Level-10

d3135ad

Merge pull request #8 from mrweikiat/branch-Level-10

3ae9e3e

PR branch-Level-10

Merge branch 'master' into branch-A-CodingStandard

e03420c

Merge pull request #9 from mrweikiat/branch-A-CodingStandard

c0ff7f4

PR branch-A-CodingStandard

branch-Level-9

9f48c36

branch-Level-9

ef654c5

Merge pull request #11 from mrweikiat/branch-Level-9

49007e1

PR branch-Level-9

Update Code Quality

526d705

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

9659072

Add jar file

7e573ec

Update README.md

7ba0d2d

Update JavaDoc and Code Standard

6cc0997

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

081d1f8

Update README.md

4247723

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Goh Wei Kiat] iP #270

[Goh Wei Kiat] iP #270

mrweikiat commented Feb 6, 2021

danielonges left a comment

danielonges Feb 6, 2021

danielonges Feb 6, 2021

danielonges Feb 6, 2021

danielonges Feb 6, 2021

danielonges Feb 6, 2021

weixue123 left a comment

weixue123 Feb 6, 2021

weixue123 Feb 6, 2021

weixue123 Feb 6, 2021

weixue123 Feb 6, 2021

weixue123 Feb 6, 2021


		case "DONE":

		String num = text.substring(5); // take out the int value of the task to be completed

[Goh Wei Kiat] iP #270

Are you sure you want to change the base?

[Goh Wei Kiat] iP #270

Conversation

mrweikiat commented Feb 6, 2021

danielonges 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

weixue123 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