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
Jhchen/ft manage account #157
Conversation
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.
Seems good. But I think the position of the balance in the GUI should be changed, unless I'm reading the code wrong.
@Override | ||
public String toString() { | ||
return getName().toString() + " (" + getDescription().toString() + ")"; | ||
} |
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 don't think the user will ever see the result of toString()
, so up to you if you want to add the balance
in it too.
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.
Looks fine, but take a look at Kenneth's comments in particular.
src/main/java/budgetbuddy/model/transaction/TransactionList.java
Outdated
Show resolved
Hide resolved
src/main/java/budgetbuddy/model/transaction/TransactionList.java
Outdated
Show resolved
Hide resolved
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.
Looks good.
checkBalanceValidity(balance = balance + toAdd.getAmount().toLong()); | ||
balance = balance + toAdd.getAmount().toLong(); | ||
} else { | ||
checkBalanceValidity(balance = balance - toAdd.getAmount().toLong()); | ||
balance = balance - toAdd.getAmount().toLong(); |
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.
Not sure why there's assignment within the arguments here
cf1c3d9
to
83ddc65
Compare
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.
Rebased.
add balance field to the account
(currently all other account commands are able to update the balance displayed in the list.. except for account list... )
closes #136