[Mark Biju George] iP#306
Conversation
LowJiaHao99
left a comment
There was a problem hiding this comment.
Overall, this is a pretty good effort in following Java Coding Standards closely. The code is pretty clean and simple and it helps to improve readability. Some names of the variable can be improved to further enhance it. There are also some inconsistency, that needs addressing, in regards to the way you organize your code.
Other than that, good job!! 👍
| + "|____/ \\__,_|_|\\_\\___|\n"; | ||
| System.out.println("Hello from\n" + logo); | ||
| public static void main(String[] args) throws DukeException { | ||
| Scanner myObj = new Scanner(System.in); |
There was a problem hiding this comment.
Would "scanner" be a better variable name here?
| int counter = 0; | ||
| ArrayList<Task> tasks = new ArrayList<Task>(); | ||
|
|
||
| System.out.println("Hello! I'm Duke"); |
There was a problem hiding this comment.
Would you consider creating a constant final String for these messages to be printed?
| if (input.equals("bye")) { | ||
| System.out.println("Bye. Hope to see you again soon!"); | ||
| break; | ||
| } else if (input.equals("list")){ |
There was a problem hiding this comment.
As per java Coding Standards
Java reserved words should be followed by a white space.
Perhaps you can add a white space between the condition and the open parenthesis to be consistent with the rest of the code.
I notice many similar issues throughout the rest of the code.
| System.out.println(currentTask); | ||
| } catch (StringIndexOutOfBoundsException e) { | ||
| throw new DukeException("☹ OOPS!!! Please tell me which task you want me to mark as not done!"); | ||
|
|
There was a problem hiding this comment.
Perhaps this might be a mistake, do remember to leave out empty lines within a logic block.
I see such mistake appearing occasionally in the rest of the code.
| System.out.println("Hello from\n" + logo); | ||
| public static void main(String[] args) throws DukeException { | ||
| Scanner myObj = new Scanner(System.in); | ||
| int counter = 0; |
There was a problem hiding this comment.
How about a more intuitive name here? Suggestion: "numOfTasksInList".
| } catch (StringIndexOutOfBoundsException e) { | ||
| throw new DukeException("☹ OOPS!!! Please tell me which task to mark as done!"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Are the empty lines used to differentiate between the next condition in the user input processing? This is a pretty good style as it notifies readers/reviewers of the change in logic.
| this.isDone = false; | ||
| } | ||
|
|
||
| public String getStatusIcon() { |
There was a problem hiding this comment.
I like the way you name this method. It is short, direct and natural.
Much better than getIsDone() which is what I named mine. 😅
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
I like the use of toString() to print details of the task. The method name itself is already specific to its action and it is a built-in method in all java objects.
|
|
||
| public Deadline(String description, String by) { | ||
| super(description); | ||
| this.by = by; |
There was a problem hiding this comment.
Perhaps this variable name can be more specific? One might mistake 'by' as 'added by' instead of 'complete by'.
| } | ||
| } catch(ArrayIndexOutOfBoundsException e) { | ||
| throw new DukeException("☹ OOPS!!! Please provide a timing for your event."); | ||
| } |
There was a problem hiding this comment.
Perhaps this might have been missed during checks, did you made a mistake and left out a empty white line to separate the if-else logic blocks?
| f.createNewFile(); | ||
| } | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Perhaps, parsing user inputs could be done in another class called Parser for more abstraction and OOP.
| int toBeMarked = Integer.parseInt(input.substring(5)); | ||
| Task currentTask = tasks.get(toBeMarked - 1); | ||
| currentTask.markAsDone(); | ||
| System.out.println("Nice! I've marked this task as done:"); |
There was a problem hiding this comment.
Perhaps, printing of messages could be done in another Ui class for better abstraction and OOP.
| f.createNewFile(); | ||
| } | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Perhaps, this code block is too long, could work towards better abstraction overall.
Parsing and storage may occur differently if there are more spaces/unexpected input Assertions are needed to test whether parsing and storage is working properly Add assertions in parts of storage and parser code to enforce testing. Let's observe the results of these assertions using different inputs
Some lines are too long. They need to be shortened to improve readability. Line breaks are placed such that longer lines of code are broken down into shorter segments
Add assertions
Branch A-CodeQuality
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features
Here's a sneak peek into what's going on behind the scenes😏