Skip to content
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

Make isSettlement flag immutable #201

Merged
merged 2 commits into from
Nov 9, 2019
Merged

Make isSettlement flag immutable #201

merged 2 commits into from
Nov 9, 2019

Conversation

liakify
Copy link

@liakify liakify commented Nov 9, 2019

Expense was meant to be sort of stateless (with the exception of isDeleted) to minimize risk of bugs but along the way became mutable.

Changes:

  • Made isSettlement flag immutable

Actually tried to make involvedIds[] immutable but it turned out to be lot more involved (pun not intended) than I thought it would be so didn't do it.

@liakify liakify self-assigned this Nov 9, 2019
@liakify liakify requested a review from a team November 9, 2019 07:26
Copy link

@Aulud Aulud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liakify Overall, the changes look okay. However, regarding the array involvedIds, any particular reason we decided to use a primitive integer array for it instead of say, a List<Integer>? Using a Collection would make immutability much simpler since the ArrayList class specifies a copy constructor which we can use to quickly copy an ArrayList to mutate before setting it as a final reference in an Expense instance.

Does this also fix the bug from @JohnNzj's PR with serialisation and de-serialisation of JSON expenses involving settlements and deletion of expenses? Currently, the UI is still doing the logic work for rendering the list of settlements but I'm currently doing a refactoring pass on the UI and Activity class to ensure proper separation of concerns - so I'll update you tonight.

Copy link

@Aulud Aulud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving since the branch was updated to merge new changes from master at commit d109f5b.

@Aulud Aulud merged commit f8be36e into master Nov 9, 2019
@Aulud Aulud deleted the immutable-settlement branch November 9, 2019 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants