-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AY1920S1-CS2113T-F14-1] MooMooMoney #37
base: master
Are you sure you want to change the base?
[AY1920S1-CS2113T-F14-1] MooMooMoney #37
Conversation
* B-Reminders * B-reminders modification * duke.java * modified buidl.gradle and Command * user guide * user guide table of contents * user guide link TOC * user guide * UI * corrected mock UI * rename UI * UI update * updated user guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Good effort in following the comment and naming convention. Do take care of avoiding magic numbers and strings though.
Some of your methods are hard to understand as they contain a lot of underlying detail. Do try to refactor to make it more understandable.
Please close the pr once you have read the comments
src/main/java/moomoo/MooMoo.java
Outdated
} catch (MooMooException e) { | ||
ui.printException(e); | ||
ui.showResponse(); | ||
categoryList = new CategoryList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we avoid using exceptions to control normal workflow as exception is used for "unusual" events
*/ | ||
public static void main(String[] args) { | ||
if (args.length > 0) { | ||
if (args[0].equals("GUI")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary nesting?
import moomoo.task.Storage; | ||
import moomoo.task.Ui; | ||
|
||
public class AddCategoryCommand extends Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to include descriptive header comments for all classes and public methods
* @param storage Storage object for interaction with filesystem. | ||
* @throws MooMooException Thrown when error such as invalid input occurs | ||
*/ | ||
public void execute(ScheduleList calendar, Budget budget, CategoryList catList, Category category, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since its empty you could make it abstract
|
||
/** | ||
* Command that takes in the amount of money spent and the expenditure name from the Parser as strings | ||
* to be converted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like your class description instead
* Value should be true in this class. | ||
* @param input Input given by the user | ||
*/ | ||
public ExitCommand(boolean isExit, String input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need input for this?
scrollPane.vvalueProperty().bind(dialogContainer.heightProperty()); | ||
} | ||
|
||
public void setMoomoo(MooMoo d) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is d?
/** | ||
* Populate the categoryList array with dummy variables. FOT TESTING PURPOSES | ||
*/ | ||
public void testPopulate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be in your test class instead
Scanner scanner = new Scanner(input); | ||
String commandType = scanner.next(); | ||
switch (commandType) { | ||
case ("bye"): return new ExitCommand(true, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your return should be on the next line
public void saveScheduleToFile(ScheduleList calendar) throws MooMooException { | ||
createFileAndDirectory(this.scheduleFilePath); | ||
|
||
String list = "Schedule: \n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this named list?
Resolved Merge conflicts and tested Delete Expenditure Command
# Conflicts: # src/main/java/moomoo/command/AddExpenditureCommand.java # src/main/java/moomoo/task/Category.java # src/main/java/moomoo/task/Expenditure.java # src/main/java/moomoo/task/Parser.java
Add Delete Expenditure Function
…anch-fix-tutor-issues
Allow user to set one budget for multiple categories
Get updated changes
Added alert feature
Updating GY repo
Fixed category test files
Added sorted schedule list and notifications test
Graph Commands Junit test completed
Final (?) merge to gy
Get updated code
Fixed schedule output bug
Fix display and added documentation
Merge to gy pls
Fixed offset issue with graph axis
@guanyewtan
@jinwentay
@shannonlee98
@JOSHTAM