[Nguyen D.Q. Minh] iP#450
Conversation
afroneth
left a comment
afroneth
left a comment
There was a problem hiding this comment.
Good job! The overall code quality is high. However, there is a need to refactor some small code style violations.
| public String toString() { | ||
| return String.format("[D]%s (by: %s)", super.toString(), this.by); | ||
| } | ||
| } |
There was a problem hiding this comment.
I like the clean code here. Is it perhaps better to rename the variable "by" to something clearer?
| } | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| new Duke().execute(); |
| Parser parser = new Parser(command); | ||
| String commandType = parser.getCommandType(); | ||
| switch (commandType) { | ||
| case "bye": { |
There was a problem hiding this comment.
Shouldn't there be no indentation for case clauses?
| private String commandType; | ||
| private int index; | ||
|
|
||
| public Parser(String command) { |
There was a problem hiding this comment.
Is there too much usage of if-else loops here?
| BufferedWriter writer = Files.newBufferedWriter(this.filePath); | ||
| writer.write(String.valueOf(tasks.size())); | ||
| writer.newLine(); | ||
| for(Task task : tasks) { |
There was a problem hiding this comment.
Should there be a white space character after 'for'?
| openFile(); | ||
| BufferedReader reader = Files.newBufferedReader(this.filePath); | ||
| int taskCount = Integer.parseInt(reader.readLine()); | ||
| for(int i = 0; i < taskCount; ++i) { |
There was a problem hiding this comment.
Should there be a white space character after 'for'?
| boolean isDone = (status.equals("[O]")); | ||
| Task result; | ||
|
|
||
| switch (taskType) { |
There was a problem hiding this comment.
Shouldn't there be no indentation for case clauses?
| throw new DukeException("Error while loading data."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is line 35 (blank statement) removable?
Asuraxsoul
left a comment
Asuraxsoul
left a comment
There was a problem hiding this comment.
In general, I feel that your code mostly complies to the coding standards specified, and I feel that there are only a few minor tweaks here and there required.
Good job for designing your duke program in a way that it is very easy to read and understand! 👍
| @@ -0,0 +1,18 @@ | |||
| public class Deadline extends Task { | |||
| protected String by; | |||
There was a problem hiding this comment.
Maybe you can change "by" variable name to be more descriptive? Can consider "timeDescription"?
| @@ -0,0 +1,18 @@ | |||
| public class Event extends Task { | |||
| protected String at; | |||
There was a problem hiding this comment.
Similar to Deadline class, could the "at" variable name be changed to something more descriptive?
| throw new DukeException(":( OOPS!!! The description of an event cannot be empty."); | ||
| } | ||
| String rest = command.substring(6); | ||
| int at_position = rest.indexOf("/at "); |
There was a problem hiding this comment.
Should underscore be used for naming here? According to the coding standards, underscores may be used in test method names and for constant names that are all caps (eg, MAX_ITERATIONS).
| } | ||
|
|
||
| private String getStatusIcon() { | ||
| return isDone ? "O" : "X"; |
There was a problem hiding this comment.
Consider storing "O" and "X" as constants and then referring to them?
| public String toString() { | ||
| return String.format("[%s] %s", this.getStatusIcon(), this.description); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
I feel that it would be better to add 1 empty line at the end of this file so that git knows that the file terminates here.
| System.out.println(e.getMessage()); | ||
| } | ||
|
|
||
| public void greetings() { |
There was a problem hiding this comment.
Can consider changing "greetings" method name to a verb form such as "greet".
Merge branch-A-Assertions
Merge branch-A-CodeQuality
Merge branch-BCD-extensions
3 participants
No description provided.