[Varun] iP #198

flerovious · 2021-09-02T11:51:59Z

No description provided.

Add support for tracking todos, deadlines and events

astralum

Excellent work! After some modifications, it'll become even more readable and clear :)

astralum · 2021-09-03T04:56:53Z

src/main/java/Event.java

@@ -0,0 +1,14 @@
+public class Event extends Task{
+    private static String SYMBOL = "E";


Could the name be more descriptive?

astralum · 2021-09-03T04:58:08Z

src/main/java/Duke.java

+        while (true) {
+            String command = input.nextLine();
+
+            // parse and handle commands


Do you think you can put this logic in a different method?

astralum · 2021-09-03T04:59:53Z

src/main/java/Duke.java

+                String eventTime = parsedInput[1].strip();
+                TaskManager.addEvent(eventTitle, eventTime);
+            } else {
+                // handle invalid command


This comment is helpful :)

astralum · 2021-09-03T05:02:05Z

src/main/java/TaskManager.java

@@ -0,0 +1,47 @@
+public class TaskManager {
+    private static int taskNo = 0;
+    final private static int maxTasks = 100;


Perhaps a more informative name would be maxTasksNo to keep in line with taskNo, or name those two constants such that they sound similar

astralum · 2021-09-03T05:03:30Z

src/main/java/TaskManager.java

+        addTask(todo);
+    }
+
+    static void addDeadline(String deadlineName, String deadlineDue) {


Perhaps for the second parameter, you can use the same name as in Deadline.java (in this case it is dueDate), or a similar one in style with deadlineName

astralum · 2021-09-03T05:05:29Z

src/main/java/Duke.java

+            String command = input.nextLine();
+
+            // parse and handle commands
+            if (command.equals(EXIT_CMD)) {


I like the use of constants here! Makes it clear and easy to change

xingyuan123 · 2021-09-03T05:06:32Z

src/main/java/Duke.java

+                // handle invalid command
+                System.out.println("Invalid command! Please enter a valid command");
+            }
+        }


maybe you could try to refactor the code to avoid long methods( above 30 LOC)

xingyuan123 · 2021-09-03T05:12:58Z

src/main/java/TaskManager.java

+        Event event = new Event(eventName, eventTime);
+        addTask(event);
+    }
+


good use of refactoring to split the subtasks creation into 3 functions

xingyuan123 · 2021-09-03T05:18:40Z

src/main/java/Todo.java

