[Syed Muhammad Aljunied] Duke Increments by muhammadaljunied · Pull Request #341 · nus-cs2103-AY1920S1/duke

muhammadaljunied · 2019-09-01T16:12:08Z

No description provided.

Add toolVersion block in to Gradle code sample to prevent errors.

Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)

Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.

Add configuration for console applications

Added Gradle Support Added Checkstyle plugin Added Shadow plugin Added JUnitTest Dependency

This reverts commit 721456d.

Added Gradle Support Added Checkstyle plugin Added Shadow plugin Added JUnitTest Dependency

Added Echo Added Exit

Implemented "List all task" feature

This reverts commit 708252a.

Implemented "List all task" feature

Implemented pattern matching for commands and arguments with regex Abstracted out Commands to reduce excessive if-else for executing commands Specific commands print their relevant messages Added Mark as completed

Resolved Checkstyle Errors Added package seedu.duke.trackables ahead of level-4 requirements

Refactored task object for abstraction Refactored Command objects to reflect abstraction of task object.

Updated patterns for Regex Added Support for Event, Todo, Deadline Added Error Abstractions for Common errors (Invalid Command, Invalid Arguments) Duke specific exceptions are thrown with original cause (if available) to provide more useful stack trace. ** Forgot to commit Level 4, thus tagged as level 4 and 5)

TSAI-HSIAO-HAN

The overall Java style seems correct. Can break some lines to increase readability.

TSAI-HSIAO-HAN · 2019-09-03T09:51:16Z

