-
Notifications
You must be signed in to change notification settings - Fork 205
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
[Lim Ao Jun Joel] Duke Increments #15
base: master
Are you sure you want to change the base?
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S2#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
The OpenJFX plugin expects applications to be modular and bundled with jlink, resulting in fat jars that are not cross-platform. Let's manually include the required dependencies so that shadow can package them properly.
… directories if file path is non-existent
…ad of generating a new one for each command
A-Assertions: Use assert feature (not JUnit assertions) to document important assumptions that should hold at various points in the code.
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.
In general, you may want to pay specific attention to SLAP. There are a few instances where some code looks repeated. Those can be extracted to methods.
src/main/java/duke/Duke.java
Outdated
try { | ||
inputLock.acquire(); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); |
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.
You have some custom exceptions, why not use it here effectively?
In general, printing stacktrace is not considered good exception handling.
} | ||
|
||
// Save to disk | ||
try { |
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.
btw, each of this can be extracted out to separate methods to improve the SLAP.
e.printStackTrace(); | ||
} | ||
} | ||
} |
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.
You may want to add a new line at the end of the file to keep checkstyle happy 😛
*/ | ||
public interface Command { | ||
/** | ||
* Parses the given argument string and executes the 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.
if the method is really doing both parsing and executing the command, you may want to separate out the two functionalities.
private void executeCommand(String commandWord, String arg) { | ||
try { | ||
commands.get(commandWord).execute(arg, tasks, ui, storage); | ||
} catch (NullPointerException 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.
why not handle this NPE earlier before calling the execute command?
dtFormatters.add(DateTimeFormatter.ofPattern("dd/MM/yyyy[ HH][:][mm][:ss]")); | ||
dtFormatters.add(DateTimeFormatter.ofPattern("dd-MM-yyyy[ HH][:][mm][:ss]")); | ||
dtFormatters.add(DateTimeFormatter.ofPattern("dd MM yyyy[ HH][:][mm][:ss]")); |
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 agree with the above ☝️ 🙂
return String.format("deadline\n%s\n%s\n%s", deadline.getName(), deadline.getDateTime(), | ||
deadline.getIsDone()); | ||
} else { | ||
assert task instanceof Event; |
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.
👍
src/main/java/duke/ui/TextUi.java
Outdated
private Scanner sc; | ||
|
||
public TextUi() { | ||
lineBreak = "===========================================================\n"; |
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.
why is lineBreak here? I don't see it being used immediately.
src/main/java/duke/ui/Ui.java
Outdated
package duke.ui; | ||
|
||
/** | ||
* Allows Duke to display output to the user. |
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.
You can give a better description in this header comment
String invalidDate = "11/13/2000"; | ||
String validDateWithSlash = "11/12/2000"; | ||
String validDateWithDash = "11-12-2000"; | ||
String validDateWithSpace = "11 12 2000"; |
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.
Thanks prof for the feedback and suggestions! I will refactor my code to better adhere to SLAP and to reuse code more :-) |
|
||
save(storage, tasks); | ||
|
||
ui.showReply(String.format("Got it. I've added this task:\n %s\nNow you have %d tasks in the list.", newTask, |
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.
Our static analysis engine detects that your pull request includes a change that a format string \n
instead of %n
. According to this description, it is a bad practice, since using \n
in format string may cause cross-platform issues.
|
||
save(storage, tasks); | ||
|
||
ui.showReply(String.format("Got it. I've added this task:\n %s\nNow you have %d tasks in the list.", newTask, |
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.
Our static analysis engine detects that your pull request includes a change that a format string \n
instead of %n
. According to this description, it is a bad practice, since using \n
in format string may cause cross-platform issues.
|
||
save(storage, tasks); | ||
|
||
ui.showReply(String.format("Got it. I've added this task:\n %s\nNow you have %d tasks in the list.", newTask, |
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.
Our static analysis engine detects that your pull request includes a change that a format string \n
instead of %n
. According to this description, it is a bad practice, since using \n
in format string may cause cross-platform issues.
|
||
Task removed = removeTask(tasks, taskNo); | ||
|
||
ui.showReply(String.format("Noted. I've removed this task:\n %s\nNow you have %d tasks in the list.", removed, |
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.
Our static analysis engine detects that your pull request includes a change that a format string \n
instead of %n
. According to this description, it is a bad practice, since using \n
in format string may cause cross-platform issues.
private String toSaveable(Task task) { | ||
assert task != null : "task should not be null"; | ||
if (task instanceof Todo) { | ||
return String.format("todo\n%s\n%s", task.getName(), task.getIsDone()); |
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.
Our static analysis engine detects that your pull request includes a change that a format string \n
instead of %n
. According to this description, it is a bad practice, since using \n
in format string may cause cross-platform issues.
No description provided.