@@ -0,0 +1,12 @@
+public class Todo extends Task {
+    private static String SYMBOL = "T";


i dont think it is a constant name , just a variable name but im not sure

xingyuan123 · 2021-09-03T05:24:38Z

src/main/java/TaskManager.java

+        for (int i = 0; i < taskNo; ++i) {
+            Task task = tasks[i];
+            System.out.printf("%d. %s\n", i + 1, task);
+        }


should stick to using printf or println not both as it is confusing

arvejw

Overall it is a good implementation. Basically whether certain variables are constants is vague as capitalized variable name implies a constant which should be set as a final variable

arvejw · 2021-09-03T06:30:18Z

src/main/java/Duke.java

+    public static String EXIT_CMD = "bye";
+    public static String LIST_CMD = "list";
+    public static String DONE_CMD = "done";
+    public static String TODO_CMD = "todo";
+    public static String DEADLINE_CMD = "deadline";
+    public static String EVENT_CMD = "event";


Perhaps you should make these variables final variables as well. From the naming convention used I am assuming you intend for these to be constants, but I may be misunderstanding something.

arvejw · 2021-09-03T06:32:19Z

src/main/java/Deadline.java

@@ -0,0 +1,14 @@
+public class Deadline extends Task{
+    private static String SYMBOL = "D";


Perhaps this should be a final variable as I presume the SYMBOL is supposed to be a fixed constant

@arvejw is correct, as seen in the coding standards or code quality guidelines. The final keyword is to indicate that the String SYMBOL will never change. You can take a look at all constants and make sure they adhere to this.

arvejw · 2021-09-03T06:34:29Z

src/main/java/Duke.java

+            String command = input.nextLine();
+
+            // parse and handle commands
+            if (command.equals(EXIT_CMD)) {


As pointed out by others, very nice intuitive way of managing the various commands as the reader easily understands what command each code block carries out

arvejw · 2021-09-03T06:36:21Z

src/main/java/Duke.java

+                String parsedInput = command.split(" ")[1];
+                int taskNo = Integer.parseInt(parsedInput);
+                TaskManager.markTaskNoAsDone(taskNo - 1);


Perhaps you might consider extracting out the execution of the commands into their own separate method

arvejw · 2021-09-03T06:42:40Z

src/main/java/Event.java

@@ -0,0 +1,14 @@
+public class Event extends Task{
+    private static String SYMBOL = "E";


Similar to previous comments about constants, you can consider making this a final variable as its value should not be changing

yzhedwin · 2021-09-03T03:56:33Z

src/main/java/Duke.java

+                String eventTime = parsedInput[1].strip();
+                TaskManager.addEvent(eventTitle, eventTime);
+            } else {
+                // handle invalid command


good indentation of comment

yzhedwin · 2021-09-03T13:44:46Z

src/main/java/Duke.java

 public class Duke {
+    public static String EXIT_CMD = "bye";


Good use of constants.

yzhedwin · 2021-09-03T13:47:08Z

src/main/java/Duke.java

+                String deadlineDue = parsedInput[1].strip();
+                TaskManager.addDeadline(deadlineTitle, deadlineDue);
+            } else if (command.startsWith(EVENT_CMD)) {
+                String [] parsedInput = command.replaceFirst(EVENT_CMD, "").split("/at ");


Plural form should be used on names representing a collection of objects.

@yzhedwin is correct, following coding standards.

yzhedwin · 2021-09-03T13:48:24Z

src/main/java/TaskManager.java

+        if (taskNo < maxTasks) {
+            tasks[taskNo] = task;
+            taskNo++;
+


I think don't really need a new line

yzhedwin · 2021-09-03T13:50:47Z

src/main/java/Duke.java

+            } else if (command.startsWith(DONE_CMD)) {
+                String parsedInput = command.split(" ")[1];
+                int taskNo = Integer.parseInt(parsedInput);
+                TaskManager.markTaskNoAsDone(taskNo - 1);


Variable could be better named for less ambiguity. Overall good adherence to coding standard!

hughjazzman

Do complete the week's tasks as soon as possible. 👍

hughjazzman · 2021-09-19T07:34:47Z

src/main/java/Deadline.java

@@ -0,0 +1,14 @@
+public class Deadline extends Task{
+    private static String SYMBOL = "D";


@arvejw is correct, as seen in the coding standards or code quality guidelines. The final keyword is to indicate that the String SYMBOL will never change. You can take a look at all constants and make sure they adhere to this.

hughjazzman · 2021-09-19T07:35:47Z

src/main/java/Deadline.java

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


Perhaps there should be a space before the bracket? Similar for other classes.

Suggested change

public class Deadline extends Task{

public class Deadline extends Task {

hughjazzman · 2021-09-19T07:35:52Z

src/main/java/AddTaskException.java

@@ -0,0 +1,2 @@
+public class AddTaskException extends DukeException{


Suggested change

public class AddTaskException extends DukeException{

public class AddTaskException extends DukeException {

hughjazzman · 2021-09-19T07:38:41Z

src/main/java/Duke.java

+    public static String EXIT_CMD = "bye";
+    public static String LIST_CMD = "list";
+    public static String DONE_CMD = "done";
+    public static String TODO_CMD = "todo";
+    public static String DEADLINE_CMD = "deadline";
+    public static String EVENT_CMD = "event";


Perhaps these commands being grouped together could have a common prefix? Following coding standards:

Associated constants should have a common prefix.

Suggested change

public static String EXIT_CMD = "bye";

public static String LIST_CMD = "list";

public static String DONE_CMD = "done";

public static String TODO_CMD = "todo";

public static String DEADLINE_CMD = "deadline";

public static String EVENT_CMD = "event";

public static final String CMD_EXIT = "bye";

public static final String CMD_LIST = "list";

public static final String CMD_DONE = "done";

public static final String CMD_TODO = "todo";

public static final String CMD_DEADLINE = "deadline";

public static final String CMD_EVENT = "event";

hughjazzman · 2021-09-19T07:39:04Z

src/main/java/DeadlineException.java

@@ -0,0 +1,2 @@
+public class DeadlineException extends DukeException{


Suggested change

public class DeadlineException extends DukeException{

public class DeadlineException extends DukeException {

hughjazzman · 2021-09-19T07:53:41Z

src/main/java/TaskManager.java

+public class TaskManager {
+    private static int taskNo = 0;
+    final private static int maxTasks = 100;
+    final static Task[] tasks = new Task[maxTasks];


Perhaps this should be private, following the encapsulation paradigm of OOP?

Suggested change

final static Task[] tasks = new Task[maxTasks];

private static final Task[] tasks = new Task[MAX_TASKS];

hughjazzman · 2021-09-19T07:57:28Z

src/main/java/TaskManager.java

+    final private static int maxTasks = 100;
+    final static Task[] tasks = new Task[maxTasks];
+
+    static void addTask(Task task) {


Consider updating the modifiers for the methods in this class. The default scope of methods in Java is package-private. Having them be static may not be advantageous as well, which you may learn later on.

hughjazzman · 2021-09-19T08:03:30Z

src/main/java/TaskManager.java

+        System.out.println("Here are the tasks in your list:");
+        for (int i = 0; i < taskNo; ++i) {
+            Task task = tasks[i];
+            System.out.printf("%d. %s\n", i + 1, task);


This is not too big a deal (printf vs println), though you can consider simply sticking to print (not printf) as Java has its own handy toString methods that can convert Integer for you. I believe printf may be a habit from C.

Suggested change

System.out.printf("%d. %s\n", i + 1, task);

System.out.println((i + 1) + ". " + task);

hughjazzman · 2021-09-19T08:03:59Z

src/main/java/TodoException.java

@@ -0,0 +1,2 @@
+public class TodoException extends DukeException{


Suggested change

public class TodoException extends DukeException{

public class TodoException extends DukeException {

hughjazzman · 2021-09-19T08:07:17Z

src/main/java/TaskManager.java

+    final static Task[] tasks = new Task[maxTasks];
+
+    static void addTask(Task task) {
+        if (taskNo < maxTasks) {


You can consider making the happy path prominent here.

# Conflicts: # src/main/java/duke/TaskManager.java

Add message parameter for InvalidCommand to throw DukeException with specified error message

Add LocalDate to Deadline and Event

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

Add Find command

# Conflicts: # src/main/java/duke/task/Task.java

Update JavaDoc

21varun12 added 11 commits August 26, 2021 18:28

Add Greet

4a2729a

Add Echo

b1017c9

Add add, list

b8e223e

Add mark as done

3d06235

Comply with coding standard

012b539

Add todo

e4341c7

Refactor code

965a166

Add deadline

b1c2436

Add event

82d95e3

Update TaskManager

afaf67b

Add support for tracking todos, deadlines and events

Refactor code

f0e08de

astralum reviewed Sep 3, 2021

View reviewed changes

xingyuan123 reviewed Sep 3, 2021

View reviewed changes

arvejw reviewed Sep 3, 2021

View reviewed changes

yzhedwin reviewed Sep 3, 2021

View reviewed changes

21varun12 added 3 commits September 9, 2021 23:38

Add exception classes

152cfe6

Add exception handling

a7b694f

Merge branch 'branch-Level-5'

8460d3f

hughjazzman reviewed Sep 19, 2021

View reviewed changes

flerovious added 11 commits September 19, 2021 20:15

Refactor commands

f7d89c2

Refactor with packages

d97147d

Add delete command

1cb044c

Add save to local storage feature

7599173

Merge remote-tracking branch 'origin/branch-Level-6'

4d48ad1

Merge remote-tracking branch 'origin/branch-Level-7'

ec66276

# Conflicts: # src/main/java/duke/TaskManager.java

Fix bugs with delete command

2072337

Add JavaDocs

b0e98c2

Add Ui class for user interactions

ee08d39

Refactor TaskList

603a718

Refactor Parser

ea0d5eb

flerovious added 23 commits October 1, 2021 01:54

Add LocalDate to Deadline and Event

7753232

Update InvalidCommand

133254d

Add message parameter for InvalidCommand to throw DukeException with specified error message

Add find command

8eac149

Merge pull request #1 from flerovious/branch-Level-8

59cc549

Add LocalDate to Deadline and Event

Merge branch 'master' into branch-Level-9

4cdedaa

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

Fix import bug

4e4d246

Merge pull request #2 from flerovious/branch-Level-9

dd2db1b

Add Find command

Add JavaDocs

29af211

Refactor code to follow coding standard

4a13fca

Add JavaDoc for Task

70533ff

Merge branch 'master' into branch-A-JavaDoc

881d2d4

# Conflicts: # src/main/java/duke/task/Task.java

Merge pull request #3 from flerovious/branch-A-JavaDoc

371f144

Update JavaDoc

Add help command

b50c692

Update User Guide

5f4d0b3

Set theme jekyll-theme-cayman

ffcec7a

Set theme jekyll-theme-slate

53714ec

Set theme jekyll-theme-cayman

ee1cd58

Set theme jekyll-theme-hacker

a87690f

Set theme jekyll-theme-cayman

244f594

Set theme jekyll-theme-minimal

d326977

Fix table rendering

a067104

Fix table rendering

dc92544

Set theme jekyll-theme-cayman

97b015f

		@@ -0,0 +1,14 @@
		public class Event extends Task{
		private static String SYMBOL = "E";

		@@ -0,0 +1,12 @@
		public class Todo extends Task {
		private static String SYMBOL = "T";

		@@ -0,0 +1,14 @@
		public class Deadline extends Task{
		private static String SYMBOL = "D";

	public class Deadline extends Task{
	public class Deadline extends Task {

		@@ -0,0 +1,2 @@
		public class AddTaskException extends DukeException{

		@@ -0,0 +1,2 @@
		public class DeadlineException extends DukeException{

	final static Task[] tasks = new Task[maxTasks];
	private static final Task[] tasks = new Task[MAX_TASKS];

	System.out.printf("%d. %s\n", i + 1, task);
	System.out.println((i + 1) + ". " + task);

		@@ -0,0 +1,2 @@
		public class TodoException extends DukeException{

[Varun] iP #198

Are you sure you want to change the base?

[Varun] iP #198

Conversation

flerovious commented Sep 2, 2021

astralum 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

arvejw 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

arvejw Sep 3, 2021 • edited Loading

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

hughjazzman 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

arvejw Sep 3, 2021 •

edited

Loading