5
\$\begingroup\$

I'm building a clone banking app at the moment, and one of the things I'm trying to do is add Split Transaction (which can then be shared with a set of friends paying a given amount each).

Initially, the transaction is split equally amongst friends (unless it doesn't split equally, in which case the remainder gets added on to one unlucky friend). The user can then manually adjust the amount each pays, which then updates the others. If the user has manually adjusted an amount for a friend, this friends split doesn't get updated automatically when the user adjusts another friend's amount (i.e. if the user says friend1 pays £12, it will always be £12 until the user says otherwise).

I've been fiddling for a while trying to make the method as concise and Swifty as possible - but I'd really appreciate any feedback on my approach.

For the purposes here, I'm only trying split the money equally between people (but I still wanted to explain the user-defined split so that the current code makes sense).

I'm using https://github.com/Flight-School/Money to represent the transaction value, all within a Transaction class. I need to round quite a bit to ensure the split and remainder stick to 2 decimal places. Here's the relevant code:

A struct to hold an amount along with if the user set it or not (needs to be a custom object for codable reasons):

struct SplitTransactionAmount: Codable {
    let amount: Money<GBP>
    let setByUser: Bool
}

A dictionary to hold the friend names, along with their split, and if it's set by the user - also a namesOfPeopleSplittingTransaction array for easy display.

var splitTransaction: [String: SplitTransactionAmount]
var namesOfPeopleSplittingTransaction = [String]()

And here's the method to split the transaction:

private func splitTransaction(amount: Money<GBP>, with friends: [String]) -> [String: SplitTransactionAmount] {

    //First we remove any duplicate names.
    let uniqueFriends = friends.removingDuplicates()
    //Create an empty dictionary to hold the new values before returning.
    var newSplitTransaction = [String: SplitTransactionAmount]()

    let totalAmountToSplitRounded = amount.rounded.amount
    let numberOfSplitters = uniqueFriends.count

    let eachTotalRaw = totalAmountToSplitRounded / Decimal(numberOfSplitters)
    let eachTotalRounded = Money<GBP>(eachTotalRaw).rounded.amount

    let remainder = totalAmountToSplitRounded - (Decimal(numberOfSplitters) * eachTotalRounded)

    if remainder == 0 {
        //If the amount to split each goes in to the total with no remainder, everyone pays the same.
        for friend in uniqueFriends {
            newSplitTransaction[friend] = SplitTransactionAmount(amount: Money(eachTotalRounded), setByUser: false)
        }
    } else {
        for friend in uniqueFriends {
            if friend == uniqueFriends.first! {
                //Unlucky first friend has to pay a few pence more!
                newSplitTransaction[friend] = SplitTransactionAmount(amount: Money(eachTotalRounded + remainder), setByUser: false)
            } else {
                newSplitTransaction[friend] = SplitTransactionAmount(amount: Money(eachTotalRounded), setByUser: false)
            }
        }
    }
    return newSplitTransaction
}

I think the problem I'm finding is the code makes perfect sense to me, but I'm not sure how clear it is to an outside reader. Any thoughts on my approach would be much appreciated (and sorry for the long question!). And I'd also love to know if there's any way to write this more concisely!

I've also extended array to remove duplicates:

extension Array where Element: Hashable {
    func removingDuplicates() -> [Element] {
        var addedDict = [Element: Bool]()

        return filter {
            //When filter() is called on a dictionary, it returns nil if the key is new, so we can find out which items are unique.
            addedDict.updateValue(true, forKey: $0) == nil
        }
    }

    //This will change self to remove duplicates.
    mutating func removeDuplicates() {
        self = self.removingDuplicates()
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is it mandatory to have the first friend pay the extra remainder or should it be random? \$\endgroup\$ – ielyamani May 28 at 13:23
  • 1
    \$\begingroup\$ Rounding eachTotalRaw may result in a negative remainder and in that case, the first friend would be the lucky one \$\endgroup\$ – ielyamani May 28 at 13:50
4
\$\begingroup\$

Separation of concerns

There is (in my opinion) too much logic in the main function. Splitting an amount “fairly” into \$ n \$ parts is difficult enough. That function should not know about “friends,” that a list of friends might not be unique, or that friends are represented as strings.

I would suggest to define a function

splitAmount(total: numParts: )

which takes an amount and a number and returns an array of amounts (the exact signature is discussed later). Your splitTransaction() function can then build on splitAmount().

Why only GBP? – Make it generic!

Your splitTransaction() function takes a Money<GBP> amount, i.e. it works only with pound sterling, the British currency. Only minimal changes are required to make the same function work with arbitrary currencies: Change the function type to

func splitTransaction<Currency>(amount: Money<Currency>, with friends: [String]) -> [String: SplitTransactionAmount]
    where Currency: CurrencyType {
    // ...
}

and replace Money<GBP> by Money<Currency> in the function body.

For the splitAmount() function suggested above that would be

func splitAmount<Currency>(amount: Money<Currency>, numParts: Int) -> [Money<Currency>]
    where Currency: CurrencyType {
    // ...
}

The algorithm

Here are two examples how an amount split into 6 parts with your algorithm:

10.04 -> 1.69, 1.67, 1.67, 1.67, 1.67, 1.67
10.05 -> 1.65, 1.68, 1.68, 1.68, 1.68, 1.68

In the first example, the remainder is \$0.02\$, and a better result would be achieved by splitting that among two friends:

10.04 -> 1.68, 1.68, 1.67, 1.67, 1.67, 1.67

In the second example, the remainder is \$-0.03\$. Again the split is not optimal, in addition the first amount is less then the others, contrary to the description of your method. A better result would be

10.05 -> 1.68, 1.68, 1.68, 1.67, 1.67, 1.67

The problem is of course the floating point arithmetic. Even if Money uses the Decimal type which can represent decimal fractions exactly: It still cannot represent the result of a division like \$ 10.05 / 6 \$ exactly. Rounding that fraction to a multiple of \$ 0.01 \$ (the “minor unit” of the currency) can result in a smaller or a larger value (as in the second example).

Actually such calculations are much better done in integer arithmetic. If £10.04 are represented as 1004 (pennies), then we can perform an integer division with remainder

1004 = 167 * 6 + 2

so that each part is 167 pennies, plus 1 penny for the first two parts. Similarly:

1005 = 167 * 6 + 3

Unfortunately, the Money class does not provide methods to get an amount as integer multiple of the currency's minor unit, but that is not too difficult to implement:

extension Money {

    /// Creates an amount of money with a given number of “minor units” of the currency.
    init(units: Int) {
        self.init(Decimal(units) * Decimal(sign: .plus, exponent: -Currency.minorUnit, significand: 1))
    }

    /// The amount of money as the count of “minor units” of the currency.
    var units: Int {
        var amount = self.amount 
        var rounded = Decimal()
        NSDecimalRound(&rounded, &amount, Currency.minorUnit, .bankers)
        rounded *= Decimal(sign: .plus, exponent: Currency.minorUnit, significand: 1)
        return NSDecimalNumber(decimal: rounded).intValue
    }
}

With these preparations, the splitAmount() function is easily implemented:

func splitAmount<Currency>(amount: Money<Currency>, numParts: Int) -> [Money<Currency>]
    where Currency: CurrencyType {
        let units = amount.units
        let fraction = units / numParts
        let remainder = units % numParts

        return Array(repeating: Money<Currency>(units: fraction + 1),
                     count: remainder)
            + Array(repeating: Money<Currency>(units: fraction),
                    count: numParts - remainder)
}

The above works for non-negative amounts only. If the amounts can be negative as well then a possible implementation would be

func splitAmount<Currency>(amount: Money<Currency>, numParts: Int) -> [Money<Currency>]
    where Currency: CurrencyType {
        let units = amount.units
        let fraction = units / numParts
        let remainder = abs(units) % numParts

        return Array(repeating: Money<Currency>(units: fraction + units.signum()),
                     count: remainder)
            + Array(repeating: Money<Currency>(units: fraction),
                    count: numParts - remainder)
}

Examples:

// Pound Sterling, minor unit is 0.01:
print(splitAmount(amount: Money<GBP>(10.04), numParts: 6))
// [1.68, 1.68, 1.67, 1.67, 1.67, 1.67]

// Iraqi Dinar, minor unit is 0.001:
print(splitAmount(amount: Money<IQD>(10.000), numParts: 6))
// [1.667, 1.667, 1.667, 1.667, 1.666, 1.666]
\$\endgroup\$
  • \$\begingroup\$ I've had couple of great answers for this - thank you! I've selected this answer as it's the clearest (and most understandable at my level - although that's totally subjective!). \$\endgroup\$ – ADB May 29 at 11:06
  • \$\begingroup\$ Out of interest, how would you extend this for negative amounts please? I've tried just using the absolute value for units but I'm not sure if it's as simple as that. \$\endgroup\$ – ADB Jun 3 at 11:25
  • 1
    \$\begingroup\$ @ADB: I have added a possible implementation which works for both positive and negative amounts. \$\endgroup\$ – Martin R Jun 3 at 20:47
  • \$\begingroup\$ That's so helpful - thank you so much. \$\endgroup\$ – ADB Jun 4 at 7:21
4
\$\begingroup\$

A couple of observations:

  • When splitTransaction is diving into Decimal types and the like, that indicates that you’ve placed algorithms at the wrong level.

  • To that end, let’s give the Money the necessary arithmetic operators so that splitTransaction doesn’t get into the weeds of Decimal or other details of the Money type.

  • You have a rounded computed property. I’d suggest making that a function and allowing the caller to specify the type of rounding to be applied.

  • You have made Currency conform to Codable (which you presumably did so you could make Money also Codable). I don’t think you intended to do that because currencies will have all sorts of properties that you most likely don’t want to encode. That should not be codable, and Money should have the currency code as a property, not the currency itself. That code is presumably the only thing you want Money to encode.

  • Your algorithm assumes there will be one user who picks up the remainder. I’d suggest you spread it around. If the bill is €9.02, rather than sticking the first person with €3.02, let’s give two people €3.01 and the third €3.00.

  • You are applying the remainder to the first user. I’d suggest you randomize it. If the list was sorted, for example, you don’t want the same people always getting the short end of the stick.

  • Your rounding algorithm presumably assumes that you’ll be rounding to the nearest pence. But you should handle currencies that generally don’t use fractional values. In fact, going a step further, you should really handle arbitrary minimum units for the currency. E.g. imagine a world where the US decides it doesn’t want to deal with pennies any more, and makes the nickel the minimum unit. Well, that’s how you should be rounding (to the nearest $0.05), not to two decimal places. Likewise, imagine that Japan decided that they weren’t going to deal with anything smaller than ¥20. Then, again, that’s what you should be rounding to, not to the nearest yen.

  • The splitTransaction should probably be generic.

Thus, you end up with something like:

private func splitTransaction<T: Currency>(_ transaction: Money<T>, with friends: [String]) -> [String: SplitTransaction<T>] {
    let uniqueFriends = friends.removingDuplicates()

    guard !uniqueFriends.isEmpty else { return [:] }

    let currency = transaction.currency
    let minimumUnit = currency.minimumUnit
    let total = transaction.roundedToMinimumUnit()
    let count = uniqueFriends.count
    let baseIndividualAmount = (total / count).roundedToMinimumUnit(.down)
    let remainder = total - baseIndividualAmount * count
    let howManyAdjustments = currency.howManyUnits(remainder)

    // split the transaction

    var splitTransaction = uniqueFriends.reduce(into: [:]) {
        $0[$1] = SplitTransaction(amount: baseIndividualAmount, setByUser: false)
    }

    // adjust random friends based upon remainder

    for friend in uniqueFriends.shuffled()[0..<howManyAdjustments] {
        splitTransaction[friend]!.amount = splitTransaction[friend]!.amount + minimumUnit
    }
    return splitTransaction
}

Where:

protocol Currency {
    var code: String { get }
    var minimumUnit: Decimal { get }
    var symbol: String { get }
    var decimalPlaces: Int { get }
    init()
}

extension Currency {
    func howManyUnits<T: Currency>(_ money: Money<T>) -> Int {
        return NSDecimalNumber(decimal: money.value / minimumUnit).intValue
    }
}

struct GBP: Currency {
    let code = "GBP"
    let symbol = "£"
    let decimalPlaces = 2
    let minimumUnit = Decimal(sign: .plus, exponent: -2, significand: 1) // 0.01
}

struct EUR: Currency {
    let code = "EUR"
    let symbol = "€"
    let decimalPlaces = 2
    let minimumUnit = Decimal(sign: .plus, exponent: -2, significand: 1) // 0.01
}

struct JPY: Currency {
    let code = "JPY"
    let symbol = "¥"
    let decimalPlaces = 0
    let minimumUnit = Decimal(sign: .plus, exponent: 1, significand: 2) // 20, for demonstration purposes only
}

class Currencies {
    static let shared = Currencies()

    let availableCurrencies: [String: Currency] = {
        let array: [Currency] = [GBP(), EUR(), JPY()]
        return Dictionary(grouping: array) { $0.code }
            .mapValues { $0.first! }
    }()

    func currency(for code: String) -> Currency {
        return availableCurrencies[code]!
    }
}

extension Array where Element: Hashable {
    func removingDuplicates() -> [Element] {
        return Array(Set(self))
    }

    mutating func removeDuplicates() {
        self = self.removingDuplicates()
    }
}

struct Money<T: Currency>: Codable {
    let value: Decimal
    let currencyCode: String
    var currency: Currency { return Currencies.shared.currency(for: currencyCode) }

    init(_ value: Decimal) {
        self.value = value
        self.currencyCode = T().code
    }

    func roundedToMinimumUnit(_ mode: NSDecimalNumber.RoundingMode = .bankers) -> Money {
        var input = (value / currency.minimumUnit)
        var result = input
        NSDecimalRound(&result, &input, 0, mode)
        return Money(result * currency.minimumUnit)
    }

    static func + (lhs: Money, rhs: Money) -> Money {
        return Money(lhs.value + rhs.value)
    }

    static func + (lhs: Money, rhs: Decimal) -> Money {
        return Money(lhs.value + rhs)
    }

    static func - (lhs: Money<T>, rhs: Money<T>) -> Money<T> {
        return Money(lhs.value - rhs.value)
    }

    static func / (lhs: Money<T>, rhs: IntegerLiteralType) -> Money<T> {
        return Money(lhs.value / Decimal(rhs))
    }

    static func * (lhs: Money<T>, rhs: IntegerLiteralType) -> Money<T> {
        return Money(lhs.value * Decimal(rhs))
    }
}

struct SplitTransaction<T: Currency>: Codable {
    var amount: Money<T>
    let setByUser: Bool
}

Thus:

let money = Money<JPY>(10_040) // with my imaginary rendition where ¥20 is the smallest unit
let split = splitTransaction(money, with: ["fred", "george", "sue"])

That yields:

fred, 3360
george, 3340
sue, 3340

And

let money = Money<GBP>(9.02)

Yields:

fred, 3.00
george, 3.01
sue, 3.01

And in both of these cases, who gets hit with the remainder is randomized.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.