[Cao Wenjie] iP #351

shadowezz · 2020-08-25T05:24:35Z

No description provided.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

sigmund-c

Overall, a very neatly written project! Only found minor violations to the coding standard.

sigmund-c · 2020-08-31T07:49:03Z

src/main/java/duke/Duke.java

+     * Creates an instance of Duke.
+     * It initialises the UI, creates a storage based on the filepath
+     * and initialises the existing tasks.
+     * @param filePath The location where the task data is stored


Minor nitpick (that I'm personally ok with), but the coding standard suggests adding an empty line between the description and the parameter section

sigmund-c · 2020-08-31T07:53:59Z

src/main/java/duke/Parser.java

+ * It stores the user's command in the form of an array to separate
+ * the different components of the command.
+ */
+public class Parser {


Not 100% sure about this one myself, but would a name like ParsedCommand be more accurate here since it represents one command? Though I understand that a previous level requires a class to be explicitly named Parser :P

I think Parser is a suitable name for this class because its function is to decode user input and convert it into Commands!

sigmund-c · 2020-08-31T08:00:33Z

src/main/java/duke/Ui.java

+    private final static String line = "______________________________________________________";
+    private final static String lineIndent = "    ";
+    private final static String listIndent = "       ";
+    private final static String textIndent = "     ";


Since these are constants, it's recommended to have them all be uppercase (LINE, LINE_INDENT, ...). And perhaps that since they're associated with one another, they could also be prefixed with a common name (MSG_LINE, MSG_LINE_INDENT, ...)

I think naming constants as @sigmund-c suggested is compliant with coding standards!

Yup I think both of you are right! Thanks for the suggestions!

sc-arecrow

Overall, your code is very clean and readable! I was trying my best to look for improvements in naming and coding standard violations but I only found a handful :)

sc-arecrow · 2020-08-31T14:51:05Z

src/main/java/duke/Parser.java

+ * It stores the user's command in the form of an array to separate
+ * the different components of the command.
+ */
+public class Parser {


I think Parser is a suitable name for this class because its function is to decode user input and convert it into Commands!

sc-arecrow · 2020-08-31T14:55:10Z

src/main/java/duke/Storage.java

+     * @param task the task that needs to be converted to string format
+     * @return the string format of input task
+     */
+    private String formatTask(Task task) {


I like your storage formatting! 👍

sc-arecrow · 2020-08-31T14:56:32Z

src/main/java/duke/Storage.java

+        if (taskComponent[0].equals("T")) {
+            taskList.add(new Todo(name, isComplete));
+        } else if (taskComponent[0].equals("D")) {
+            String by = taskComponent[3];


Would it be clearer if this variable was named something like dateTimeString? Same for Line 126 :)

sc-arecrow · 2020-08-31T14:58:10Z

src/main/java/duke/Ui.java

+    private final static String line = "______________________________________________________";
+    private final static String lineIndent = "    ";
+    private final static String listIndent = "       ";
+    private final static String textIndent = "     ";


I think naming constants as @sigmund-c suggested is compliant with coding standards!

sc-arecrow · 2020-08-31T15:00:18Z

src/main/java/duke/format/DateFormat.java

+import java.util.List;
+
+public class DateFormat {
+    public static List<String> dateFormats = Arrays.asList("d/M/yy HHmm", "d-M-yy HHmm",


I like that you check for many possible date formats! 👍

sc-arecrow · 2020-08-31T15:01:05Z

src/main/java/duke/task/Deadline.java

+ * Represents a deadline type of task that contains an additional due date.
+ */
+public class Deadline extends Task {
+    protected Date by;


Perhaps a better name for this variable could be dueDate?

sc-arecrow · 2020-08-31T15:01:33Z

src/main/java/duke/task/Event.java

+ * Represents an event type of task with the date of event.
+ */
+public class Event extends Task {
+    protected Date at;


Perhaps a better name for this variable could be eventDate?

sc-arecrow · 2020-08-31T15:02:15Z

src/main/java/duke/task/TaskType.java

+    TODO,
+    DEADLINE,
+    EVENT;


I think according to the coding standard, enum names should be written in PascalCase!

I am not really sure on this myself but I thought enum name refers to TaskType instead. I will double check!

yuki-cell

Overall, code was very organized and easy to follow.
Some cosmetic changes and suggestion for task's constructor requested.

yuki-cell · 2020-09-01T20:29:06Z

src/main/java/duke/Duke.java

+import duke.exception.MissingInformationException;
+
+import java.io.IOException;
+


maybe can delete this extra space

yuki-cell · 2020-09-01T20:32:19Z

src/main/java/duke/Storage.java

+     * @throws DateException If the date of tasks in the file are of the wrong format.
+     */
+    public List<Task> load() throws IOException, DateException {
+


maybe can delete this extra space?

yuki-cell · 2020-09-01T20:36:19Z

src/main/java/duke/task/Deadline.java

+     * @param isComplete the completion status of deadline
+     * @param by the due date of deadline
+     */
+    public Deadline(String name, boolean isComplete, Date by) {


maybe set isComplete false by default so no need to assign isComplete every time making deadline (same applies for other task related class)
ex) new Deadline("task", false, 2020-9-1) => new Deadline("task", 2020-9-1)

yuki-cell · 2020-09-01T20:42:03Z

src/test/java/duke/task/TodoTest.java

+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+


this is minor detail but can remove this extra space

yuki-cell · 2020-09-01T20:44:09Z

src/test/java/duke/ParserTest.java

@@ -0,0 +1,42 @@
+package duke;
+


can delete extra space

yuki-cell · 2020-09-01T20:45:53Z

src/main/java/duke/Parser.java

+     * @throws DateException If the date provided is of the wrong format.
+     * @throws DukeException If the user command is invalid.
+     */
+    public static Command parse(String fullCommand) throws MissingInformationException, DateException, DukeException {


maybe can make each command's string as enum?

Add assertions to Parser and TaskList methods

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

Improve code quality and allow gui to exit after user types bye

Implement sorting of tasks

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

Add Gradle support

3b19ba1

Implemented greet,echo,exit

a5d34e6

Implemented add and list tasks

489bbc5

Allow tasks to be marked as done

f86d3c6

Implemented 3 kinds of task: Todo, Deadline and Event

679c3c7

Implement UI testing

97d29df

Handle incorrect inputs by user

d8cfc8c

Allow deleting of tasks from the list

7a81bca

Use Enums for the different types of task

ac88dd7

Add load and save tasks functionality

15dc295

Implement date and time

c39b2fa

Merge branch 'branch-Level-7'

a0d3164

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Merge branch 'branch-Level-8'

afb9d85

Extract the classes: Ui, Storage, Parser, TaskList

c0c1f88

Organise into packages

b535ae0

Add JUnit Tests

05b0e9a

Create a JAR File

936cb7f

Add JavaDoc comments

f1d55ab

Make styling adjustments to comply with coding standards

55e67d6

Allow users to find tasks by searching a keyword

baff8ec

Merge branch 'branch-A-JavaDoc'

460e916

Merge branch 'branch-A-CodingStandard'

ae63890

Merge branch 'branch-Level-9'

11c86d8

sigmund-c reviewed Aug 31, 2020

View reviewed changes

Extract Command classes and implement new jUnit tests for Parser class

3d35963

sc-arecrow reviewed Aug 31, 2020

View reviewed changes

shadowezz added 2 commits September 1, 2020 11:47

Merge branch 'add-gradle-support' into branch-A-Gradle

70b9fa0

Include checkstyle and refactor code to follow checkstyle rules

bb1b803

yuki-cell reviewed Sep 1, 2020

View reviewed changes

Add GUI to Duke using JavaFx

f75b30d

shadowezz and others added 12 commits September 9, 2020 22:19

Merge branch 'branch-Level-10'

5f50bea

Add assertions to Parser and TaskList methods

fd06e81

Improve code quality and allow gui to exit after user types bye

ed349cb

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

b376701

Add assertions to Parser and TaskList methods

Merge branch 'master' into branch-A-CodeQuality

c259c3b

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

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

c94787a

Improve code quality and allow gui to exit after user types bye

Implement sorting of tasks

6626d54

Merge pull request #3 from shadowezz/branch-C-Sort

af2ac0e

Implement sorting of tasks

Follow checkstyle guide

4dfd1c2

Fix bug where jar file cannot be launched

f99305d

Add user guide and product website

8be0b88

Rename UI.png to Ui.png

01363ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cao Wenjie] iP #351

[Cao Wenjie] iP #351

shadowezz commented Aug 25, 2020

sigmund-c left a comment

sigmund-c Aug 31, 2020

sigmund-c Aug 31, 2020

sc-arecrow Aug 31, 2020

sigmund-c Aug 31, 2020

sc-arecrow Aug 31, 2020

shadowezz Aug 31, 2020

sc-arecrow left a comment

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

sc-arecrow Aug 31, 2020

shadowezz Aug 31, 2020

yuki-cell left a comment

yuki-cell Sep 1, 2020

yuki-cell Sep 1, 2020

yuki-cell Sep 1, 2020

yuki-cell Sep 1, 2020

yuki-cell Sep 1, 2020

yuki-cell Sep 1, 2020

		import duke.exception.MissingInformationException;

		import java.io.IOException;

		import org.junit.jupiter.api.Test;
		import static org.junit.jupiter.api.Assertions.assertEquals;

[Cao Wenjie] iP #351

Are you sure you want to change the base?

[Cao Wenjie] iP #351

Conversation

shadowezz commented Aug 25, 2020

sigmund-c 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

sc-arecrow 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

yuki-cell 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