[Chen Jiehan] Duke Increments #362
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (#9)
There was a problem hiding this comment.
Hello, I like the splitting of commands into different classes, would definitely help when expanding on the list of commands to use. One thing I have difficulties understanding is the isExit method for all commands. Could the same be accomplished by return false/true without the isExit boolean? Also, I noticed that the ByeCommand isExit method seems to return false, which would let the loop on Duke continue. Is that intended or did I misinterpret the code?
Nice job on the date formatting too!
| } | ||
|
|
||
| /** | ||
| * execute the command of adding task |
There was a problem hiding this comment.
Incomplete JavaDocs, should give clearer description.
| @@ -0,0 +1,41 @@ | |||
| package Command; | |||
There was a problem hiding this comment.
Package naming should begin with small letter to avoid conflict with names of classes or interfaces.
src/main/java/Duke.java
Outdated
| private Storage storage; | ||
| private TaskList tasks; | ||
| private Ui ui; | ||
|
|
There was a problem hiding this comment.
Fields pertaining to Duke such as Storage, TaskList and Ui should be declared outside the main method block.
There was a problem hiding this comment.
I believe this is due to the incorrect indentation level
src/main/java/Duke.java
Outdated
| } | ||
| } | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
File path should be one that is accessible with reference to duke as the root file, rather than your C: Drive
src/main/java/Parser/Parser.java
Outdated
| public static Command parse(String fullCommand) { | ||
| int i = fullCommand.indexOf(' '); | ||
| String first = getFirstWord(fullCommand); | ||
| switch (first) { |
There was a problem hiding this comment.
switch case indentation is off. Case should be aligned with switch, according to Java Coding Standard documentation.
src/main/java/TaskList/Task.java
Outdated
| String aa; | ||
| int time; | ||
|
|
||
| switch (Integer.parseInt(str[0])) { |
| } | ||
|
|
||
| /** | ||
| * |
src/main/java/Command/Command.java
Outdated
|
|
||
| public abstract void execute(TaskList t, Ui ui, Storage storage) throws IOException; | ||
|
|
||
| public abstract boolean isExit(); |
There was a problem hiding this comment.
Ideally, the name of the method should not be the same as the variable name
src/main/java/Command/Command.java
Outdated
|
|
||
| public Command() {} | ||
|
|
||
| public abstract void execute(TaskList t, Ui ui, Storage storage) throws IOException; |
There was a problem hiding this comment.
One suggestion is to allow this method to be overloaded, since not every command will require all 3 parameters to be used.
|
|
||
| public class DeleteCommand extends Command { | ||
|
|
||
| private int taskNo; |
There was a problem hiding this comment.
Variable naming could be clearer, something like taskNumber
src/main/java/Duke.java
Outdated
| @@ -1,10 +1,59 @@ | |||
| import Command.Command; | |||
There was a problem hiding this comment.
This file has incorrect indentation level, plus multiple lines of unnecessary newlines.
|
|
||
| @Override | ||
| public String toString() { | ||
|
|
| public class Deadlines extends Task { | ||
|
|
||
| public Deadlines(String description) { | ||
|
|
src/main/java/Ui/Ui.java
Outdated
| showToUser(error); | ||
| } | ||
|
|
||
| public void showToUser (String... message) { |
|
|
||
| public class DeleteCommandTest { | ||
| @Test | ||
| public void executeTest() throws IOException { |
There was a problem hiding this comment.
The method name should be clear on what this test case is about
|
|
||
| public class TaskTest { | ||
| @Test | ||
| public void getStatusNumberTest() { |
There was a problem hiding this comment.
Incorrect naming convention for test method name
A-Assertions
A-CodeQuality
C-FriendlierSyntax
No description provided.