[Sng Jia Ming Fadi Faris] Duke Increments #345

fadisng · 2019-09-02T04:14:51Z

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)

# Conflicts: # src/main/java/Parser.java

This reverts commit 9e36be2.

justarock111 · 2019-09-03T13:46:39Z

src/main/java/Parser.java

+            c = new DeleteCommand(Integer.parseInt(arr[1]) - 1);
+            break;
+        default:
+            c = new AddCommand(arr);


It may be more easier to specify the deadline, description, whether it is done and other attributes inside the constructor of AddCommand, since the String input is already split.

justarock111 · 2019-09-03T13:50:12Z

src/main/java/AddCommand.java

+        try {
+            if (next.equals("todo") || next.equals("deadline") || next.equals("event")) {
+                if (arr.length == 1) {
+                    throw new EmptyDescriptionException("☹ OOPS!!! The description of a " + next + " cannot be empty.");


It may be a little easier to have the message already in toString() method of EmptyDescriptionException, so you only have take in the type as a parameter. E.g throw new EmptyDescriptionException("todo"). Since you will be throwing this exception frequently, you won't need to type the message everytime. :)

justarock111 · 2019-09-03T13:50:45Z

src/main/java/AddCommand.java

+                    }
+                }
+            } else {
+                throw new UnknownTaskException("☹ OOPS!!! I'm sorry, but I don't know what that means :-(");


It may be a little easier to have the message already in toString() method of UnknownTaskException :)

justarock111 · 2019-09-03T13:53:05Z

src/main/java/AddCommand.java

+                t = new Todos(description.trim());
+            } else if (next.equals("deadline")) {
+                int index = description.indexOf("/");
+                String byWhen = description.substring(index + 4);


Good idea! Another idea to split your string by "...".split("/by") instead so you would not need to care about the index:)

justarock111 · 2019-09-03T13:54:27Z

src/main/java/AddCommand.java

+                String desc = description.substring(1, index - 1);
+                t = new Deadline(desc, byWhen);
+            } else {
+                int index = description.indexOf("/");


This is a smart idea! You can also split the string by "...".split("/at") so you would not need to count the index. :)

Just to add on, if you use the indexOf("/"), this might not have the intended effect when you have a deadline task initialised with the following command as an example:
deadline todo/or not to do that is the question /by 12/12/12 12:12
You can split as @Joanna-YJA suggested, and check if there is more than 2 items in the resulting String array. From there you may proceed to throw the appropriate DukeExceptions.

justarock111 · 2019-09-03T14:11:13Z

src/main/java/Storage.java

+            File f = new File(file);
+            Scanner sc = new Scanner(f);
+            Task t;
+            while (sc.hasNext()) {


Using Scanner is a good idea!

justarock111 · 2019-09-03T14:12:44Z

src/main/java/Storage.java

+            fw.write(textToAdd);
+            fw.close();
+        } catch (IOException e) {
+            System.out.println("File not found.");


I used some helper methods like updateText(int taskNum), deleteText(int taskNum) to help me execute 'done' , 'delete' and more commands. I would recommend that.

justarock111 · 2019-09-03T14:14:12Z

src/main/java/Ui.java

+    }
+
+    public String readCommand() {
+        Scanner sc = new Scanner(System.in);


It may not be necessary to create a new Scanner object everytime readCommand() is called.

justarock111 · 2019-09-03T14:16:44Z

src/test/java/ParserTest.java

+
+public class ParserTest {
+    @Test
+    public void parseTest() {


I would recommend using this naming standard to name your test methods: featureUnderTest_testScenario_expectedBehavior()

justarock111 · 2019-09-03T14:18:41Z

src/test/java/ParserTest.java

+public class ParserTest {
+    @Test
+    public void parseTest() {
+        assertEquals(new DoneCommand(2), Parser.parse("done 3"));


Could I ask why is the command DoneCommand(2) instead of DoneCommand(3) ? As the attribute name in constructor DoneCommand is called taskNum. Thank you!

Q-gabe

I think you did a good job! Aside from some possible bugs that may be missed in terms of parsing the input and other more graceful methods of implementation, it looks pretty solid. LGTM!

Q-gabe · 2019-09-05T13:45:05Z

src/main/java/AddCommand.java

+ */
+
+import java.text.ParseException;
+


I think it's custom to have the class's javadoc comments to be directly above the declaration of the class, after any relevant import statements.

Q-gabe · 2019-09-05T15:09:10Z

src/main/java/AddCommand.java

+                String desc = description.substring(1, index - 1);
+                t = new Deadline(desc, byWhen);
+            } else {
+                int index = description.indexOf("/");


Just to add on, if you use the indexOf("/"), this might not have the intended effect when you have a deadline task initialised with the following command as an example:
deadline todo/or not to do that is the question /by 12/12/12 12:12
You can split as @Joanna-YJA suggested, and check if there is more than 2 items in the resulting String array. From there you may proceed to throw the appropriate DukeExceptions.

Q-gabe · 2019-09-05T15:13:08Z

src/main/java/Deadline.java

+            return "D @ 0 @ " + this.description + " @ " + this.by;
+        }
+    }
+}


Consider adding newlines at the end of your files, as it is a general convention. It doesn't really cause any issues beside making GitHub and checkstyle whine, but there is a reason behind doing so, https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline. Hope this gives some insight!

Q-gabe · 2019-09-05T15:22:09Z

src/main/java/Storage.java

+    /**
+     * Rewrites the file with the new arraylist of tasks.
+     * @param list arraylist of tasks that are updated after every command given to Duke.
+     */


Perhaps it may be a more computationally-efficient approach to have writeFile run the moment an exit command runs as the purpose of storage is only for data-permanence through starting and exiting Duke. We need not actually track every manipulation of the task list with every command. 😉

Q-gabe · 2019-09-05T15:24:20Z

src/main/java/Ui.java

+            System.out.println("  " + t + "\nNow you have " + count + " task in the list.");
+        } else {
+            System.out.println("  " + t + "\nNow you have " + count + " tasks in the list.");
+        }


Nice attention to detail here. 👍

This reverts commit 615cda1.

# Conflicts: # src/main/java/Ui.java

ShaunNgTX · 2019-09-12T07:48:12Z

src/test/java/TodosTest.java

+    public void testToString() {
+        assertEquals("[T][\u2713] play basketball", new Todos("play basketball", true).toString());
+    }
+}