+    @Override
+    public void execute(List<Task> tasks) {
+        tasks.add(deadline);
+        echo(


The indentation for wrapped lines in execute method should be 8 spaces.

Line 20 should be together with Line 19.

TSAI-HSIAO-HAN · 2019-09-03T09:52:59Z

+    @Override
+    public void execute(List<Task> tasks) {
+        tasks.get(taskId).markAsDone();
+        echo(


The indentation for wrapped lines in execute method should be 8 spaces.

Line 19 should be together with Line 18.

TSAI-HSIAO-HAN · 2019-09-03T09:53:13Z

+    @Override
+    public void execute(List<Task> tasks) {
+        tasks.add(event);
+        echo(new String[] {


The indentation for wrapped lines in execute method should be 8 spaces.

This is the indentation that CheckStyle approves.

Line 19 at level 8
Line 20-22 at level 12
Line 23 at level 8 (to close the String array instantiation)

ChrisKheng

Last note: In the DeleteCommand class, maybe you want to consider not to decrease the index of the task by 1? Cuz I think maybe in the future you may need to retrieve the index of the task from the command and hence it may cause confusion or bug when the index is different from the "surface" index (i.e. the id starts from 1 when displaying the tasks). You can decrease the index by 1 in the execute() method when trying to retrieve the task.

Overall, I think you could improve on the structure of the program by separating the logic of the program into more classes (Single Responsibility Principle). This can make it easier to manage the program's code, especially when the program scale becomes larger and larger. All the best and cheers!

ChrisKheng · 2019-09-05T11:26:29Z

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class Duke {


Regarding Duke class, maybe you want to separate the functionalities in it into more classes? Like Storage, Parser etc. I think there are too many functionalities in the Duke class and it doesn't really correspond to the Single Responsibility principle in SOLID. It may make the process of changing the program's code to fit new requirement harder as it may be quite hard to locate the right piece of code to change among the extensive lines of code.

Also, I think the purpose of Duke class should act like a body which links all its parts(Storage, Parser, TaskList) together and not doing too much of logic. This concept may help you to refactor your code.

ChrisKheng · 2019-09-05T11:29:17Z

+     * Prints out the message.
+     * @param messages Array of messages to show.
+     */
+    protected void echo(String[] messages) {


Maybe you want to separate the logic of printing, handling database into more classes such as Storage and Parser? This would reduce the responsibility of the Command classes and may aid in coding or making changes when doing further extensions of the program.

ChrisKheng · 2019-09-05T11:30:27Z

+
+    @Override
+    public void execute(List<Task> tasks) {
+        tasks.get(taskId).markAsDone();


Maybe you want to assign the task that you have retrieved to a variable so that later you can directly call toString() on it? This will then prevent from doing two get(), which means traversing twice through the list, which can increase the complexity of the solution.

ChrisKheng · 2019-09-05T11:31:02Z

+
+import java.util.List;
+
+public class TodoCommand extends Command {


Maybe it's good to rename the TodoCommand, DeadlineCommand and EventCommand to maybe AddToDoCommand, AddDeadlineCommand and AddEventCommand? This can improve the clarity of the code.

ChrisKheng · 2019-09-05T11:31:32Z

+public class Task {
+    protected String description;
+    protected boolean isDone;
+


Maybe it would be good to make an enum to label the respective classes (ToDo, Deadline, Event)? This may help in the future when you want to extend your program.

ChrisKheng · 2019-09-05T11:32:07Z

+
+public class Event extends Task {
+
+    protected String at;


Maybe you want to use another name such as "time" instead of "at" for the string which stores the date? I think this would improve the clarity of your code.

ChrisKheng · 2019-09-05T11:32:38Z

+
+public class Deadline extends Task {
+
+    protected String by;


Maybe you want to use "deadline" instead of "by" to improve the clarity of the code?

Resolved Checkstyle Issues

Refactored Command Arguments parsing into respective command objects Error handling now cleaner with more specific error messages.

# Conflicts: # src/main/java/seedu/duke/Duke.java # src/main/java/seedu/duke/trackables/Deadline.java

…flict on Duke.java.

Abstracted out Ui and printing operations to Ui.java from Duke.java Updated Exceptions with new constructor e*(String message)

Updated Javadocs. Updated UI to handle reading inputs. Refactored IndexOutOfBounds error handling to TaskList from DeleteCommand and DoneCommand.

This reverts commit da3854e.

* branch-JavaDoc: Added Complete JavaDoc.

* branch-CodingStandard: Updated codebase to match Java Coding Standards. # Conflicts: # src/main/java/seedu/duke/commands/AddCommand.java # src/main/java/seedu/duke/trackables/Event.java

…e previous commit.

Linked UI with Duke.

Added missing Javadocs. Added UI tweaks

Added Stop handler when user closes using window close Added wrapping width for text node in UI

j-lum and others added 24 commits August 6, 2019 15:25

Add support for Gradle workflow

65f72a8

Add sample checkstyle configuration

0112efe

Add toolVersion block in to Gradle code sample to prevent errors.

Change file mode on gradle to be executable

cfd6da7

Merge pull request nus-cs2103-AY1920S1#12 from j-lum/gradle+x

6e6ace1

Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)

Add configuration for console applications

a3ca5a4

Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.

Merge pull request nus-cs2103-AY1920S1#13 from j-lum/javaexec

7b60e81

Add configuration for console applications

Merge branch 'gradle'

f26d506

Restructured project package to match seedu.duke

721456d

Added Gradle Support Added Checkstyle plugin Added Shadow plugin Added JUnitTest Dependency

Revert "Restructured project package to match seedu.duke"

4b0e11d

This reverts commit 721456d.

Restructured project package to match seedu.duke

927003f

Added Gradle Support Added Checkstyle plugin Added Shadow plugin Added JUnitTest Dependency

Added Text UI Test script

ba90bb3

Correct directory structure for main.

d90d445

Added Greeting

f266333

Added Echo Added Exit

Implemented "Add to list" feature

ab09b93

Implemented "List all task" feature

Revert "Implemented "Add to list" feature"

9aeab16

This reverts commit 708252a.

Implemented "Add to list" feature

122ef5a

Implemented "List all task" feature

Replaced Horizontal Line printing with Method printLine()

1cd0b74

Resolve Conflicts

da82080

Added Enums for Commands to easily identify.

eabde14

Implemented pattern matching for commands and arguments with regex Abstracted out Commands to reduce excessive if-else for executing commands Specific commands print their relevant messages Added Mark as completed

Added error handling for parsing of command input

b6374a5

Resolved Checkstyle Errors Added package seedu.duke.trackables ahead of level-4 requirements

Added Abstraction for Tasks - Event, Todo & Deadline

2af728d

Refactored task object for abstraction Refactored Command objects to reflect abstraction of task object.

Added Commands to support 3 new trackables

565e91b

Delete feature added

565e66e

TSAI-HSIAO-HAN reviewed Sep 3, 2019

View reviewed changes

Resolve Formatting issue for PR Comment

84c5441

ChrisKheng reviewed Sep 5, 2019

View reviewed changes

nus-se-pr-bot added team.4 tutorial.CS2103-T14 tutor.kylase-learning labels Sep 6, 2019

muhammadaljunied and others added 30 commits September 12, 2019 14:38

Corrected Travis Script

37dddf2

Added Save/Load Feature

e9405ca

Resolved Checkstyle Issues

Added support for Date (Event and Deadline type tasks)

5963c68

Refactored Command Arguments parsing into respective command objects Error handling now cleaner with more specific error messages.

Merge branch 'branch-Level-8'

eb6f476

# Conflicts: # src/main/java/seedu/duke/Duke.java # src/main/java/seedu/duke/trackables/Deadline.java

Resolved Regression due to Level-7/Level-8 Merge and missed Merge Con…

f81ab80

…flict on Duke.java.

Abstracted out Parsing into Parser.java from Duke.java

525b6b8

Abstracted out Ui and printing operations to Ui.java from Duke.java Updated Exceptions with new constructor e*(String message)

Resolved Regressions from Abstracting UI

9c7bebb

Resolved regression caused by abstracting List<Task> to TaskList.java.

ae8268c

Updated Javadocs. Updated UI to handle reading inputs. Refactored IndexOutOfBounds error handling to TaskList from DeleteCommand and DoneCommand.

Added JUnit Testing for DeleteCommand, DoneCommand and Parser

77a384d

Implemented Find functionality

da3854e

Revert "Implemented Find functionality"

d82b284

This reverts commit da3854e.

Implemented Find functionality

aa20df6

Added Complete JavaDoc.

cf56d68

Updated codebase to match Java Coding Standards.

d92462d

Merge branch 'branch-JavaDoc'

f6dca9a

* branch-JavaDoc: Added Complete JavaDoc.

Merge branch 'branch-CodingStandard'

b8e2b87

* branch-CodingStandard: Updated codebase to match Java Coding Standards. # Conflicts: # src/main/java/seedu/duke/commands/AddCommand.java # src/main/java/seedu/duke/trackables/Event.java

Corrected the 0-Index issue with delete and done command.

2c1f4cd

Fixed a Dead format issue and reversed the 0-index correction from th…

f38016b

…e previous commit.

Added JavaFX Support

a4b785f

Updated Commands to return the response as a String.

a35f3ad

Resolved clipping issue with DialogBox.

9989611

Linked UI with Duke.

Added Assertions (#6)

f00a2c3

Updated codebase to match Code Quality.

3f199be

Added missing Javadocs. Added UI tweaks

Updated Photos

43986b8

Added Stop handler when user closes using window close Added wrapping width for text node in UI

Added Ui Screenshot

f7e9957

Updated build for Cross Platform support.

49a883c

Incremented Build

73d5a54

Updated README.md

6d68dcd

update README.MD

d56b7f4

Set theme jekyll-theme-cayman

fc3ad4e


		import java.util.List;

		public class TodoCommand extends Command {

Conversation

muhammadaljunied commented Sep 1, 2019

Uh oh!

TSAI-HSIAO-HAN 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!

ChrisKheng 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants