[Hong Wen] Duke increments #66

aegis-bot · 2020-01-26T17:35:09Z

Duke Increments

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

Change file mode on `gradle` to be executable (nus-cs2103-AY1920S2#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

The OpenJFX plugin expects applications to be modular and bundled with jlink, resulting in fat jars that are not cross-platform. Let's manually include the required dependencies so that shadow can package them properly.

…duke to new classes.

… more functions to tasklist.

…ng in inputloop

adding gradle support to duke

…Main.java in place

…du & main.java

…& testReadCommand() to ParserTest

yjskrs

Like how you give user instructions at bot start up! Also really clean code and liked that you incorporated exceptions.

src/main/java/seedu/java/util/Ui.java

yjskrs · 2020-02-02T07:59:33Z

src/main/java/seedu/java/util/TaskList.java

+        return "Affirmative. Adding a to-do task :) \n" + "added: " + task + "\n" + totalTask();
+    }
+
+    private String addDead(String task, String timing) {


I feel that this method deserves a better name... What do you think?

Certainly XD

yjskrs · 2020-02-02T08:02:46Z

src/main/java/seedu/java/util/Duke.java

+        storage = new Storage(filePath);
+        try {
+            tasks = new TaskList(storage.load());
+        } catch (Exception e) {


Pokemon catching!! I think it might be better to specify the exact exceptions you want to catch (e.g. FileNotFoundException) and also for those methods where you just throw Exception, might be better to be more specific.

Thanks! I was wondering how to create custom exceptions until i reviewed other people's work!

yjskrs · 2020-02-02T08:04:03Z

src/main/java/seedu/java/util/Ui.java

+    /**
+     * Prints the logo & gives instructions.
+     */
+    public void intro() {


Can consider using more descriptive names for this class (i.e. introduceBot, readInput or something)

alcen

Overall, you made a really commendable effort!! I feel that you took great care in following general Object-Oriented principles, and including documentation for methods that may not be so understandable.

That being said, I have left comments on some areas of improvement to work on below, and I hope the comments are helpful!! Do add to the discussion as you read along so we can strive for an improved program

Javadocs are present for most methods, but I feel that more details can be given in some cases (see comments below for some examples)
Many Exceptions! You mentioned that you did not know how to create your own, and that could have led to the widespread general Exception usage. Hope you will change it, if possible!
Long if/else and switch statements are quite common, which could affect future extensibility of the program
I feel like you don't have to commit the .class files, as these can be easily compiled from the source code

alcen · 2020-02-03T13:39:29Z

src/main/java/seedu/java/util/Command.java

+     * @param cmd - a piece of text
+     * @return Command
+     */
+    public static Command convert(String cmd) {


I feel like another way you could do this, is by instantiating each Command constant with a String

Example:

public enum Command { ERROR("error"), TODO("todo"), ..., // all your other commands FIND("find"); private final String cmd; Command(String cmd) { this.cmd = cmd; } public static Command convert(String cmd) { for (Command c : Command.values()) { if (c.cmd.equals(cmd)) { return c; } } return ERROR; // maybe you could throw an Exception instead }

This would avoid the lengthy if/else statements and allow you to add new Commands easily!

alcen · 2020-02-03T13:43:00Z

src/main/java/seedu/java/util/Complete.java

+package seedu.java.util;
+
+public class Complete {
+    private boolean yesOrNo;


As per the coding style conventions, this could be named something like isComplete instead

alcen · 2020-02-03T13:49:27Z

src/main/java/seedu/java/util/Complete.java

@@ -0,0 +1,18 @@
+package seedu.java.util;
+
+public class Complete {


Seems like this class is only used in Task! I'm not very sure why a Complete class is used to wrap a boolean value here, could you please share the rationale? To me, I feel that encapsulation should not be used where it is not required

alcen · 2020-02-03T13:53:27Z

src/main/java/seedu/java/util/Event.java

+
+public class Event extends Task {
+    private String timing;
+


Seems like Event could make use of java.time.LocalDate as well, just like Deadline! In fact, I think it is a good idea to make both Event and Deadline extend some abstract class with concrete methods for formatting a String into a LocalDate, to ensure that code is not duplicated. Just a minor note for future extension

alcen · 2020-02-03T13:58:31Z

src/main/java/seedu/java/util/Duke.java

+    public void run() {
+        ui.intro();
+        ui.output(tasks.listToPrint());
+        while (true) {


To me, a while (true) loop makes it unclear whether the program will eventually exit the loop or not. Granted, the code inside the loop is quite short, and it is quite obvious that the program will break out of the loop when some condition is met. What are your thoughts?

alcen · 2020-02-03T14:29:26Z

src/main/java/seedu/java/util/TaskList.java

+        return "You have " + (taskArr.size()) + " task on your list.";
+    }
+
+    private String addTodo(String task) {


Again, possible violation of Open-Closed Principle where a new method will need to be created for every new Task! I feel that it would be better to have a general addTask(Task t) method to add all possible Tasks

alcen · 2020-02-03T14:32:16Z

src/main/java/seedu/java/util/TaskList.java

+        return toPrint;
+    }
+
+    private String listToPrint(ArrayList<Task> tasks) {


Seems like unnecessary code duplication! (the two listToPrint methods)

Would it be a better idea to just use the one with signature listToPrint(ArrayList<Task> tasks) and pass in this.taskArr when required? It may also make it more evident which list you are printing

alcen · 2020-02-03T14:33:06Z

src/main/java/seedu/java/util/TaskList.java

+    private String findTask(String keyword) {
+        ArrayList<Task> matchedTask = new ArrayList<>();
+        for (Task x: taskArr) {
+            if(x.getTask().contains(keyword)) {


I think that there should be a space after the if and before the brackets, following the common coding style conventions

alcen · 2020-02-03T14:43:16Z

src/main/java/seedu/java/util/TaskList.java

+    public String read(String input) {
+        try {
+            Command cmd = Parser.readCommand(input);
+            switch (cmd) {


To me, this switch statement feels like it could be improved, as it is another potential violation of Open-Closed Principle. Adding a new command will require a new case, and even a new method in class TaskList!

It would be quite tedious to solve this issue though. One way I have in mind would be to expand Command into a full class, and have it implement a method executeCommand(ArrayList<Task> tasksList, String task, String timing, ...) (you can use String[] or String... for the many Strings) so you would just call executeCommand here, regardless of what the command actually is. But that would require an overhaul of a large portion of the code

Any thoughts?

This is a great comment! Now it makes more sense as to why one of the increments suggested adding Commands and its subclasses now. At that point I didn't see much sense in it, but now, maybe I'll integrate that into my own Duke!

alcen · 2020-02-03T14:44:43Z

src/main/java/seedu/java/util/Ui.java

+                + "\n" + "DELETE -- Deletes a task. Type 'delete' & a task number to delete that task."
+                + "\n" + "LIST -- If you want to view what's on your tasklist, key in 'list'."
+                + "\n" + "FIND -- Too many Task that you're not sure which is which? key in 'find' & the key word. A list of task based on the keyword will be printed."
+                + "\n" + "BYE -- When you're sick of using this useless software and want to quit, type 'bye' & I'll terminate operation."


Like yjskrs said, interesting start-up message! Quite informative, though a bit long

Perhaps a way is unclutter the startup message would be to simply show "HELP -- Show a list of commands Duke can understand.", and place the code under that.

teo-jun-xiong

Overall, the logic of the code and the coding style is well done. Some pointers are:

Improve readability by inserting new lines appropriately, rather than clumping the code into one large block
Detailed JavaDocs comments, though not exactly a requirement at this stage of the iP
Abstract duplicated code out, e.g. in Deadline and Event, to its superclass, Task

teo-jun-xiong · 2020-02-04T15:32:22Z

src/main/java/data/duke.txt

@@ -0,0 +1,4 @@
+T/0/borrow book


Perhaps it would make sense to exclude files like this being committed to GitHub, since the list of Tasks will always be added at the initialization.

teo-jun-xiong · 2020-02-04T15:36:43Z

src/main/java/seedu/java/util/Parser.java

+     * @return Command
+     * @throws Exception
+     */
+    public static Command readCommand(String input) throws Exception {


I think that the javadocs for this method can be clearer, e.g. why is the Exception being thrown? Also, adding on to @alcen, it would be clearer to perhaps create a custom exception like InvalidCommandException.

teo-jun-xiong · 2020-02-04T15:38:05Z

src/main/java/seedu/java/util/Parser.java

+        String[] arr = input.split(" ");
+        String task = "";
+        int i = 1;
+        while (i < arr.length && !arr[i].startsWith("/"))  {


Adding new lines to separate blocks of code (while loop, if-block) increases readability!

teo-jun-xiong · 2020-02-04T15:39:07Z

src/main/java/seedu/java/util/Storage.java

+     * Loads up a txt file and converts it to an Arraylist of Task.
+     * @return ArrayList of Tasks
+     * @throws FileNotFoundException means your text file is missing/corrupted
+     */


The javadocs is clear and concise, documenting the reason for the thrown Exception.

teo-jun-xiong · 2020-02-04T15:40:12Z

src/main/java/seedu/java/util/Storage.java

+        return arr;
+    }
+
+    private static boolean toBool(String com) {


Just a pointer, this function can be simplified to

return com.equals("1");

teo-jun-xiong · 2020-02-04T15:42:33Z

src/main/java/seedu/java/util/Task.java

@@ -0,0 +1,47 @@
+package seedu.java.util;
+
+public class Task {


Perhaps making Task an abstract class can be considered, since a Task is never created. Todo, Event, and Deadline can simply extend it. Methods such as Task#completeTask, Task#getTask can be kept, while perhaps abstract methods can be implemented.

teo-jun-xiong · 2020-02-04T15:44:58Z

src/main/java/seedu/java/util/TaskList.java

+    public String read(String input) {
+        try {
+            Command cmd = Parser.readCommand(input);
+            switch (cmd) {


This is a great comment! Now it makes more sense as to why one of the increments suggested adding Commands and its subclasses now. At that point I didn't see much sense in it, but now, maybe I'll integrate that into my own Duke!

teo-jun-xiong · 2020-02-04T15:46:41Z

src/main/java/seedu/java/util/Ui.java

+                + "\n" + "DELETE -- Deletes a task. Type 'delete' & a task number to delete that task."
+                + "\n" + "LIST -- If you want to view what's on your tasklist, key in 'list'."
+                + "\n" + "FIND -- Too many Task that you're not sure which is which? key in 'find' & the key word. A list of task based on the keyword will be printed."
+                + "\n" + "BYE -- When you're sick of using this useless software and want to quit, type 'bye' & I'll terminate operation."


Perhaps a way is unclutter the startup message would be to simply show "HELP -- Show a list of commands Duke can understand.", and place the code under that.

…ext.

sinhayash · 2020-02-09T06:04:03Z

Good effort!

…o handle fxml files

…. Modified Duke to read and output as an application.

…ction to Tasklist

…l java files

j-lum and others added 14 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-AY1920S2#12 from j-lum/gradle+x

6e6ace1

Change file mode on `gradle` to be executable (nus-cs2103-AY1920S2#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-AY1920S2#13 from j-lum/javaexec

7b60e81

Add configuration for console applications

JavaFX tutorial: Support cross-platform JARs

c4678f7

The OpenJFX plugin expects applications to be modular and bundled with jlink, resulting in fat jars that are not cross-platform. Let's manually include the required dependencies so that shadow can package them properly.

JavaFX tutorial: Support cross-platform JARs [nus-cs2103-AY1920S2#16]

30efbae

Add inputloop function.

f173bac

Added Enum Command.

89473f0

Task & Tasklist class created. Input functions and enum removed from …

48a5093

…duke to new classes.

Create child of Task. Boolean completed converted into a class. Added…

0ad2338

… more functions to tasklist.

modified complete task. remove arrays for arraylist. exception handli…

1cb577b

…ng in inputloop

added delete method & enum

5a6afa2

aegis-bot changed the title ~~Hong Wen~~ [Hong Wen] Duke increments Jan 26, 2020

aegis-bot added 15 commits January 27, 2020 21:18

added auto-save function and resumeSave() method

de787bb

Modified Deadline class to handle LocalDate

5775d54

merged 7 & 8

d29ea14

incomplete 0.0.1. Added Ui, Parser, Command, Storage

5f433ca

incomplete 0.0.1. Added Ui, Parser, Command, Storage

74a9037

incomplete 0.0.1. Added Ui, Parser, Command, Storage

fa9597a

added ui, parser, storage. code cleaned

de9adf4

Merge branch 'gradle'

22bc5d8

adding gradle support to duke

cleared obsolete codes and added gradle

8582151

add plugins checkstyle

2d4a788

Packaged java files & transferred to seedu/java/util folder. Created …

b2b628d

…Main.java in place

created test copy of main. Deleted all files in main except data, see…

2c2e6f4

…du & main.java

added runtest.sh

df905ec

Removed Redundant testers, added testConvertTiming() to DeadlineTest …

1dd9bd9

…& testReadCommand() to ParserTest

added shadow plugin in jar

945f79e

aegis-bot added 5 commits February 1, 2020 22:48

Added JavaDocs to most public classes/methods & added manual on UI.java

947dfb0

Tweak to follow coding standard

0cc6630

added find() function & enumeration in TaskList & Command respectively

34982da

merged coding standard and javaDocs

b7a8a9d

merged level-9

ee9aeb5

yjskrs reviewed Feb 2, 2020

View reviewed changes

minor edits to instructions

18c17dd

alcen reviewed Feb 3, 2020

View reviewed changes

teo-jun-xiong reviewed Feb 4, 2020

View reviewed changes

This was referenced Feb 6, 2020

Feedback on week 4 project progress for alcen alcen/duke#4

Open

Feedback on week 4 project progress for teo-jun-xiong teo-jun-xiong/duke#3

Open

Feedback on week 4 project progress for yjskrs yjskrs/duke#3

Open

aegis-bot added 2 commits February 8, 2020 14:20

GUI features added. User can send text & duke replies back the same t…

f85ca08

…ext.

MainWindow & fxml files created. Fine tuning of Duke

cb0efd4

aegis-bot added 15 commits February 11, 2020 00:38

added Main & MainWindow class to package. Modified Duke & Dialogbox t…

1ad30b4

…o handle fxml files

minor edits

b718372

Added enum Text. Added Help & getHelp() fn to Command enum & TaskList…

3d2b5b9

…. Modified Duke to read and output as an application.

README.md added, DialogBox Vertical resize fixed, & avatar changed

ad0ad0b

cleaned up README.MD

93c8ffe

further modified docs/README.MD

a98fc2e

fixed MainWindow. 1.3.2 can run on Mac

1ecbf4f

added meow command & function to TaskList & Command. Added exit() fun…

a51d56b

…ction to Tasklist

added meow help to Text.java

9234564

added meow help to Text.java

a329849

Modifed Event class to handle LocalDate & LocalTime.

7eb0f3c

inserted assertion into Event class and removed redundant codes in al…

e39f003

…l java files

modified Command to be able to meowwwwwwww input

719ac85

Ui.png added

6555cbe

Userguide & Ui.png added

4445160

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hong Wen] Duke increments #66

[Hong Wen] Duke increments #66

aegis-bot commented Jan 26, 2020

yjskrs left a comment

yjskrs Feb 2, 2020

aegis-bot Feb 2, 2020

yjskrs Feb 2, 2020

aegis-bot Feb 2, 2020

yjskrs Feb 2, 2020

alcen left a comment

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

alcen Feb 3, 2020

teo-jun-xiong Feb 4, 2020

alcen Feb 3, 2020 •

edited

Loading

teo-jun-xiong Feb 4, 2020

teo-jun-xiong left a comment

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

teo-jun-xiong Feb 4, 2020

sinhayash commented Feb 9, 2020

		@@ -0,0 +1,18 @@
		package seedu.java.util;

		public class Complete {

[Hong Wen] Duke increments #66

Are you sure you want to change the base?

[Hong Wen] Duke increments #66

Conversation

aegis-bot commented Jan 26, 2020

yjskrs 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

alcen 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

alcen Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-jun-xiong 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

sinhayash commented Feb 9, 2020

alcen Feb 3, 2020 •

edited

Loading