Very neat and well written code

Javadocs

Indentation

j-lum and others added 28 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)

Level - 1

ba11701

Level - 2

c80085c

Level - 3

244fb4c

Level - 4

c0d63f6

Level - 5

47514be

Level - 6

6cc2e4d

Level-7

b382bd3

Level-8

7fc08ec

Level-8

72b02cc

A-MoreOOP

a9033c2

A-JUnit

1c677e6

A-JUnit

31cc0a3

A-JUnit

4ea22d1

A-Jar

4065b7f

A-JavaDoc

743b785

A-CodingStandard

8c1dace

branch-Level-9

126535c

Merge branch 'branch-A-JavaDoc'

02cf134

Merge branch 'branch-Level-9'

9e1c42d

# Conflicts: # src/main/java/Parser.java

Merge branch 'gradle' of https://github.com/fadisng/duke

de2854d

A-Gradle

625f50e

Level-10

9e36be2

Revert "Level-10"

646cc92

This reverts commit 9e36be2.

branch-A-Gradle

1ebbacc

justarock111 reviewed Sep 3, 2019

View reviewed changes

Level-10

474710e

Q-gabe reviewed Sep 5, 2019

View reviewed changes

fadisng added 2 commits September 6, 2019 14:23

Level-10

e6e293d

Level-10

e71990a

nus-se-pr-bot added tutorial.CS2103T-T17 team.1 tutor.junrong09 labels Sep 6, 2019

fadisng added 6 commits September 10, 2019 00:28

A-Assertions

e32e2e1

A-CodeQuality

615cda1

Revert "A-CodeQuality"

cc3d359

This reverts commit 615cda1.

A-CodeQuality

c878f87

Merge branch 'branch-A-Assertions' into branch-A-CodeQuality

77a0e35

# Conflicts: # src/main/java/Ui.java

C-Update

ecd4eb2

ShaunNgTX reviewed Sep 12, 2019

View reviewed changes

fadisng and others added 8 commits September 28, 2019 15:39

Final changes

57ada88

UG update

72e8820

Ui

1639538

Indentation

2220297

Javadocs

e979df1

Merge pull request #5 from fadisng/Javadocs

c8aea88

Javadocs

Merge pull request #6 from fadisng/Checkstyle

2a8a2ce

Indentation

Update README.md

56de869

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sng Jia Ming Fadi Faris] Duke Increments #345

[Sng Jia Ming Fadi Faris] Duke Increments #345

fadisng commented Sep 2, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

Q-gabe Sep 5, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

justarock111 Sep 3, 2019

Q-gabe left a comment

Q-gabe Sep 5, 2019

Q-gabe Sep 5, 2019

Q-gabe Sep 5, 2019

Q-gabe Sep 5, 2019

Q-gabe Sep 5, 2019

ShaunNgTX Sep 12, 2019

[Sng Jia Ming Fadi Faris] Duke Increments #345

Are you sure you want to change the base?

[Sng Jia Ming Fadi Faris] Duke Increments #345

Conversation

fadisng commented Sep 2, 2019

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

Q-gabe 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