[Nguyen Hoang Hai Minh] iP by nevirmc · Pull Request #445 · nus-cs2103-AY2021S1/ip

nevirmc · 2020-08-27T09:14:13Z

No description provided.

* branch-Level-8: level 8 added # Conflicts: # src/main/java/Duke.java

A-MoreOOP added

* branch-Level-9: Level-9 Added

jonasngs

Overall, I like how your code is concise, readable and very easy to understand for the most parts. Most method and class names are clear and correctly adhered to coding standards. However, it is worth noting that there are a few minor coding standard violations and missing JavaDoc header comments for several classes and methods.
It looks good to merge. Keep up the good work 👍

jonasngs · 2020-08-31T08:01:55Z

+    String description;
+    String deadline;
+    boolean isDone;


Should these class variables be declared with a non-public access modifier to adhere to encapsulation principles? Perhaps you could consider using protected or private?

jonasngs · 2020-08-31T08:13:42Z

+    /**
+     * Status in format [D][x] return book (by: 6 June)
+     */


Perhaps you could consider following the guidelines for Java coding standards when writing JavaDoc comments? I think you are missing a 'return parameter' for the JavaDoc comment, or perhaps is there any reason why you wrote the JavaDoc comments in the way you did? I observed the same issue in several other methods as well.

jonasngs · 2020-08-31T08:22:03Z

@@ -1,10 +1,152 @@
+//package duke;
+import java.io.IOException;
+import java.util.*;


Perhaps you could list out all the imported classes explicitly? I noticed the same issue in several other classes as well.

jonasngs · 2020-08-31T08:23:32Z

+    private Storage storage;
+    private TaskList tasks;
+    private Ui ui;
+    private Parser parser;


Perhaps you could consider adding JavaDoc comments for class members as well?

jonasngs · 2020-08-31T08:31:07Z

+    public void run() {
+        ui.sayHi();
+        Scanner myScanner = new Scanner(System.in);
+        while(true) {


Perhaps you could add a whitespace after 'while'? The guidelines on Java coding standards states that Java reserved words should be followed by a white space. I noticed this issue in other methods containing while, if and if-else blocks as well.

jonasngs · 2020-08-31T10:09:01Z

+                myReader.close();
+            }
+        } catch (FileNotFoundException e) {
+        }


Perhaps you can consider handling the caught exception in this catch block? Is there any specific reason why you chose not to handle the caught exception? I observed a similar issue in other methods as well, such as on line 65-66 in this same class.

jonasngs · 2020-08-31T10:28:12Z

+        return tasks.size();
+    }
+
+    public Task get(int i) {


Perhaps this method name is very ambiguous and does not give readers a clear idea on what variable is being retrieved? Perhaps you can use 'getTask' as the method name? I believe this provides more clarity as well. I noticed similar issues in other classes as well.

jonasngs · 2020-08-31T10:33:46Z

+        tasks.add(tmp);
+    }
+
+    public TaskList(ArrayList<Task> tasks) {


Perhaps you could shift this constructor to line 11 below the other constructor? Placing a constructor at the bottom of the class might not be ideal for readability.

jonasngs · 2020-08-31T10:36:24Z

+
+public class TaskListTest {
+    @Test
+


Perhaps line 9 could be removed so that the '@test' annotation is directly above the junit test method? This might not be a serious issue though. I observed a similar issue for the TodoTest class below.

jonasngs · 2020-08-31T11:16:19Z

+/**
+ * Deadline <- Task
+ */
+public class Deadline extends Task {


I like how all the class names in your code and clear, concise and adhered to the Java coding standards and naming conventions.

Syasyazman

Overall, I feel like there are generally no major improvements needed to your coding quality as it is very comprehensive and readable. I also appreciate that you generally did not recycle variables or parameters and avoided empty catch blocks. However, I do feel like certain dead codes could have been omitted although it does not affect the readability of your code.

It has been a pleasant read and I love the simplicity in your coding style! I think it is good to go for merging.

Syasyazman · 2020-08-31T15:26:25Z

+/**
+ * Deadline <- Task
+ */


Is there a better way to describe the Deadline class in the Javadoc comment? I understand what you are trying to convey but perhaps it would be better to elaborate on it! I personally prefer this representation as its very concise however it might not be intuitive enough for other readers.

Syasyazman · 2020-08-31T15:31:58Z

+        ui = new Ui();
+        storage = new Storage();
+        tasks = new TaskList(storage.readData());
+        parser = new Parser();


I like this way of initialising the variables as its very concise. However, perhaps you could consider using "this" to refer to the instance variables? I feel like it would be a more explicit representation of the initialisation in the constructor.

Syasyazman · 2020-08-31T15:36:06Z

+                    System.out.println(i + "." + tasks.get(i - 1).getStatus());
+                }
+            }
+            else if(cmd.length() >= 4 && cmd.substring(0, 4).equals("done")) {


Perhaps you could use comments to further elaborate on your "else if" statements' condition? I feel like it would definitely be clearer and will increase readability for readers!

Syasyazman · 2020-08-31T15:38:32Z

+                    checkCmd(cmd, 8, "☹ OOPS!!! The description of a deadline cannot be empty.");
+                    String getName = parser.getNameBy(cmd);
+                    String getDeadline = parser.getDeadlineBy(cmd);
+                    getDeadline = formatDate(getDeadline);
+                    Deadline tmp = new Deadline(getName, getDeadline);
+                    System.out.println("Got it. I've added this task: ");
+                    System.out.println("  " + tmp.getStatus());
+                    tasks.add(tmp);
+                    System.out.println("Now you have " + tasks.getSize() + " tasks in the list.");


I appreciate how clean your code is in the try blocks! But maybe you could include more spacings and comments between different segments of the code so that it is easier to capture the main gist of your code?

Syasyazman · 2020-08-31T15:42:29Z

+/**
+ * translate command -> name and time
+ */
+public class Parser {


I really like how your Parser class separates different functions in a very intuitive manner to readers. However, perhaps you could provide a more detailed naming to the functions? This is so that the functionality of your functions can be more easily inferred by its name.

Syasyazman · 2020-08-31T15:48:15Z

+        Todo todo = new Todo("test");
+        assert(todo.getType() == "T");


I really like how you did testing of the lowest complexities as it shows that you place focus on the minute details of your code!

…000/ip into A-Gradle * 'add-gradle-support' of https://github.com/minhhhnguyen2000/ip: Add Gradle support # Conflicts: # gradle/wrapper/gradle-wrapper.jar # gradle/wrapper/gradle-wrapper.properties # gradlew # gradlew.bat

* master: Add Stream Collectors

Add Assert

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

Add Gradle support

3b19ba1

Implement Level-1

cb0dcf9

Level-2

ff00562

Level-3

299005e

Level-3

052071c

Level-4

d53eb38

TextUiTesting

c9c67ac

Level-5

7345f13

Level-6

61ace1f

level 7 added

828bf0b

no message

87a8790

level 8 added

eff84ea

Merge branch 'branch-Level-8'

716d3c3

* branch-Level-8: level 8 added # Conflicts: # src/main/java/Duke.java

level-8 added

a9a8581

Level-8 fixed

9e2f0f5

A-MoreOOP added

A-Packages added

12fe9af

A-JUnit added

758241f

no message

367da21

A-JavaDoc added

d47603e

Level-9 Added

3f6b540

Merge branch 'branch-Level-9'

53a20a7

* branch-Level-9: Level-9 Added

jonasngs reviewed Aug 31, 2020

View reviewed changes

Syasyazman reviewed Aug 31, 2020

View reviewed changes

nevirmc added 7 commits September 3, 2020 05:22

Fix package

e5257ac

no message

f71b3c9

add GUI

af6a803

no message

ca6ec8c

no message

7428cf4

no message

8e6c851

nevirmc and others added 30 commits September 10, 2020 12:20

Merge branch 'master' into branch-A-Assertions

2b4f16d

* master: Add Stream Collectors

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

29bd1ae

Add Assert

no message

e393d3d

Create gradle.yml

692c58e

Add dowithin

d4b0d9d

no message

6e5ad76

no message

e8b9524

no message

00f4ff0

Change to better OOP / Code Quality / JavaDoc

426b401

no message

f9481a7

Divide into multiple packages

0cef394

Change Task.java to abstract class

23525e3

better UI

407b183

no message

462aaa7

fix CheckStyle to detect coding style violations

28b3deb

seperate DateFormatter

09a96b7

seperate DateFormatter

1f73d1d

no message

2e39786

added product picture

befc6ad

Fix Ui.png

c2d3a3d

Better OOP and Packages

c24f512

update README.md

b28e166

Set theme jekyll-theme-cayman

779eb86

updates README.md

e88c219

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

601725b

no message

012b4f6

no message

ce880a7

no message

1ed4376

Fix some minor JavaDoc error and updated build.gradle

08f971a

Update UI because of bug with jar

18e4cee

Conversation

nevirmc commented Aug 27, 2020

Uh oh!

jonasngs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasngs Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasngs Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasngs Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasngs Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Syasyazman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonasngs left a comment •

edited

Loading

jonasngs Aug 31, 2020 •

edited

Loading

jonasngs Aug 31, 2020 •

edited

Loading

jonasngs Aug 31, 2020 •

edited

Loading

jonasngs Aug 31, 2020 •

edited

Loading