3
\$\begingroup\$

I am taking a class in mobile application development this fall. I have finished my first assignment of creating a class to divide an arbitrary dollar amount into the minimum number of bills and coins. I have completed the assignment and everything works as intended (outside of an occasional error I'm having with computing pennies, but I am working on that). My main reason for posting is I would like to try and make my code more Swift like. I am coming from a mostly C background so I am learning the ins and outs of Swift and OOP on the fly. So if you have any suggestions to making my code more "Swift like" I would greatly appreciate it.

public class Cash
{
    //    Denominations of monies
    let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

    //    Backing variable
    var value:Double

    //    Computed Property of minimum number of bills to satisfy the value sent in
    public var minBills:[Int]? {
        var tempArray = [Int]()
        var tempValue: Double
        if value >= 0 {
            for index in 0..<bills.count {
                tempValue = value / bills[index]                  //Calculating the amount of bills
                value -= Double(Int(tempValue)) * bills[index]    //Recalculating the value
                tempArray.append(Int(tempValue))                  //Adding number of bills needed to tempArray
            }
            return tempArray                                      //Returns entire tempArray to minBills
        }
        else {
            return nil
        }
    }

    //Constructor
    public init(value: Double) {
        self.value = value
    }
}
\$\endgroup\$
3
\$\begingroup\$

The computed property minBills modifies the value property, which would be unexpected to the caller:

let cash = Cash(value: 50.0)
print(cash.minBills!) // [1, 0, 0, 0, 0, 0, 0, 0, 0]  OK!
print(cash.minBills!) // [0, 0, 0, 0, 0, 0, 0, 0, 0]  What?

Better make value a constant property:

let value: Double

and create a mutable copy only for the computation.

let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

can be made a type property

static let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

because it is the same for each instance.

But why do you need a class at all? This could be just a (free) function

func minimumBills(for value: Double, withDenominations: [Double]) -> [Int]?

instead. Or, if you plan to add more functionality to the Cash type, consider defining it as a struct unless the reference semantics of a class are explicitly needed.

Another possible design would be to define a CoinSystem type with “factory methods” for known coin systems:

public struct CoinSystem {
    let denominations: [Double]

    public init(denominations: [Double]) {
        self.denominations = denominations
    }

    public func optimalBills(for value: Double) -> [Int]? {
        // ... computation here ...
    }

    public static var usDollar = CoinSystem(denominations:
        [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01])
}

This would be used as

let usCoins = CoinSystem.usDollar
let optimal = usCoins.optimalBills(for: 12.34)

and allows adding more coin systems easily.

Now let us consider the “main code,” i.e. the computation of

public var minBills:[Int]?

tempArray and tempValue are bad variable names because they do not indicate what the variable stands for. Also tempValue is only needed inside the scope of the for-loop.

Returning early on error conditions simplifies the code and saves one indentation level:

if value < 0 { return nil }

or

guard value >= 0 else { return nil }

Iterating of the bills array is better done with

for bill in bills {
    // ...
}

instead of subscripting.

Summarizing these suggestions, the code could look like this:

public struct CoinSystem {
    let denominations: [Double]

    public init(denominations: [Double]) {
        self.denominations = denominations
    }

    public func optimalBills(for value: Double) -> [Int]? {
        guard value >= 0 else { return nil }

        var remainingValue = value
        var result = [Int]()
        for denomination in denominations {
            let count = Int(remainingValue/denomination)
            remainingValue -= Double(count) * denomination
            result.append(count)
        }

        return result
    }

    public static var usDollar = CoinSystem(denominations:
        [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01])
}

You said

.... outside of an occasional error I'm having with computing pennies ...

That is to be expected, because Double is binary floating point number, and cannot represent all (decimal) fractions exactly. Good resources for this topic are

Possible remedies are

  • Work with integers only (representing cents, or whatever the lowest possible denomination is).
  • Use the Decimal type to represent the monetary values.

Finally: What you are solving is the Change-making problem, and your approach is the “greedy method:” Always pick the largest denomination which is not greater than the remaining amount. This gives the optimal result for so-called “canonical coin systems,” like the one used in the US, but not necessarily for arbitrary coin systems.

The general problem can be solved with dynamic programming, and you'll find further information in the above referenced Wikipedia page.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the response and the resources you provided. I will take a careful look at your code and the information you linked to improve my future work! \$\endgroup\$ – Austin Aug 29 '18 at 17:46

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.