[Kaitlyn] iP#461
Conversation
w-yeehong
left a comment
w-yeehong
left a comment
There was a problem hiding this comment.
Apart from a few easily correctable nits here and there, I think your project is going great! Once the JavaDoc is added in, everything should be even neater. Keep up the awesome work 😀
| import java.util.Scanner; | ||
|
|
||
| public class Ui { | ||
| private static final String logo = |
There was a problem hiding this comment.
I think the ASCII cat is really cute 👍 Maybe consider changing the variable name to all uppercase because it can be treated as a constant?
| package duke.datetime; | ||
|
|
||
| public enum DateTimeFormat { | ||
| String, |
There was a problem hiding this comment.
I like the use of an enum to keep track of valid & invalid formats. Much better than using integer status codes. Perhaps these should be in uppercase as it is an enum? I noticed a few other variables in a similar position.
| @@ -0,0 +1,12 @@ | |||
| package duke.command; | |||
|
|
|||
| public enum CMD { | |||
There was a problem hiding this comment.
I think enum names should be written in PascalCase based on the coding standard? 🤔
| @@ -0,0 +1,25 @@ | |||
| package duke.command; | |||
|
|
|||
| import duke.*; | |||
There was a problem hiding this comment.
Would it be possible to avoid wildcard imports?
| ArrayList<String> tasksStr = new ArrayList<>(); | ||
| String str; | ||
| reader.readLine(); | ||
| while((str = reader.readLine()) != null) { |
There was a problem hiding this comment.
Maybe add a space character after while?
|
|
||
| public class Parser { | ||
| public static Command parse(String userInput) throws DukeException { | ||
| int space_idx = userInput.indexOf(' '); |
| } | ||
|
|
||
| @Override | ||
| public void execute(TaskList taskList, Ui ui, Storage storage) throws DukeException { |
There was a problem hiding this comment.
Do you think the nesting may be a bit deep here? I tend to find nested if-else hard to read when it goes down to 3-4 layers. One possible way of avoiding this:
if (taskList.isEmpty()) {
ui.display("UR LIST HAZ NUTHIN LOLOL");
return; // Stop the rest of the method from executing
}
if (by.isEmpty()) {
/* continue from here */
| if (taskList.isEmpty()) { | ||
| ui.display("UR LIST HAZ NUTHIN LOLOL"); | ||
| } else { | ||
| if (this.by.isEmpty()) { |
There was a problem hiding this comment.
Maybe the this keyword can be removed?
A-CodingStandard: Tweak the code to comply with a coding standard
jonfoocy
left a comment
jonfoocy
left a comment
There was a problem hiding this comment.
Overall I think most of the naming was quite well done! 👍
I just had some suggestions for 1/2 names which I didn't understand immediately.
| import duke.ui.Ui; | ||
|
|
||
| public class DeleteCommand extends Command { | ||
| private int idx; |
There was a problem hiding this comment.
Perhaps change this to index? In my opinion it's more understandable from the get go and it's not too many more characters 😉
| } | ||
|
|
||
| /** | ||
| * Overloaded class constructor if a deadline is spceified for filtering out |
| } | ||
|
|
||
| private static String tasks2String(ArrayList<Task> tasks) { | ||
| String ret = ""; |
There was a problem hiding this comment.
Just a suggestion but I would name it as toReturn instead :)
| * @param msg | ||
| * @return | ||
| */ | ||
| public String fmtMsg(String msg) { |
There was a problem hiding this comment.
Perhaps you might consider changing it to formatMsg instead?
Add A-Assertions
Revert "Add A-Assertions"
3 participants
No description provided.