[Yuki Akizuki] iP #172

yuki-cell · 2020-08-22T14:54:55Z

No description provided.

# Conflicts: # iP/out/production/iP/Duke.class # iP/src/Duke.java

# Conflicts: # iP/src/duke/Ui.java

YangJiyu98

Very neat code! very easy to read

YangJiyu98 · 2020-08-31T05:57:06Z

iP/src/duke/Deadline.java

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


Maybe can consider having a empty new line to group your package/import statements

I second the suggestion made by @YangJiyu98

YangJiyu98 · 2020-08-31T05:59:38Z

iP/src/duke/Duke.java

+
+        } else {
+            // invalid input
+            if (!loading){


You might have missed a whitespace here

I second that

YangJiyu98 · 2020-08-31T06:00:01Z

iP/src/duke/Duke.java

+                ui.showError(e.getMessage());
+            }
+
+        } else if (instructionType.equals("event")){


You might have missed a white space here

I second that

YangJiyu98 · 2020-08-31T06:10:13Z

iP/src/duke/Duke.java

+    /**
+     * run todo app
+     */


Not sure if this would be necessary as it is simple enough fro a reader to understand

yeohhq

I think your code is generally easy to read and follow, with inline comments to also improve readability which is good! :)

yeohhq · 2020-08-31T14:02:30Z

iP/src/duke/Duke.java

@@ -1,6 +1,7 @@
 package duke;


Maybe you can have a new empty line indentation after your package line before your import statements?

Repeated suggestion. I second this as well.

yeohhq · 2020-08-31T14:06:39Z

iP/src/duke/TaskList.java

@@ -47,4 +47,14 @@ public Task getTask(int index) {
        Task task = taskList.get(index);
        return task;
    }
