[AY1920S1-CS2113T-T12-2] Duke++ #28
Conversation
|
@ChaojieLiu666 |
There was a problem hiding this comment.
While it is impressive that you used advanced java concepts to achieve certain tasks, and this speaks of your technical capabilities, but keep in mind that simplicity always trumps everything else. There are certain aspects of your code that are do not immediately convey their purpose. Either consider good documentation around it, or maybe just refactor it.
Note that we follow an object oriented paradigm, so please try to stick to that at all times. I hope you are also aware of all project constraints.
src/main/java/duke/Duke.java
Outdated
| * Represents our Duke and contains the main program of Duke. | ||
| */ | ||
| /* | ||
| public class Duke { |
There was a problem hiding this comment.
Commented code should not exist on master.
| @@ -0,0 +1,117 @@ | |||
| package duke.commons; | |||
|
|
|||
| // borrowed from AddressBook Level_3 | |||
There was a problem hiding this comment.
Try not add comments like this here. This could be part of the class javadoc. You could prepend it with a 'NOTE' tag.
| String[] commandNameWords = Arrays.copyOfRange(commandName.split("\\s+"), 0, 2); | ||
|
|
||
| if (commandNameWords.length == 2) { | ||
| List<Command> validCommands = COMMANDS.get() |
There was a problem hiding this comment.
Clever solution. Consider documenting this though.
As someone reading your code for the first time, it wasn't obvious what was happening.
src/main/java/duke/model/Budget.java
Outdated
|
|
||
| public class Budget { | ||
|
|
||
|
|
There was a problem hiding this comment.
Try not to leave out stray empty lines
src/main/java/duke/model/Budget.java
Outdated
| * | ||
| * @throws DukeException if unable to save the file successfully | ||
| */ | ||
| public void save() throws DukeException { |
There was a problem hiding this comment.
This seems like a storage feature. Do you think this this violates the separation of concerns principle? The Budget model should not deal with file writing. You must restructure this.
| * | ||
| * @param <T> the subclassed builder; see the sof link above. | ||
| */ | ||
| abstract static class Builder<T extends Builder<T>> { |
There was a problem hiding this comment.
I think this needs a bit more explanation.
| * | ||
| * @param <T> The {@code DukeItem} contained in the list. | ||
| */ | ||
| abstract class DukeList<T extends DukeItem> { |
There was a problem hiding this comment.
From a quick glance, it looks like you're saving files as bytes, not in human readable files (as strings). This violates one of the project constraints. Please make sure you are complying with all of them.
Also, there seems to be commented code scattered across the codebase. This is not good practice. We are version controlling code, so you can always go back in time and get stuff you may need.
| * Implements the interface of model module. | ||
| */ | ||
|
|
||
| public class DukePP implements Model { |
There was a problem hiding this comment.
This is an interesting way to structure your code. It looks like this wraps around every operation duke++ is intended to do. I'm concerned that this may violate the separation of concerns and single responsibility principles.
All related logic must be grouped into a single class. Adding, deleting, updating an expense must belong to one class. Stuff like monthly income list could belong to another class. You may want to think about this.
| import static org.junit.jupiter.api.Assertions.fail; | ||
|
|
||
|
|
||
| public class CommandParamsTest { |
There was a problem hiding this comment.
Good job with test cases! Keep it up :)
This reverts commit 8bf8178
Base functionality of PlanBot , now implemented as a Queue of Questions to ask
Fixed regressions
Make recommendations based on User's attributes, checkstyle fixes
AutoComplete
…evious-merge fix the bug caused by previous merge.
Fixed PieChart from previous PR
Refactor in MainWindow
update profile picture name
Final Revision
Update Ui.png
* Add schedule command * Yellow flag handling * Add ui to show schedule * Add ViewSchedule command * Add view schedule message * Add command allowing user to view time specific tasks * Change Date states to public * Change Date states to public * Add test for ViewScheduleCommand * Add ViewScheduleCommand * Change back to private states * Change back to private states * Add test for ViewScheduleCommand * Add overload showSearchResult method * Add ViewScheduleCommand * ViewScheduleCommand displays separate events and deadlines for any given time period * ViewScheduleCommand displays separate events and deadlines for any given time period * Add getDate methods * Add getDate methods * Add ViewScheduleCommand * Add javadoc * Add javadoc * Add javadoc * Organise imports * Expand imports for checkstyle :( * Bug fix * Add abstract boolean isWithinTimeFrame * Change schedule ui * Add abstract method isWithinTimeFrame * Add abstract method isWithinTimeFrame * Add abstract method isWithinTimeFrame * Test ViewScheduleCommand * Add new abtract method isWithinTimeFrame * Add new abtract method isWithinTimeFrame * Add new abtract method isWithinTimeFrame * Add Schedule for printing time table * Add ViewScheduleCommand * Add timetable interface * Add abstract Schedule method * Add abstract Schedule method * Add abstract Schedule method * Add abstract Schedule method * Organise imports * Update with ui image, features and commands * Update * Update * Update * Update * Update * Update * Update * Update * Update * Update * Update * Update * Add javadoc and format code * Change nested loop * Add javadoc * Organise imports * Organise imports * im so technically challenged * Add test for Schedule * Add test for Schedule * Update features and commands * update link to Ui.png
No description provided.