[Kwong Chung Yue Jerry] Duke Increments#354
[Kwong Chung Yue Jerry] Duke Increments#354jerryk1997 wants to merge 60 commits intonus-cs2103-AY1920S1:masterfrom
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)
# Conflicts: # src/main/java/tasks.txt
To add gradle support to the project.
| */ | ||
| public void showFoundTasks(LinkedList<Task> taskList) { | ||
| ListIterator<Task> iter = taskList.listIterator(); | ||
| Task current; |
There was a problem hiding this comment.
Your code looks pretty clean with the accurate variable naming as well as javadocs comments. Perhaps you could consider making packages to make it more organized? Otherwise it looks good
| @FXML | ||
| public void initialize() { | ||
| scrollPane.vvalueProperty().bind(dialogContainer.heightProperty()); | ||
| dialogContainer.getChildren().addAll(DialogBox.getUserDialog(Ui.showWelcome(), dukeImage)); |
There was a problem hiding this comment.
I really like your idea of calling it in initialize, good job!
|
|
||
| ui.showTaskAdded(newEvent, taskList); | ||
| return ui.showTaskAdded(newEvent, taskList); | ||
| } |
There was a problem hiding this comment.
Good use of OOP and clean code! But reading through this it might be slightly unclear, maybe you can refactor this part to use case and switch statements?
etlow
left a comment
etlow
left a comment
There was a problem hiding this comment.
Methods and variables are well named and I appreciate the use of StringBuilders! Just have some comments about some parts.
| public Event(String description, String eventDate, boolean status) { | ||
| super(description); | ||
| this.eventDateString = makeEventDate(eventDate); | ||
| this.eventDate = storeAsDateTime(eventDateString); |
There was a problem hiding this comment.
Can be replaced with this(description, eventDate);
| return builder.toString(); | ||
| } | ||
|
|
||
| public static boolean correctInput(String input) { |
| taskList.addTask(newEvent); | ||
|
|
||
| return ui.showTaskAdded(newEvent, taskList); | ||
| } |
There was a problem hiding this comment.
Consider splitting into methods or classes. Maybe todo, deadline and event can go into another method parseTask, and the repeated lines can also be factored out?
|
|
||
| sb.append(hour + ":" + minutes + ":00"); | ||
|
|
||
| LocalDateTime dateTime = LocalDateTime.parse(sb.toString()); |
There was a problem hiding this comment.
How about
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("d/M/yyyy HHmm");
LocalDateTime dateTime = LocalDateTime.parse(date, formatter);
|
|
||
| return output; | ||
| } | ||
| } No newline at end of file |
| } | ||
| } | ||
|
|
||
| public boolean isExit() { |
| return welcomeMessage; | ||
| } | ||
|
|
||
| public String showGoodbye() { |
There was a problem hiding this comment.
Could you include javaDoc for this class
| } | ||
| } | ||
|
|
||
| public String showTaskDone(Task task) { |
There was a problem hiding this comment.
The two methods have the same body, maybe you could consider delete one.
| * Filler method to imitate an interface. | ||
| * @return String empty string | ||
| */ | ||
| protected String toFileFormat() { |
There was a problem hiding this comment.
could you abstract this class and method?
| } | ||
| } | ||
|
|
||
| public String run(String input) { |
There was a problem hiding this comment.
Good morning @jerryk1997 ,
I think that you have good OOP implementation; class responsibilities are clear and non-conflicting, e.g Ui not managing anything else other than I/O interaction. However, I believe you haven't used gradle checkstyle to verify your compliance to coding standard. In addition, I think you could try extracting repeated methods calls to reduce code duplication. You could also improve the parser by simplifying multiple if-case into a switch-case. This should also simplify input verification as non-expected inputs will generally enter into the default case.
There is also a Ui issue.
But all in all. It is good individual effort with taught methodologies.
Keep it up. 👍
no message
Added way for user to specify priorities of tasks
8 participants
No description provided.