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
Implement commands and encapsulate fields in Purchase object #137
Conversation
Encapsulation of fields in Purchase & Implementation of RemovePaidCommand
* Stores the details to edit the installment with. Each non-empty field value will replace the | ||
* corresponding field value of the installment. | ||
*/ | ||
public static class EditInstallmentDescriptor { |
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.
can consider abstracting this class out? might be neater.
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.
alright noted! thanks :)
EditInstallmentCommand.MESSAGE_USAGE), pe); | ||
} | ||
|
||
EditInstallmentCommand.EditInstallmentDescriptor editInstallmentDescriptor = |
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.
Can consider importing the nested class straight to improve readability.
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.
noted, thanks for the tip!
@@ -51,8 +53,12 @@ public void resetData(PurchaseList purchaseList) { | |||
//=========== Getter Methods ================================================================================== | |||
|
|||
public Purchase getPurchase(int purchaseIndex) { | |||
Index index = Index.fromOneBased(purchaseIndex); | |||
return allPurchases.get(index.getZeroBased()); | |||
try { |
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.
I think the try
/ catch
statement is not needed here, a guard conditional might be better.
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 would this be so?
|
||
public double getMonthlyLimit() { | ||
return monthlyLimit; | ||
public ArrayList<Purchase> getPurchaseList() { |
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 return a List
instead? Not much issue here actually, but I think its good to return the interface instead of concrete class instead.
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.
ArrayLists used were changed to ObservableLists to accomodate ListFinancesCommand
@@ -132,7 +99,7 @@ public void addSinglePurchase(Purchase purchase) { | |||
* | |||
* @param itemNumber of payment to be deleted | |||
*/ | |||
public Purchase deleteSinglePurchase(int itemNumber) { | |||
public Purchase deleteSinglePurchase(int itemNumber) throws PurchaseNotFoundException { |
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.
Runtime exception no need to declare in method signature, but I am not too sure on which is better. There is an argument that it may be confusing to differentiate between checked exceptions, and can also clutter logic. But I am not too sure how valid these opinions are.
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.
alright thanks for the tip!
@@ -97,7 +121,7 @@ public Purchase deletePurchase(int purchaseIndex) { | |||
public double totalSpending() { | |||
double total = 0; | |||
for (Purchase purchase : allPurchases) { | |||
total += purchase.getMoneySpent(); | |||
total += purchase.getMoneySpent().getPurchaseAmount(); |
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.
return purchase.stream().reduce(0, (prev, next) -> prev + next.getMoneySpent().getPurchaseAmount()
works too
Change purchaseList and installmentList to ObservableList
Things done:
commands added: add-paid, delete-paid, edit-install, list
encapsulation of fields in
Purchase
objectreorganise methods in files