[John Nicholas S] iP by johnsuharjono · Pull Request #88 · nus-cs2113-AY2122S2/ip

johnsuharjono · 2022-02-03T19:39:29Z

No description provided.

mafpovbul · 2022-02-04T02:35:09Z

+public class Brave {
+    public static void main(String[] args) {
+        String input;
+        String[] splitInput;


Plural form should be used on names representing a collection of objects - maybe use splitInputs instead?

mafpovbul · 2022-02-04T02:36:45Z

+                tasks.addTask(new Event(description, eventTime));
+                break;
+            default:
+                System.out.println("Wrong input, available command are -> list/mark/unmark/todo/deadline/event");


Nice catch for unknown commands!

mafpovbul · 2022-02-04T02:37:06Z

+        this.by = by;
+    }
+
+    @Override


Good note on method overriding!

mafpovbul · 2022-02-04T02:37:44Z

+public class Event extends Task {
+    private String eventTime;
+
+    public Event(String description, String eventTime) {
+        super(description);
+        this.eventTime = eventTime;
+    }
+
+    @Override
+    public String getStatusIcon() {
+        return "[E]" + super.getStatusIcon();
+    }
+
+    public String getDescription() {
+        return super.getDescription() + String.format(" (at: %s)", this.eventTime);
+    }
+}


Well done, I don't think there's any violations here :-)

mafpovbul · 2022-02-04T02:39:59Z

+
+

Should this be 1 newline?

wli-linda

Overall good quality code, nice work! I like your way of getting rid of magic numbers

wli-linda · 2022-02-04T02:52:25Z

