AY1920S1-CS2103T-W11-2 / main

SplitWiser - Epic debt tracking
https://ay1920s1-cs2103t-w11-2.github.io/main/
MIT License
0 stars 5 forks source link

Add Settle command to log transfers made in an Activity #190

Closed podocarp closed 4 years ago

podocarp commented 4 years ago

Settle debts

Settle debts with settle p/1 p/2 e/[amount] which means 1 pays 2 $amount. If $amount is missing, then the entire debt is settled. Bounds checking performed for

Minor changes to other things:

Todo

Also I noticed that in Activity.java we have getParticipantsIds and getParticipantIds. One returns List<Int> and the other ArrayList<Int>. Should clean that up.

Aulud commented 4 years ago
1. Haven't thought through closely what's the best way to do this, but it would be good to be able to have settlement "expenses" made visually distinct from normal expenses cause its possible for users to type in the exact same description as what a settlement expense has, and it can potentially be confusing.

settle p/p1 p/p2 e/10 is currently indistinguishable from expense p/p2 e/20 p/p1 d/p1 paid p2

1. Right now it seems like you're using `Amount(0)` as a way of flagging to `SettleCommand` to simply pay off all the debt. Rightfully I would think that there should be a separate class from `Expense`, e.g. `Payment` or something, and then have both `Expense` and `Payment` extend/implement `Transaction`.

But non-essential cause we're probably not touching this code once this module is over.

We should try to implement more specific classes (e.g. use the Amount class for individual expense items and a Transfer class for required and actual transfers made) to distinguish between expenses and transfers.

Preferably, the Activity class should expose methods such as getRequiredTransfers so that it can retrieve a List<Transfer> for transfers yet to be made (we can lazily calculate this), and a getCompletedTransfers that will return a List<Transfer> for transfers that were actually made.

Otherwise, it is difficult for the UI component to be able to distinguish between expenses and actual transfers if they are all grouped together and treated as an actual Expense.

I could move some code from the UI component back into Activity, since right now it is doing some of the computational work.

liakify commented 4 years ago

Currently settlement is reusing Amount and Expense to pay off debts. However, due to constraints on Amount, such bugs can occur where users are unable to directly settle amounts that exceed 1000000.

The program simply doesn't do anything which may confuse the user, and you can only do settle p/tom p/you e/1000000 to pay off at most $1m at a time. image

podocarp commented 4 years ago

Currently settlement is reusing Amount and Expense to pay off debts. However, due to constraints on Amount, such bugs can occur where users are unable to directly settle amounts that exceed 1000000.

What do you suggest we do? To be honest I don't get the upper bound on Amount. If Amount has an upper bound, we should cap the total expenses in the whole activity, not just one expense in that activity. I for one am for removing the upper bound altogether.

podocarp commented 4 years ago

Preferably, the Activity class should expose methods such as getRequiredTransfers so that it can retrieve a List<Transfer> for transfers yet to be made (we can lazily calculate this), and a getCompletedTransfers that will return a List<Transfer> for transfers that were actually made.

Done. See getNonSettlementExpenses(), getSettlementExpenses(), getSolution()

Aulud commented 4 years ago

Preferably, the Activity class should expose methods such as getRequiredTransfers so that it can retrieve a List<Transfer> for transfers yet to be made (we can lazily calculate this), and a getCompletedTransfers that will return a List<Transfer> for transfers that were actually made.

Done. See getNonSettlementExpenses(), getSettlementExpenses(), getSolution()

Nice!! Thanks haha