-
Notifications
You must be signed in to change notification settings - Fork 270
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
[Tham Jin Lin] iP #288
base: master
Are you sure you want to change the base?
[Tham Jin Lin] iP #288
Conversation
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.
Generally looks good except just some naming to be resolved.
src/main/java/Deadlines.java
Outdated
@@ -0,0 +1,23 @@ | |||
public class Deadlines extends Task { |
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 the class name could be singular?
I like that you used inheritance for different types of Task.
src/main/java/Deadlines.java
Outdated
@@ -0,0 +1,23 @@ | |||
public class Deadlines extends Task { | |||
protected static String type = "[D]"; |
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.
Would it be better for the type to be static final and capitalized as in the coding standards for constants since this type won't be changing?
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.
Agreed
src/main/java/DukeTest.java
Outdated
|
||
exit(); | ||
scanner.close(); | ||
} catch (DukeException e) { |
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.
I like that you considered the many potential errors in the user inputs 😄
src/main/java/DukeTest.java
Outdated
|
||
while (!(isExit(command))) { | ||
switch (command) { | ||
case "list": |
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.
Just a reminder that the case could be indented in the same line as the switch command as in the coding standards
src/main/java/DukeTest.java
Outdated
String command = splitInput[0]; | ||
|
||
while (!(isExit(command))) { | ||
switch (command) { |
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.
Great that you used switch case and have modularized your code to perform the respective actions for the different conditions, looks neat!
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.
Looks good other than a few little details that should be easy to fix. Comments are missing.
src/main/java/Deadlines.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return this.isDone ? "[D][X] " + this.description |
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 you could remove the 'this' keywords since the variables are not shadowed
src/main/java/DukeTest.java
Outdated
private static void addTask(Task task) { | ||
taskList.add(task); | ||
System.out.println("Got it. I've added this task:\n" + task.toString() + | ||
"\nNow you have " + taskList.size() + " tasks in the list."); |
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.
Proper indentation 👍
src/main/java/Deadlines.java
Outdated
@@ -0,0 +1,23 @@ | |||
public class Deadlines extends Task { | |||
protected static String type = "[D]"; |
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.
Agreed
Important assumptions mainly regarding validity of parameters exist in various methods of the different classes in Duke. Certain assumptions may not hold and that can cause the program to fail. Let's add assertions at every part of the code where important assumptions are being held to ensure that they hold true. Using assertions to document assumptions within the program allows the runtime to verify them. Upon assertion failure, the program typically terminates with an error message and this prevents possible bugs in the program from harming the program.
Branch A-Assertions
* 'master' of https://github.com/jltham/ip: Added assertions throughout the code base.
MarkCommand and UnmarkCommand are very similar in nature, sharing many lines of duplicated codes. Duplicated codes contributes to a longer code and possibly more issues arising due to more lines of code. Review process will be increasingly tedious as well. Let's abstract the functionality of Mark and Unmark to ChangeMark in TaskList, Storage and Ui.
Created new classes: ChangeMark to contain both Mark and Unmark.
Added update and clone functionalities.
Corrected file path for storage file creation.
Jin Lin's Duke
My duke helps you be lazy and lie in bed all day 🛌
It will
most definitely🤞Let me give you a step by step tutorial to it 🤪
Here, I'll give you a sneak peek to the most complex code, the
goat
method ! 😵💫Alright that's enough giving from me ! Follow me for more content !