-
Notifications
You must be signed in to change notification settings - Fork 89
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
[CS2113-T18-3] Budget_planner #22
base: master
Are you sure you want to change the base?
Conversation
Implement Calendar 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.
Overall, I think codebase is well abstracted into different classes and has good OOP practices. Just look at ways to make parts of your code more readable and succinct as it makes your code more readable and neat.
* This command supports searching by a specific date or by a month. If a date isn't provided, it will not be used | ||
* as a filter. Similarly, if the category or description isn't provided, they won't be used as filters. | ||
*/ | ||
public class FindCommand extends Commands { |
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 use of inheritance
throw new KaChinnnngException("Failed to create directory " + dir.getAbsolutePath()); | ||
} | ||
} | ||
FileHandler fh = new FileHandler("logs/FindCommand.log", 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.
You could perhaps change logs/FindCommand.log
into a static variable
* @throws KaChinnnngException if there is an error in the command | ||
*/ | ||
public void setAmount(double amount) throws KaChinnnngException { | ||
assert amount >= 0 : "amount should not be negative"; |
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 use of assertions in this class
if (expenseCategoryString.equals("food")) { | ||
if (expenseTypeString.equals("breakfast")) { | ||
return new Food(expenseDescriptionString, expenseDate, expenseAmount, MealType.BREAKFAST); | ||
} else if (expenseTypeString.equals("lunch")) { | ||
return new Food(expenseDescriptionString, expenseDate, expenseAmount, MealType.LUNCH); | ||
} else if (expenseTypeString.equals("dinner")) { | ||
return new Food(expenseDescriptionString, expenseDate, expenseAmount, MealType.DINNER); | ||
} else { | ||
return new Food(expenseDescriptionString, expenseDate, expenseAmount, MealType.UNDEFINED); | ||
} | ||
} else if (expenseCategoryString.equals("transport")){ | ||
if (expenseTypeString.equals("train")) { | ||
return new Transport(expenseDescriptionString, expenseDate, expenseAmount, | ||
TransportationType.TRAIN); | ||
} else if (expenseTypeString.equals("bus")) { | ||
return new Transport(expenseDescriptionString, expenseDate, expenseAmount, | ||
TransportationType.BUS); | ||
} else if (expenseTypeString.equals("taxi")) { | ||
return new Transport(expenseDescriptionString, expenseDate, expenseAmount, | ||
TransportationType.TAXI); | ||
} else if (expenseTypeString.equals("fuel")) { | ||
return new Transport(expenseDescriptionString, expenseDate, expenseAmount, | ||
TransportationType.FUEL); | ||
} else { | ||
return new Transport(expenseDescriptionString, expenseDate, expenseAmount, | ||
TransportationType.UNDEFINED); | ||
} | ||
} else if (expenseCategoryString.equals("utilities")) { | ||
if (expenseTypeString.equals("water")) { | ||
return new Utilities(expenseDescriptionString, expenseDate, expenseAmount, UtilityType.WATER); | ||
} else if (expenseTypeString.equals("electricity")) { | ||
return new Utilities(expenseDescriptionString, expenseDate, expenseAmount, UtilityType.ELECTRICITY); | ||
} else if (expenseTypeString.equals("gas")) { | ||
return new Utilities(expenseDescriptionString, expenseDate, expenseAmount, UtilityType.GAS); | ||
} else{ | ||
return new Utilities(expenseDescriptionString, expenseDate, expenseAmount, UtilityType.UNDEFINED); | ||
} | ||
} else { | ||
throw new KaChinnnngException("Please enter a valid 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.
You could look at reducing nesting here to reduce arrowhead code!
if (incomes.isEmpty()) { | ||
System.out.println("You have no recorded incomes."); | ||
} else { | ||
System.out.println("Here are your incomes:"); | ||
for (int i = 0; i < incomes.size(); i++) { | ||
System.out.println((i + 1) + ". " + incomes.get(i).toString()); | ||
totalIncome += incomes.get(i).getAmount(); | ||
} | ||
System.out.printf("Total income is: $%.2f.\n", totalIncome); |
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.
Personally I feel you could abstract this out into a method as it is quite similar to your for loop below. This also helps to make your code neater.
public void getHelp(){ | ||
ui.showLineDivider(); | ||
// Shows a message linking the user to the user guide of our app | ||
System.out.println("You can access our user guide by " + | ||
"https://docs.google.com/document/d/1BOz_v4eYQ8y7Dje6Jm6nqymi9jmrsb9MAohLCL_sLvI/edit?usp=sharing\n"); | ||
// Displays the various commands that user can use including the respective formats of the commands | ||
System.out.println("Functions and their format:\n"); | ||
System.out.println("Adding an entry: add\nFormat:\n" + "Add expense /category /description /value\n" + | ||
"Add income /description /value\n"); | ||
System.out.println("Listing all entries: list\nFormat:\n" + "list\n" + "list income\n" + | ||
"list expense\n"); | ||
System.out.println("Deleting an entry: delete\nFormat:\ndelete income [index_pos]\n" + | ||
"delete expense [index_pos]\n"); | ||
System.out.println("Check balance of income: balance\nFormat:\n" + "balance\n"); | ||
System.out.println("Exiting the program: exit\nFormat:\n" + "exit"); | ||
ui.showLineDivider(); |
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.
You could consider constants for this rather than string literals. You could make your entire menu in a string constant and thus simplify this method.
double amountFromFile = Double. parseDouble(textLine.split(" \\| ")[2]); | ||
LocalDate dateFromFile = LocalDate.parse(textLine.split(" \\| ")[3]); | ||
// Check for valid amount range | ||
if (amountFromFile > 999999.99 || amountFromFile <= 0) { |
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.
Perhaps you could convert this into a constant variable rather than a string literal in case you'd like to change your boundary values in the future and it needs to be used elsewhere in your codebase.
Added JUnit tests and javadocs
Bryan more j unit test
…nCheung18/tp into JUnit-test-for-clear-function
JUnit test for storage
J unit test for clear function
* add-expense-manager-unit-tests: add expenseManager unit tests
Fix find parser bug
minor bug fix
edited ug and dg
no message
no message
A financial tracker for general users