-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Varun] iP #198
base: master
Are you sure you want to change the base?
[Varun] iP #198
Changes from 11 commits
4a2729a
b1017c9
b8e223e
3d06235
012b539
e4341c7
965a166
b1c2436
82d95e3
afaf67b
f0e08de
152cfe6
a7b694f
8460d3f
f7d89c2
d97147d
1cb044c
7599173
4d48ad1
ec66276
2072337
b0e98c2
ee08d39
603a718
ea0d5eb
7753232
133254d
8eac149
59cc549
4cdedaa
4e4d246
dd2db1b
29af211
4a13fca
70533ff
881d2d4
371f144
b50c692
5f4d0b3
ffcec7a
53714ec
ee1cd58
a87690f
244f594
d326977
a067104
dc92544
97b015f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
public class Deadline extends Task{ | ||
private static String SYMBOL = "D"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this should be a final variable as I presume the SYMBOL is supposed to be a fixed constant There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @arvejw is correct, as seen in the coding standards or code quality guidelines. The |
||
private String dueDate; | ||
|
||
public Deadline(String name, String dueDate) { | ||
super(name); | ||
this.dueDate = dueDate; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "[" + SYMBOL + "]" + super.toString() + " (by: " + dueDate + ")"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,58 @@ | ||||||||||||||||||||||||||
import java.util.Scanner; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public class Duke { | ||||||||||||||||||||||||||
public static String EXIT_CMD = "bye"; | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good use of constants. |
||||||||||||||||||||||||||
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"; | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps these commands being grouped together could have a common prefix? Following coding standards:
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public static void main(String[] args) { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this method is too long? You can consider putting the logic in a separate class. |
||||||||||||||||||||||||||
String logo = " ____ _ \n" | ||||||||||||||||||||||||||
+ "| _ \\ _ _| | _____ \n" | ||||||||||||||||||||||||||
+ "| | | | | | | |/ / _ \\\n" | ||||||||||||||||||||||||||
+ "| |_| | |_| | < __/\n" | ||||||||||||||||||||||||||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||||||||||||||||||||||||||
System.out.println("Hello from\n" + logo); | ||||||||||||||||||||||||||
greetUserOnStart(); | ||||||||||||||||||||||||||
Scanner input = new Scanner(System.in); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
while (true) { | ||||||||||||||||||||||||||
String command = input.nextLine(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// parse and handle commands | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think you can put this logic in a different method? |
||||||||||||||||||||||||||
if (command.equals(EXIT_CMD)) { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the use of constants here! Makes it clear and easy to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||
} else if (command.equals(LIST_CMD)) { | ||||||||||||||||||||||||||
TaskManager.listTasks(); | ||||||||||||||||||||||||||
} else if (command.startsWith(DONE_CMD)) { | ||||||||||||||||||||||||||
String parsedInput = command.split(" ")[1]; | ||||||||||||||||||||||||||
int taskNo = Integer.parseInt(parsedInput); | ||||||||||||||||||||||||||
TaskManager.markTaskNoAsDone(taskNo - 1); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you might consider extracting out the execution of the commands into their own separate method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Variable could be better named for less ambiguity. Overall good adherence to coding standard! |
||||||||||||||||||||||||||
} else if (command.startsWith(TODO_CMD)) { | ||||||||||||||||||||||||||
String parsedInput = command.replaceFirst(TODO_CMD, ""); | ||||||||||||||||||||||||||
String todo = parsedInput.strip(); | ||||||||||||||||||||||||||
TaskManager.addTodo(todo); | ||||||||||||||||||||||||||
} else if (command.startsWith(DEADLINE_CMD)) { | ||||||||||||||||||||||||||
String [] parsedInput = command.replaceFirst(DEADLINE_CMD, "").split("/by "); | ||||||||||||||||||||||||||
String deadlineTitle = parsedInput[0].strip(); | ||||||||||||||||||||||||||
String deadlineDue = parsedInput[1].strip(); | ||||||||||||||||||||||||||
TaskManager.addDeadline(deadlineTitle, deadlineDue); | ||||||||||||||||||||||||||
} else if (command.startsWith(EVENT_CMD)) { | ||||||||||||||||||||||||||
String [] parsedInput = command.replaceFirst(EVENT_CMD, "").split("/at "); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Plural form should be used on names representing a collection of objects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yzhedwin is correct, following coding standards. |
||||||||||||||||||||||||||
String eventTitle = parsedInput[0].strip(); | ||||||||||||||||||||||||||
String eventTime = parsedInput[1].strip(); | ||||||||||||||||||||||||||
TaskManager.addEvent(eventTitle, eventTime); | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
// handle invalid command | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good indentation of comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is helpful :) |
||||||||||||||||||||||||||
System.out.println("Invalid command! Please enter a valid command"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe you could try to refactor the code to avoid long methods( above 30 LOC) |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
greetUserOnEnd(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public static void greetUserOnStart() { | ||||||||||||||||||||||||||
System.out.println("Hello! I'm Duke"); | ||||||||||||||||||||||||||
System.out.println("What can I do for you?"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public static void greetUserOnEnd() { | ||||||||||||||||||||||||||
System.out.println("Bye. Hope to see you again soon!"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,14 @@ | ||||||
public class Event extends Task{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
private static String SYMBOL = "E"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the name be more descriptive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to previous comments about constants, you can consider making this a final variable as its value should not be changing |
||||||
private String timeslot; | ||||||
|
||||||
public Event(String name, String timeslot) { | ||||||
super(name); | ||||||
this.timeslot = timeslot; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public String toString() { | ||||||
return "[" + SYMBOL + "]" + super.toString() + " (at: " + timeslot + ")"; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,26 @@ | ||||||
public class Task { | ||||||
final private String name; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the modifiers could be in the standard order? Separately, you may also want to have a
Suggested change
|
||||||
private Boolean isDone; | ||||||
|
||||||
public Task(String name) { | ||||||
this.name = name; | ||||||
this.isDone = false; | ||||||
} | ||||||
|
||||||
public String getName() { | ||||||
return name; | ||||||
} | ||||||
|
||||||
public String getStatusIcon() { | ||||||
return (isDone ? "X" : " "); | ||||||
} | ||||||
|
||||||
public void markAsDone() { | ||||||
isDone = true; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public String toString() { | ||||||
return "[" + getStatusIcon() + "] " + getName(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,47 @@ | ||||||
public class TaskManager { | ||||||
private static int taskNo = 0; | ||||||
final private static int maxTasks = 100; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a more informative name would be maxTasksNo to keep in line with taskNo, or name those two constants such that they sound similar There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this should be uppercase, as it is a constant?
Suggested change
|
||||||
final static Task[] tasks = new Task[maxTasks]; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this should be
Suggested change
|
||||||
|
||||||
static void addTask(Task task) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider updating the modifiers for the methods in this class. The default scope of methods in Java is package-private. Having them be |
||||||
if (taskNo < maxTasks) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can consider making the happy path prominent here. |
||||||
tasks[taskNo] = task; | ||||||
taskNo++; | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think don't really need a new line |
||||||
System.out.print("Got it. I've added this task:\n"); | ||||||
System.out.printf(" %s\n", task); | ||||||
System.out.printf("Now you have %d tasks in the list.\n", taskNo); | ||||||
} | ||||||
} | ||||||
|
||||||
static void addTodo(String todoName) { | ||||||
Todo todo = new Todo(todoName); | ||||||
addTask(todo); | ||||||
} | ||||||
|
||||||
static void addDeadline(String deadlineName, String deadlineDue) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
Deadline deadline = new Deadline(deadlineName, deadlineDue); | ||||||
addTask(deadline); | ||||||
} | ||||||
|
||||||
static void addEvent(String eventName, String eventTime) { | ||||||
Event event = new Event(eventName, eventTime); | ||||||
addTask(event); | ||||||
} | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good use of refactoring to split the subtasks creation into 3 functions |
||||||
static void listTasks() { | ||||||
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); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not too big a deal (
Suggested change
|
||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should stick to using printf or println not both as it is confusing |
||||||
} | ||||||
|
||||||
static void markTaskNoAsDone(int taskNo) { | ||||||
Task task = tasks[taskNo]; | ||||||
task.markAsDone(); | ||||||
|
||||||
System.out.println("Nice! I've marked this task as done:"); | ||||||
System.out.printf(" %s\n", task); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
public class Todo extends Task { | ||
private static String SYMBOL = "T"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think it is a constant name , just a variable name but im not sure |
||
|
||
public Todo(String name) { | ||
super(name); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "[" + SYMBOL + "]" + super.toString(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there should be a space before the bracket? Similar for other classes.