+            switch (command) {
+            case "list":
+                tasks.printTaskList();
+                break;
+            case "mark":
+                tasks.markTask(Integer.parseInt(splitInput[1]) - 1); // 0 indexing
+                break;
+            case "unmark":
+                tasks.unmarkTask(Integer.parseInt(splitInput[1]) - 1); // 0 indexing
+                break;
+            case "todo":
+                tasks.addTask(new Todo(splitInput[1]));
+                break;
+            case "deadline":
+                // To-do validate arguments~
+                arguments = splitInput[1].split(" /by ", 2);
+                description = arguments[0];
+                String by = arguments[1];
+                tasks.addTask(new Deadline(description, by));
+                break;
+            case "event":


The main method is a little long, maybe it'd be nice to extract this portion as a separate method?

wli-linda · 2022-02-04T02:53:11Z

+    public void addTask(Task description) {
+        this.tasks[this.tasksCount] = description;
+        System.out.println("\t____________________________________________________________");
+        System.out.println("\tYepp, I have added this task\n\t" + this.tasks[this.tasksCount].description + "\n\tYou currently have " + (this.tasksCount + 1) + " task in the list");


Consider splitting this line so it's not so long?

wli-linda · 2022-02-04T02:54:22Z

@@ -0,0 +1,17 @@
+public class Deadline extends Task {


Nice use of class inheritance

wli-linda · 2022-02-04T02:55:50Z

+    public static final int MAX_TASK = 100;
+    private final Task[] tasks = new Task[MAX_TASK];


Good way of getting rid of magic numbers!

wli-linda · 2022-02-04T02:56:39Z

+    @Override
+    public String getStatusIcon() {
+        return "[E]" + super.getStatusIcon();
+    }


Good method overriding :)

WrinklyToad

I only see one naming convention you could consider correcting. The readability of your code is quite nice; I appreciate how concise your 'Brave.java' is, and all of your other classes are very understandable. I added a few notes on things I thought were well done, and a few suggestions such as Javadoc comments.

WrinklyToad · 2022-02-04T02:57:04Z

@@ -0,0 +1,44 @@
+public class TaskManager {


I think the inclusion of a TaskManager class is a very good idea; it makes the code in 'Main' much more concise

WrinklyToad · 2022-02-04T02:58:18Z

+    public void addTask(Task description) {
+        this.tasks[this.tasksCount] = description;
+        System.out.println("\t____________________________________________________________");
+        System.out.println("\tYepp, I have added this task\n\t" + this.tasks[this.tasksCount].description + "\n\tYou currently have " + (this.tasksCount + 1) + " task in the list");


The max line length is 120 characters; maybe you could split this between two lines?

WrinklyToad · 2022-02-04T02:59:45Z

+    public static final int MAX_TASK = 100;
+    private final Task[] tasks = new Task[MAX_TASK];


Good job including a MAX_TASK constant for this; it is a common mistake to forget it (I need to fix this in my code)

WrinklyToad · 2022-02-04T03:04:48Z

+public class Task {
+    protected String description;
+    protected boolean isDone;
+
+    public Task(String description) {
+        this.description = description;
+        this.isDone = false;
+    }
+
+    public String getStatusIcon() {
+        return (isDone ? "[X]" : "[ ]"); // mark done task with X
+    }
+
+    public String getDescription() {
+        return description;
+    }
+
+    public void markAsDone() {
+        isDone = true;
+    }
+
+    public void unmarkAsDone() {
+        isDone = false;
+    }
+}


I think your Task class and all the classes that inherit from it are coded very well. Since Task has so many child classes, you could consider Javadoc comments for convention's sake.

WrinklyToad · 2022-02-04T03:06:00Z

+public class Brave {
+    public static void main(String[] args) {
+        String input;
+        String[] splitInput;


I agree with mafpovul; perhaps a plural would be better here?

LI-XINYI1 · 2022-02-04T02:54:07Z

+    public void unmarkTask(int taskIndex) {
        this.tasks[taskIndex].unmarkAsDone();
        System.out.println("OK, I've marked this task as not done yet:");
        System.out.println(this.tasks[taskIndex].getStatusIcon() + " " + this.tasks[taskIndex].getDescription());


The format is generally great and neatly following the standards. Can change lines when the text to print is too long.

LI-XINYI1 · 2022-02-04T02:58:56Z

+
        System.out.println("____________________________________________________________");
        System.out.println("Bye, Hope to see you again soon!");
        System.out.println("____________________________________________________________");


The format is clear and neat! Maybe you can use a string object to store the seperate line, then no need to type the line every time( can secure same length)

LI-XINYI1 · 2022-02-04T03:11:35Z

-        for (int i=0; i<tasksCount; i++) {
-            System.out.println(String.format("\t%d.%s %s", i+1,this.tasks[i].getStatusIcon(), this.tasks[i].getDescription() ));
+        for (int i = 0; i < tasksCount; i++) {
+            System.out.println(String.format("\t%d.%s %s", i + 1, this.tasks[i].getStatusIcon(), this.tasks[i].getDescription()));


Nice changes to add spaces between operation cahracters.

LI-XINYI1 · 2022-02-04T03:14:29Z

                break;
            case "unmark":
-                tasks.unmarkTask(Integer.parseInt(splitInput[1]) - 1);
+                tasks.unmarkTask(Integer.parseInt(splitInput[1]) - 1); // 0 indexing


Good to see more comments. It's clear for the readers.

kelvneo · 2022-02-19T18:03:13Z

+    public static void main(String[] args) {
+        String input;
+        String[] splitInputs;
+        String command;
+        String[] arguments;
+        String description;
+        Scanner in = new Scanner(System.in);
+        TaskManager tasks = new TaskManager();
+        try {
+            tasks.initialiseTasks("data/brave.txt");
+        } catch (FileNotFoundException e) {
+            System.out.println("File not found");
+        }
+
+        tasks.showWelcomeMessage();
+        while (true) {
+            input = in.nextLine();
+            splitInputs = input.split(" ", 2);
+            command = splitInputs[0]; //e.g. mark 2 -> take the first word as the command -> "mark"
+
+            if (command.equals("bye")) {
+                tasks.showFarewellMessage();
+                tasks.saveTask("data/brave.txt");
+                break;
+            }
+
+            switch (command) {
+            case "list":
+                tasks.printTaskList();
+                break;
+            case "mark":
+                try {
+                    tasks.markTask(Integer.parseInt(splitInputs[1]) - 1); // 0 indexing
+                } catch (NumberFormatException e) {
+                    System.out.println("Please put in integer value");
+                } catch (NullPointerException | ArrayIndexOutOfBoundsException e) {
+                    System.out.println("Please put in valid number of task");
+                }
+                break;
+            case "unmark":
+                try {
+                    tasks.unmarkTask(Integer.parseInt(splitInputs[1]) - 1); // 0 indexing
+                } catch (NumberFormatException e) {
+                    System.out.println("Please put in integer value");
+                } catch (NullPointerException | ArrayIndexOutOfBoundsException e) {
+                    System.out.println("Please put in valid number of task");
+                }
+                break;
+            case "todo":
+                try {
+                    description = splitInputs[1];
+                    tasks.addTask(new Todo(description));
+                } catch (IndexOutOfBoundsException e) {
+                    System.out.println(" ☹ OOPS!!! The description of a todo cannot be empty.");
+                }
+                break;
+            case "deadline":
+                // To-do validate arguments~
+                arguments = splitInputs[1].split(" /by ", 2);
+                description = arguments[0];
+                String by = arguments[1];
+                tasks.addTask(new Deadline(description, by));
+                break;
+            case "event":
+                // To-do validate arguments~
+                arguments = splitInputs[1].split(" /at ", 2);
+                description = arguments[0];
+                String eventTime = arguments[1];
+                tasks.addTask(new Event(description, eventTime));
+                break;
+            case "delete":
+                try {
+                    tasks.deleteTask(Integer.parseInt(splitInputs[1]) - 1); // 0 indexing
+                } catch (NumberFormatException e) {
+                    System.out.println("Please put in integer value");
+                } catch (NullPointerException | ArrayIndexOutOfBoundsException e) {
+                    System.out.println("Please put in valid number of task");
+                }
+                break;
+            default:
+                try {
+                    throw new IllegalArgumentException();
+                } catch (IllegalArgumentException e) {
+                    System.out.println("☹ OOPS!!! I'm sorry, but I don't know what that means :-(");
+                    System.out.println("Available command are -> list/mark/unmark/todo/deadline/event");
+                }
+                break;
+            }
+        }
+    }
+}


The method is too long, perhaps you might want to SLAP it.

kelvneo · 2022-02-19T18:05:51Z

+                if (details[1].equals("1")) {
+                    selected_task = tasks.get(taskIndex);
+                    selected_task.markAsDone();
+                }


Repetative code, perhaps you can find a way to code it once only?

kelvneo · 2022-02-19T18:07:59Z

+        Scanner in = new Scanner(System.in);
+        TaskManager tasks = new TaskManager();
+        try {
+            tasks.initialiseTasks("data/brave.txt");


Magic literal here, you might want to place it as a static constant.

kelvneo · 2022-02-19T18:11:32Z

+    //    public static final int MAX_TASK = 100;
+    public static final String LINE_SPLIT = "\t____________________________________________________________";
+    private final ArrayList<Task> tasks = new ArrayList<>();
+//    private final Task[] tasks = new Task[MAX_TASK];
+//    private int tasksCount = 0;


Remember to remove unneccesary comments before you commit

kelvneo · 2022-02-19T18:16:58Z

+                try {
+                    throw new IllegalArgumentException();
+                } catch (IllegalArgumentException e) {
+                    System.out.println("☹ OOPS!!! I'm sorry, but I don't know what that means :-(");
+                    System.out.println("Available command are -> list/mark/unmark/todo/deadline/event");
+                }
+                break;


I don't think you need to throw a new IllegalArgumentException here, just printing will do.

Add Ui Class Add Storage Class Add Parser Class

Add find method

Implement PathOf to get Directory Path

Added JavaDoc

…ory or file

johnsuharjono added 9 commits January 27, 2022 17:17

initialize project

edede19

week 3 part 1

25d1b25

Week 3 Part 2

28ad6a1

Week 3 Part 3

8818d7e

Week 3 Coding Standard

d6193fc

Add TaskManager Class

e835c55

Add TaskManager class

5cf003e

Level 4. Add ToDos, Events, Deadlines Class

dca4adb

Improve code quality, add constant

6b84407

mafpovbul reviewed Feb 4, 2022

View reviewed changes

wli-linda reviewed Feb 4, 2022

View reviewed changes

WrinklyToad reviewed Feb 4, 2022

View reviewed changes

LI-XINYI1 reviewed Feb 4, 2022

View reviewed changes

johnsuharjono added 11 commits February 10, 2022 20:14

improve code quality

cb38ba5

Adding exception for mark, unmark, todo, and invalid command

0852ee7

create package

a235cc3

Merge branch 'branch-A-Packages'

468c600

change array to ArrayList

f3bf057

Add delete function

2338c4c

add data/brave.txt

2656fb6

Add file reading

7947f40

Merge branch 'branch-Level-6'

0aa5613

Merge branch 'branch-Level-7'

301e9b0

Merge level 6 and level 7, finish read and write file

b6b4efb

kelvneo reviewed Feb 19, 2022

View reviewed changes

johnsuharjono and others added 5 commits March 3, 2022 02:55

delete some comments

acb94a4

Create more class

612bddc

Add Ui Class Add Storage Class Add Parser Class

Add find method

6e125a8

Merge pull request #1 from johnsuharjono/branch-Level-9

80ecdee

Add find method

Merge branch 'master' of https://github.com/johnsuharjono/ip

60b0564

johnsuharjono and others added 10 commits March 4, 2022 00:47

Added JavaDoc

ed140e7

Implement PathOf to get Directory Path

Merge pull request #2 from johnsuharjono/branch-javaDoc

5c02612

Added JavaDoc

Merge branch 'master' of https://github.com/johnsuharjono/ip

efb6d5e

Add User Guide

46a7af1

Edit User Guide and UI Message

29771b5

no message

351848e

Set theme jekyll-theme-cayman

1b86e4d

Handle case when file or directory not exist by creating a new direct…

cb35164

…ory or file

Merge branch 'master' of https://github.com/johnsuharjono/ip

8fb5a28

edit user guide

657eb5a

		public static final int MAX_TASK = 100;
		private final Task[] tasks = new Task[MAX_TASK];

Uh oh!

Conversation

johnsuharjono commented Feb 3, 2022

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!

wli-linda 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!

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

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!

6 participants