+
+    public ArrayList<Task> find(String keyword) {


I like how you return your output as an ArrayList to be processed later instead of immediately returning a processed string since this could improve adaptability and extensibility of your program! 👍

yeohhq · 2020-08-31T14:08:36Z

iP/src/duke/Ui.java

+
+    public void showFoundTask(ArrayList<Task> foundTasks) {
+        System.out.println("Here are the matching tasks in your list:");
+        int index = 1;


Perhaps it would be more readable if you had the for-loop in the form of:
for (int i = 1; i < foundTasks.size() + 1; i++) ?

I think that the current way is fine and I actually like it more

gordonfgz · 2020-08-31T14:20:03Z

iP/src/duke/Duke.java

@@ -1,6 +1,7 @@
 package duke;


gordonfgz · 2020-08-31T14:20:08Z

iP/src/duke/Deadline.java

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


gordonfgz · 2020-08-31T14:25:52Z

iP/src/duke/Duke.java

+                ui.showError(e.getMessage());
+            }
+
+        } else if (instructionType.equals("event")){


gordonfgz · 2020-08-31T14:26:19Z

iP/src/duke/Duke.java

+
+        } else {
+            // invalid input
+            if (!loading){


gordonfgz · 2020-08-31T14:27:20Z

iP/src/duke/Ui.java

+
+    public void showFoundTask(ArrayList<Task> foundTasks) {
+        System.out.println("Here are the matching tasks in your list:");
+        int index = 1;


I think that the current way is fine and I actually like it more

gordonfgz · 2020-08-31T14:30:52Z

iP/src/duke/Duke.java

+            tasks = new TaskList();
+            tasks = new TaskList(storage.load(this));


Perhaps there is a way of changing your code design so that you can initialise with just one line of code instead of calling the constructor twice?

gordonfgz · 2020-08-31T14:33:31Z

iP/src/duke/Duke.java

+        while (!isExit) {
+            try {
+                String fullCommand = ui.readCommand();
+                ui.showLine(); // show the divider line ("_______")


Perhaps you can put your comment on top of the line of code instead of beside it?
Not really sure if this violates Java coding standard.

gordonfgz · 2020-08-31T14:35:52Z

iP/src/duke/Duke.java

+            // quit
+            ui.showGoodBye();
+            System.exit(0);
+
+        } else if (instructionType.equals("list")) {
+            // list task
+            ui.showListOfTask(tasks.taskList);
+
+        } else if (instructionType.equals("done")) {
+            // mark done
+            // parse instruction
+            int index = Parser.parseMarkDoneInstr(user_input);
+            // execute
+            Task chosenTask = tasks.getTask(index);
+            chosenTask.markAsDone();
+            if (!loading){
+                ui.showMarkedDoneTask(chosenTask);
+            }
+
+        } else if (instructionType.equals("delete")) {
+            // delete task
+            // parse instruction
+            int index = Parser.parseDeleteInstr(user_input);
+            // execute
+            Task chosenTask = tasks.getTask(index);
+            tasks.deleteTask(index);
+            if (!loading){
+                ui.showDeletedTask(chosenTask, tasks.taskList);
+            }
+
+        } else if (instructionType.equals("find")) {
+            // find task
+            // parse instruction
+            String keyword = Parser.parseFindInstr(user_input);
+            // execute
+            ArrayList<Task> foundTasks = tasks.find(keyword);
+            if (!loading){
+                ui.showFoundTask(foundTasks);
+            }
+
+        } else if (instructionType.equals("todo")) {
+            // make todo
+            try {
+                // parse instruction
+                String description = Parser.parseAddTodoInstr(user_input);
+                // execute
+                Task todo = new Todo(description);
+                tasks.addTask(todo);
+                if (!loading){
+                    ui.showAddedTask(todo, tasks.taskList);
+                }
+            } catch (DukeException e) {
+                ui.showError(e.getMessage());
+            }
+
+        } else if (instructionType.equals("deadline")) {
+            // make deadline
+            try {
+                // parse instruction
+                HashMap<String, Object> parsedData = Parser.parseAddDeadlineInstr(user_input);
+                String description = (String) parsedData.get("description");
+                LocalDate l_time = (LocalDate) parsedData.get("time");
+                // execute
+                Task deadline = new Deadline(description, l_time);
+                tasks.addTask(deadline);
+                if (!loading) {
+                    ui.showAddedTask(deadline, tasks.taskList);
+                }
+            } catch (DukeException e) {
+                ui.showError(e.getMessage());
+            }
+
+        } else if (instructionType.equals("event")){
+            // make event
+            try {
+                // parse instruction
+                HashMap<String, Object> parsedData = Parser.parseAddEventInstr(user_input);
+                String description = (String) parsedData.get("description");
+                LocalDate l_time = (LocalDate) parsedData.get("time");
+                // execute
+                Task event = new Event(description, l_time);
+                tasks.addTask(event);
+                if (!loading){
+                    ui.showAddedTask(event, tasks.taskList);
+                }
+            } catch (DukeException e) {
+                ui.showError(e.getMessage());
+            }
+
+        } else {
+            // invalid input
+            if (!loading){
+                throw new DukeException("☹ OOPS!!! I'm sorry, but I don't know what that means :-(");
+            }
+        }
+
+        // save Data for every user input
+        Storage.save(tasks.taskList);
+    }
+}


Should this be extracted out? Any reasons why you chose to have the input processing method in Duke.java instead of a separate class?

gordonfgz · 2020-08-31T14:37:36Z

iP/src/duke/Task.java

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


I like how you named this member. It is very clear that its going to be a boolean!

gordonfgz · 2020-08-31T15:35:40Z

iP/src/duke/TaskList.java

@@ -47,4 +47,14 @@ public Task getTask(int index) {
        Task task = taskList.get(index);
        return task;
    }
+
+    public ArrayList<Task> find(String keyword) {


yongping827

I would suggest to gitignore build files, i.e. in this context, the directories iP/.idea and iP/out. The reason behind this is any changes to the code would change the files in these directories, and would create too many unnecessary changes in the git history.

yongping827 · 2020-08-31T16:12:42Z

iP/src/duke/Deadline.java

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


I second the suggestion made by @YangJiyu98

yongping827 · 2020-08-31T16:16:04Z

iP/src/duke/Deadline.java

+    protected LocalDate deadline;
+
+    /**
+     * constructor of Deadline


Perhaps capitalize the beginning of JavaDoc comments? This applies to all other JavaDoc comments as well.

yongping827 · 2020-08-31T16:18:04Z

iP/src/duke/Duke.java

@@ -1,6 +1,7 @@
 package duke;


Repeated suggestion. I second this as well.

yongping827 · 2020-08-31T16:18:47Z

iP/src/duke/Duke.java

+import java.io.IOException;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.HashMap;


Perhaps group imports based on the packages and leave an empty line between groups?

yongping827 · 2020-08-31T16:21:58Z

iP/src/duke/Duke.java

+                Command c = Parser.parse(fullCommand);
+                c.execute(tasks, ui, storage);
+                isExit = c.isExit();
+                 */


Not part of the Java coding standard, but perhaps remove commented code that I would presume is unwanted?

yongping827 · 2020-08-31T16:22:40Z

iP/src/duke/Duke.java

+                ui.showError(e.getMessage());
+            }
+
+        } else if (instructionType.equals("event")){


I second that

yongping827 · 2020-08-31T16:22:48Z

iP/src/duke/Duke.java

+
+        } else {
+            // invalid input
+            if (!loading){


I second that

jianhandev · 2020-09-01T15:12:36Z

iP/src/duke/Duke.java

+        ui = new Ui();
+        storage = new Storage(filePath);
+        try {
+            // need empty tasks to load properly


From the java coding standards: "Logical units within a block should be separated by one blank line."
Consider leaving a line before the try block?

jianhandev · 2020-09-01T15:14:12Z

iP/src/duke/Duke.java

+                // parse instruction
+                HashMap<String, Object> parsedData = Parser.parseAddEventInstr(user_input);
+                String description = (String) parsedData.get("description");
+                LocalDate l_time = (LocalDate) parsedData.get("time");


Variable name could be more descriptive.

jianhandev · 2020-09-01T15:16:34Z

iP/src/duke/Parser.java

+    static HashMap<String, Object> parseAddDeadlineInstr(String user_input) throws DukeException {
+        // check if input is valid
+        if (user_input.split(" ", 2).length == 1) {
+            throw new DukeException("☹ OOPS!!! The description of a deadline cannot be empty.");
+        }
+        String task = user_input.split(" ", 2)[1];
+        if (task.split(" /by ", 2).length < 2) {
+            throw new DukeException("☹ OOPS!!! The description and time is required for deadline");
+        }
+        // get data
+        String description = task.split(" /by ", 2)[0];
+        String time = task.split(" /by ")[1];
+        LocalDate l_time = LocalDate.parse(time);
+        HashMap<String, Object> parsedData = new HashMap<String, Object>(){
+            { put("description", description); put("time", l_time); }
+        };
+        return parsedData;
+    }


This block is generally abit chaotic, perhaps add in line breaks to separate different logical units?

jianhandev · 2020-09-01T15:21:16Z

iP/src/duke/Ui.java

+    }
+
+    /**
+     * show welcome message when app is started


Javadocs:

"show" should be "Shows"

add a full stop after the function description

before: root = CS2103 > iP(redundant folder) > folders after: root = CS2103 > folders

Merge A-Assertions branch to master branch

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

Branch a code quality branch to master branch

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

Add Gradle support

3b19ba1

commit Level-1

fcecf4c

commit level-2

010ee55

commit Level 3

36a83cf

commit Level-4

3e96f88

used A-TextUiTesting

c56e5f0

commit Level-5

90baee8

commit Level-6

dafff01

commit Level-7

78a1cbb

commit branch Level-8

2f97da1

Merge branch 'branch-Level-8'

a559047

# Conflicts: # iP/out/production/iP/Duke.class # iP/src/Duke.java

commit A-MoreOOP

c8ea186

commit A-Packages

f8eb113

commit A-JUnit

c5f491d

commit A-JUnit(newer)

a4459fd

commit A-Jar

712971f

commit A-JavaDoc

ae4d16d

commit A-CodingStandard

a6f5645

commit branch Level-9

0aa9fec

Merge branch 'branch-Level-9'

0b8ee55

# Conflicts: # iP/src/duke/Ui.java

YangJiyu98 reviewed Aug 31, 2020

View reviewed changes

Merge commit '3b19ba1790d6035a9e246f5d4ae627bf68778606'

2d02832

yeohhq reviewed Aug 31, 2020

View reviewed changes

gordonfgz reviewed Aug 31, 2020

View reviewed changes

commit A-Gradle

9376ef8

yongping827 reviewed Aug 31, 2020

View reviewed changes

jianhandev reviewed Sep 1, 2020

View reviewed changes

yuki-cell added 3 commits September 2, 2020 04:53

commit Level-10

1466ab7

commit FXML implementation of GUI

07715e8

commit change in root folder

4da2ef9

before: root = CS2103 > iP(redundant folder) > folders after: root = CS2103 > folders

yuki-cell added 25 commits September 7, 2020 21:53

commit A-Assertions

d991ad0

commit A-CodingQuality

a03b71c

Merge pull request #2 from yuki-cell/branch-A-Assertions

97f662d

Merge A-Assertions branch to master branch

Merge branch 'master' into branch-A-CodeQuality

1b4bd3f

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

Merge pull request #3 from yuki-cell/branch-A-CodeQuality

9180776

Branch a code quality branch to master branch

commit BCD-Extension (C-Priority)

0c3e22f

commit A-UserGuide

96af008

Set theme jekyll-theme-cayman

1e0be61

commit A-Release

70b6b4e

Merge branch 'master' of https://github.com/yuki-cell/ip

4c37a48

commit split UserInputHandler() function to multiple Command class

900bbee

commit test update of user guide website

ae7bfd6

Commit update user guide

5011359

Commit update user guide

5668017

commit update user guide

b0c5d8e

commit update user guide

8717ce9

commit update user guide

0ad1fde

commit update user guide

844e8b4

commit update user guide

2cae70a

commit update user guide

b531ef3

commit update user guide

fa4ac9b

commit update user guide

8da7637

commit update user guide

2b06a1f

commit fix minor issues

d72ff2c

commit fix minor issues

b78a523

		tasks = new TaskList();
		tasks = new TaskList(storage.load(this));

[Yuki Akizuki] iP #172

Are you sure you want to change the base?

[Yuki Akizuki] iP #172

Conversation

yuki-cell commented Aug 22, 2020

YangJiyu98 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

yeohhq 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

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

yongping827 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

Choose a reason for hiding this comment