Skip to content

Commit

Permalink
Rework AnyCurrency and CurrencyProtocol implementations
Browse files Browse the repository at this point in the history
Motivation:

While working on linux CI (#7) and due to bug #17, many holes in the current implementation were found.

These include issues with handling NaN representations, integer overflow, performance, and code reusability.

The primary issue is that the default implementations for protocol requirements
were not playing nicely with each other, especially while compiling with Swift 5.2.

Modifications:

- Change: `init(exactly)` to be `init<T: BinaryInteger)(minorUnits:)` to be more forgiving to the types, and to avoid confusion with `Numeric` initializers
- Change: `init(_:)` to be `init?(amount:)` to properly handle NaN values as well as to avoid `init(floatLiteral:)` accidental usage due to SE-0213
- Change: Code documentation to be more thorough and clear
- Change: `inverse` from a computed property to `negated()`, catching overflow
- Change: Localization from string interpolation is now explicit with `\(localize: <value>)` as the new form, to avoid clashes with the default interpolation behavior and default parameters

Result:

Implementations should be less buggy, Currency should compile in Swift 5+, with happier developers using the library.
  • Loading branch information
Mordil committed Feb 22, 2020
1 parent 8eead5b commit fb2c0a4
Show file tree
Hide file tree
Showing 11 changed files with 384 additions and 361 deletions.
22 changes: 12 additions & 10 deletions Sources/Currency/AnyCurrency+Algorithms.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ extension AnyCurrency {
let fraction = units / count
let remainder = Int(abs(units) % count)

var results: [Self] = .init(repeating: Self(exactly: 0), count: numParts)
var results: [Self] = .init(repeating: .zero, count: numParts)
for index in 0..<remainder {
results[index] = Self(exactly: fraction + units.signum())
results[index] = Self(minorUnits: fraction + units.signum())
}
for index in remainder..<numParts {
results[index] = Self(exactly: fraction)
results[index] = Self(minorUnits: fraction)
}

return results
Expand All @@ -62,15 +62,17 @@ extension AnyCurrency {
///
/// In this case, it is more appropriate to call `distributedEvenly(intoParts:)`.
///
/// - Complexity: O(2*n*), where *n* is the number of `originalValues`.
/// - Complexity: O(*n*), where *n* is the number of `originalValues`.
/// - Parameter originalValues: A collection of values that should be scaled proportionally so that their sum equals this currency's amount.
/// - Returns: A collection of currency values that are scaled proportionally from an original value whose sum equals this currency's amount.
public func distributedProportionally<C: Collection>(
between originalValues: C
) -> [Self] where C.Element == Self {
public func distributedProportionally<T>(
between originalValues: T
) -> [Self]
where T: Collection, T.Element == Self
{
guard originalValues.count > 0 else { return [] }

var results: [Self] = .init(repeating: Self(0), count: originalValues.count)
var results: [Self] = .init(repeating: .zero, count: originalValues.count)

let desiredTotalUnits = self.minorUnits
guard desiredTotalUnits != 0 else { return results }
Expand All @@ -83,14 +85,14 @@ extension AnyCurrency {
defer { index += 1 }

let proportion = Decimal(value.minorUnits) / .init(originalTotalUnits)
let newValue = Self(proportion * self.amount)
let newValue = Self(scalingAndRounding: self.amount * proportion)

defer { currentTotalUnits += newValue.minorUnits }

results[index] = newValue
}

results[originalValues.count - 1] = Self(exactly: desiredTotalUnits - currentTotalUnits)
results[originalValues.count - 1] = Self(minorUnits: desiredTotalUnits - currentTotalUnits)

return results
}
Expand Down
11 changes: 5 additions & 6 deletions Sources/Currency/AnyCurrency+Sequence.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ extension Sequence where Element: AnyCurrency {
/// If the sequence has no elements, you will receive a currency with a value of "0".
/// - Complexity: O(*n*) , where *n* is the length of the sequence.
/// - Returns: A currency value representing the sum total of all the amounts in the sequence.
@inlinable
public func sum() -> Element {
return self.reduce(into: .init(.zero), { $0 += $1 })
return self.reduce(into: .zero, +=)
}

/// Returns the sum total of all amounts in the sequence that satify the given predicate.
/// For example:
///
Expand All @@ -48,12 +47,12 @@ extension Sequence where Element: AnyCurrency {
/// - Returns: A currency value representing the sum total of all the amounts `isIncluded` allowed.
@inlinable
public func sum(where isIncluded: (Element) throws -> Bool) rethrows -> Element {
return try self.reduce(into: Element(0)) { result, next in
return try self.reduce(into: .zero) { result, next in
guard try isIncluded(next) else { return }
result += next
}
}

/// Returns the sum total of amounts in the sequence after applying the provided transform.
///
/// Rather than doing a `.map(_:)` and then `.sum()`, the `sum` result will be calculated inline while applying the transformations.
Expand All @@ -71,6 +70,6 @@ extension Sequence where Element: AnyCurrency {
/// - Returns: A currency value representing the sum total of all the transformed amounts in the sequence.
@inlinable
public func sum(_ transform: (Element) throws -> (Element)) rethrows -> Element {
return try self.reduce(into: .init(.zero)) { $0 += try transform($1) }
return try self.reduce(into: .zero) { $0 += try transform($1) }
}
}
Loading

0 comments on commit fb2c0a4

Please sign in to comment.