[Long Zeling] iP #235

zeling595 · 2020-08-23T12:02:43Z

No description provided.

PhongTran98

In general, I think your code is very clean and well-structured aside from some minor fixable places here and there. Also, I really like your logo :)

PhongTran98 · 2020-08-31T16:20:49Z

src/main/java/seedu/duke/Parser.java

+     * @return Command.
+     * @throws DukeException If the input is incomplete.
+     */
+    public static Command parse(String str) throws DukeException {


Perhaps consider a clearer name for the method's parameter such as "command" or "input"?

PhongTran98 · 2020-08-31T16:27:45Z

src/main/java/seedu/duke/Parser.java

+import java.time.format.DateTimeFormatter;
+import java.util.regex.Pattern;
+
+import main.java.seedu.duke.commands.*;


Should this import statement list all sub-packages explicitly?

PhongTran98 · 2020-08-31T16:27:59Z

src/main/java/seedu/duke/Parser.java

+     * @throws DukeException If the input is incomplete.
+     */
+    public static Command parse(String str) throws DukeException {
+        String[] split1 = str.split(" ", 2);


Perhaps consider changing "split1" into another name like "tokens" or "words"?

PhongTran98 · 2020-08-31T16:30:58Z

src/main/java/seedu/duke/Parser.java

+     */
+    public static Task parseTaskFromStorage(String taskStr) {
+        Task savedTask;
+        String[] split = taskStr.split(Pattern.quote(" | "));


I think this variable name "split" here and some other places below might could be changed into a more meaningful name, too.

PhongTran98 · 2020-08-31T16:38:19Z

src/main/java/seedu/duke/TaskList.java

+            System.out.println(" " + " " +
+                    "[" + taskToBeDeleted.getStatusIcon() + "] " +
+                    taskToBeDeleted.getDescription());


Good work for remembering to break lines before they get too long! But shouldn't the line be broken before the operators? I think you could move the trailing plus signs to the beginning of the next line.

PhongTran98 · 2020-08-31T16:46:28Z

src/main/java/seedu/duke/commands/DoneCommand.java

+<<<<<<< HEAD
+        tasks.doneTasks(taskNo);
+=======
+        tasks.doneTask(taskNo);
+        // ui.showDoneMessage();
+>>>>>>> branch-A-CodingStandard


Should the merge conflict be resolved before being pushed into the remote repo?

PhongTran98 · 2020-08-31T16:48:56Z

src/main/java/seedu/duke/todo/Event.java

+ * Represents an event happening at a particular time.
+ */
+public class Event extends Task {
+    protected LocalDate at;


Should the variable name be changed into a more meaningful one like "time" or "date"?

UncleGrandpa925 · 2020-09-01T09:28:37Z

data/duke.txt

+T | 0 | borrow book
+E | 0 | project meeting | Oct 15 2020


Should the data file be ignored?

UncleGrandpa925 · 2020-09-01T09:31:39Z

src/main/java/seedu/duke/Parser.java

+            if (commandWord.equals("list")) {
+                return new ListCommand();
+            } else if (commandWord.equals("bye")) {
+                return new ExitCommand();
+            } else if (commandWord.equals("done")) {
+                throw new DukeException("Please tell me the task number that you have completed.");
+            } else if (commandWord.equals("date")) {
+                throw new DukeException("Please provide a date.");
+            } else if (commandWord.equals("delete")) {
+                throw new DukeException("Please tell me the task number that you want to delete.");
+            } else if (commandWord.equals("todo")) {
+                throw new DukeException("OOPS!!! The description of a todo cannot be empty.");
+            } else if (commandWord.equals("deadline")) {
+                throw new DukeException("OOPS!!! The description of a deadline cannot be empty.");
+            } else if (commandWord.equals("event")) {
+                throw new DukeException("OOPS!!! The description of a event cannot be empty.");
+            } else {
+                throw new DukeException("OOPS!!! I'm sorry, but I don't know what that means :-(");
+            }


Perhaps this part should use switch case? And also for the todo/deadline/event, I think you can use String format to reduce the number of duplicate code

UncleGrandpa925 · 2020-09-01T09:32:47Z

src/main/java/seedu/duke/Parser.java

+     * @return Task.
+     */
+    public static Task parseTask(String taskStr) {
+        String[] splits = taskStr.split(" ",2);


Maybe change the name here from splits to tokens?

UncleGrandpa925

Hi I have finished reviewing your PR! Nice job!

UncleGrandpa925 · 2020-09-01T09:34:34Z

src/main/java/seedu/duke/Storage.java

+    public void readFile() {
+        // load the file first
+        File directory = new File(DIRECTORY_PATH);
+        if (!directory.exists()) {


I think the method name should be a verb, so maybe change exists() to checkExist instead?

UncleGrandpa925 · 2020-09-01T09:35:27Z

src/main/java/seedu/duke/Storage.java

+        } catch (IOException e) {
+            System.out.println(e.getMessage());
+        }


If there is an exception here, should the program be ended?

UncleGrandpa925 · 2020-09-01T09:36:05Z

src/main/java/seedu/duke/Storage.java

+        } else {
+            writeToFile(FILE_PATH, taskList.tasks.get(0).getData());
+            for (int i = 1; i < taskList.tasks.size(); i++) {
+                String taskData = taskList.tasks.get(i).getData();
+                appendToFile(FILE_PATH, taskData);
+            }
+        }


Maybe if you write return above then no need this else? To keep the "HAPPY" path straight?

UncleGrandpa925 · 2020-09-01T09:36:59Z

src/main/java/seedu/duke/Storage.java

+    public void appendToFile(String filePath, String textToAdd) {
+        try {
+            FileWriter fw = new FileWriter(filePath, true);
+            fw.write(textToAdd + System.lineSeparator());
+            fw.close();
+        } catch (IOException e) {
+            System.out.println(e.getMessage());
+        }
+    }
+
+    /**
+     * Write text to local file at filePath. This overwrites the whole file.
+     * @param filePath The relative path of the file.
+     * @param textToAdd The text to be added.
+     */
+    public void writeToFile(String filePath, String textToAdd) {
+        try {
+            FileWriter fw = new FileWriter(filePath);
+            fw.write(textToAdd + System.lineSeparator());
+            fw.close();
+        } catch (IOException e) {
+            System.out.println(e.getMessage());
+        }


Perhaps the two function should be merged into one to reduce duplicate codes?

UncleGrandpa925 · 2020-09-01T09:37:58Z

src/main/java/seedu/duke/TaskList.java

+        if (tasks.size() <= 0) {
+            throw new DukeException("You don't have any tasks.");
+        }
+        ArrayList<Task> taskList = new ArrayList<>();


I think the name of the taskList should be changed since it can be mistaken with tasks!

UncleGrandpa925 · 2020-09-01T09:38:19Z

src/main/java/seedu/duke/TaskList.java

+            if (taskDate != null) {
+                if (taskDate.isEqual(date)) {
+                    taskList.add(task);
+                }


I think the 2 if should be merged together!

UncleGrandpa925 · 2020-09-01T09:38:59Z

src/main/java/seedu/duke/TaskList.java

+     * Marks the task as done.
+     * @param taskNo The task number in the list.
+     */
+    public void doneTask(int taskNo) {


I think function names should be in verb-form

UncleGrandpa925 · 2020-09-01T09:40:30Z

src/main/java/seedu/duke/TaskList.java

+    }
+
+    public void searchKeyword(String keyword) throws DukeException {
+        if (tasks.size() <= 0) {


A very small thing but I think you may find useful that the size of the array cannot be negative, so I think just check if it's equal to 0 or not is enough! A better way is to check task.isEmpty()==true

UncleGrandpa925 · 2020-09-01T09:41:25Z

src/main/java/seedu/duke/Ui.java

+    public String getHorizontalLine() {
+        String line = "";
+        for (int i = 0; i < lengthOfLine; i++) {
+            line = line + "-";
+        }
+        return line;
+    }


I think the horizontal line should be a static variable and should only be created once

UncleGrandpa925 · 2020-09-01T09:41:59Z

src/main/java/seedu/duke/Ui.java

+ */
+public class Ui {
+    private int lengthOfLine = 55;
+    private String line;


I think the name line can be more descriptive!

added assertions

make changes in accordance to coding quality

…k status.

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

Add Gradle support

3b19ba1

Duke project level 1

86503a5

level 2 done

a62b5e5

level 3 done

aa6c5da

level 4 done

3ce26f7

uitest

bc677f4

level 5 completed

8b69d6a

level 6 done

adbd111

customised bot

95bb44a

leftover commit from week 2

23bdfe0

add save function

673cdf3

level 8 implement time

49a7fdb

level 8 completed

dd57bb6

Merge branch 'branch-Level-8'

8b8b586

refactor code

b09f4a6

organize into package

3071788

add Junit and tests

7396d3c

create jar file

ea028e9

add javaDocs

e341136

polish up code in compliance to coding standard

f2db26b

complete level 9. Add find command.

9952593

fix merge conflict

088a7e2

Merge branch 'branch-Level-9'

40ececf

Merge remote-tracking branch 'origin/add-gradle-support'

ce4cc8e

add gradle support

08ee0d1

add checkstyle config

13fac64

add GUI

173e675

Merge branch 'branch-A-CheckStyle'

f7ff9d1

add javafx

d3c7b25

PhongTran98 reviewed Aug 31, 2020

View reviewed changes

UncleGrandpa925 reviewed Sep 1, 2020

View reviewed changes

zeling595 and others added 13 commits September 1, 2020 20:09

add GUI fxml, before integrate GUI to duke

bb7566f

added GUI for Duke

1b0f7f9

modified ui

0ff187d

added assertions

2203b7a

make changes in accordance to coding quality

28c4250

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

6f12b6a

added assertions

Merge branch 'master' into branch-A-CodeQuality

41a617d

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

bde1dff

make changes in accordance to coding quality

added the functionality to update task description, task date and tas…

5b871d4

…k status.

a better GUI

4846cec

add a user guide

edf1876

adjust ui and checkstyle

d530aec

update testcases name

d70cfbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Long Zeling] iP #235

[Long Zeling] iP #235

zeling595 commented Aug 23, 2020

PhongTran98 left a comment

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

PhongTran98 Aug 31, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 left a comment

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

UncleGrandpa925 Sep 1, 2020

[Long Zeling] iP #235

Are you sure you want to change the base?

[Long Zeling] iP #235

Conversation

zeling595 commented Aug 23, 2020

PhongTran98 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

UncleGrandpa925 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