[Wei Xue] iP#262
Conversation
Added task loading and saving functionality (Level-7)
Added functionality to convert input date time strings to LocalDateTi…
BenedictBCJJ
left a comment
BenedictBCJJ
left a comment
There was a problem hiding this comment.
Overall I think the project is very well segmented and can easily expand in scope in the coming weeks. Good job.
| @@ -0,0 +1,8 @@ | |||
| package CustomExceptions; | |||
|
|
|||
| public class DateTimeFormatNotRecognizedException extends Exception { | |||
There was a problem hiding this comment.
I'm not so sure for exceptions but class names should be nouns according to the coding standard.
Maybe DateTimeFormatException is better since it already implies that it is invalid/not recognised.
| private static final ArrayList<String> validActions | ||
| = new ArrayList<>(Arrays.asList("todo", "deadline", "event", "done", "delete", "list", "bye")); |
There was a problem hiding this comment.
Since this is a constant, it should follow the coding standard of all uppercase and using underscore to separate words.
| return ""; | ||
| } | ||
|
|
||
| public boolean inputsAreValid() { |
There was a problem hiding this comment.
Instead of naming inputsAreValid, areInputsValid seem more inline for a boolean return value method.
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| public class Event extends Task { | ||
| private final LocalDateTime at; |
There was a problem hiding this comment.
I think 'at' is too short a name for a variable that has a rather large scope, should Event class become bigger in the future it might cause confusion in the future.
Since it might mean time or location.
spzj
left a comment
spzj
left a comment
There was a problem hiding this comment.
Overall, good job Wei Xue! Rmb to include javadocs too.
| @@ -0,0 +1,4 @@ | |||
| T | 1 | CS2107 Revision | | |||
There was a problem hiding this comment.
Hi Wei Xue! You can try to delete the output lines before committing, just in case it affects with testing.
| @@ -0,0 +1,152 @@ | |||
| import CustomExceptions.*; | |||
There was a problem hiding this comment.
Perhaps you can list out the exceptions that you will be using in this file.
| LocalDateTime at = parser.getAt(); | ||
|
|
||
| switch (action) { | ||
| case "bye": |
There was a problem hiding this comment.
According to the modules' coding standards, case and default should be at the same indentation as the switch statement.
nicoleang09
left a comment
nicoleang09
left a comment
There was a problem hiding this comment.
Your code quality is good and I could barely find any violations. I like how you broke bigger functions up into smaller ones and made your code easy to understand. Good job!
| newTask = new ToDo(description); | ||
| } else if (taskType.equals("D")) { | ||
| newTask = new Deadline(description, dateTime); | ||
| } else { |
There was a problem hiding this comment.
Maybe instead of using the else block to handle the case of an Event task, it can be used to handle invalid task types?
| } else if (task instanceof Event) { | ||
| taskType = "E"; | ||
| dateTimeString = ((Event) task).getAtDateTimeString(); | ||
| } else { |
There was a problem hiding this comment.
Should the block to handle to do tasks be stated explicitly in an else-if block instead?
| return remainingTokens.split("/by")[0].trim(); | ||
| } else if (action.equals("EVENT") && remainingTokens.contains("/at")) { | ||
| return remainingTokens.split("/at")[0].trim(); | ||
| } else { |
Added JavaDoc to describe the different classes
Branch a assertions
Branch b reminders
4 participants
No description provided.