5

I'm building a mortgage calculator as an exercise to learn Combine. Everything has been going swimmingly until I encountered a situation where I'm not getting deterministic published output from one of my Publishers when I unit test it. I'm not making any asynchronous calls. This is the problematic AnyPublisher:

public lazy var monthlyPayment: AnyPublisher<Double, Never> = {
    Publishers.CombineLatest3(financedAmount, monthlyRate, numberOfPayments)
        .print("montlyPayment", to: nil)
        .map { financedAmount, monthlyRate, numberOfPayments in
            let numerator = monthlyRate * pow((1 + monthlyRate), Double(numberOfPayments))
            let denominator = pow((1 + monthlyRate), Double(numberOfPayments)) - 1
            
            return financedAmount * (numerator / denominator)
        }
        .eraseToAnyPublisher()
}()

Let's say I change the mortgage type from a 30 year to a 15 year, a few things happen:

  1. the numberOfPayments changes due to the change in the mortgage term (length)
  2. the monthlyRate due to a change in the mortgage term (length)

End Goal

My end goal is to wait for financedAmount, monthlyRate, and numberOfPayments publishers to finish doing their thing and when they're ALL done, THEN compute the monthly payment. Merge 3 seems to pick up changes in each of the publishers and for each change, it computes and spits out output I don't want.

Repo with problematic class and associated unit tests

What I've Tried

I've tried mucking around with MergeMany, Merge3, .collect(), but I can't get the syntax right. I've Googled the snot out of this and looked for examples in public GitHub repos, but I'm coming up with nothing that's germane to my situation. I'm trying to figure out what I'm mucking up and how to fix it.

Supporting Declarations

These are my declarations for the other publishers upon which monthlyPayment relies:

@Published var principalAmount: Double
@Published var mortgageTerm: MortgageTerm = .thirtyYear
@Published var downPaymentAmount: Double = 0.0

// monthlyRate replies upon annualRate, so I'm including annualRate above
internal lazy var monthlyRate: AnyPublisher<Double, Never> = {
  annualRate
      .print("monthlyRate", to: nil)
      .map { rate in
          rate / 12
      }
      .eraseToAnyPublisher()
}()

public lazy var annualRate: AnyPublisher<Double, Never> = {
  $mortgageTerm
      .print("annualRate", to: nil)
      .map { value -> Double in
          switch value {
          case .tenYear:
              return self.rates.tenYearFix
          case .fifteenYear:
              return self.rates.fifteenYearFix
          case .twentyYear:
              return self.rates.twentyYearFix
          case .thirtyYear:
              return self.rates.thirtyYearFix
          }
      }
      .map { $0 * 0.01 }
      .eraseToAnyPublisher()
}()

public lazy var financedAmount: AnyPublisher<Double, Never> = {
  Publishers.CombineLatest($principalAmount, $downPaymentAmount)
      .map { principal, downPayment in
          principal - downPayment
      }
      .eraseToAnyPublisher()
}()

public lazy var numberOfPayments: AnyPublisher<Double, Never> = {
  $mortgageTerm
      .print("numberOfPayments: ", to: nil)
      .map {
          Double($0.rawValue * 12)
      }
      .eraseToAnyPublisher()
}()

Update

I attempted to use Merge3 with .collect(), but my unit test is timing out on it. Here's the updated monthlyPayment declaration:

 public lazy var monthlyPayment: AnyPublisher<Double, Never> = {
     Publishers.Merge3(financedAmount, monthlyRate, numberOfPayments)
         .collect()
         .map { mergedArgs in
             let numerator = mergedArgs[1] * pow((1 + mergedArgs[1]), mergedArgs[2])
             let denominator = pow((1 + mergedArgs[1]), mergedArgs[2]) - 1
             
             return mergedArgs[0] * (numerator / denominator)
         }
         .eraseToAnyPublisher()
 }()

The test now fails with a timeout and the .sink code is never called:

 func testMonthlyPayment() {
     // sut is initialized w/ principalAmount of $100,000 & downPaymentAmount of $20,000
     let sut = calculator
     
     let expectation = expectation(description: #function)
     
     let expectedPayments = [339.62, 433.97, 542.46]
     
     sut.monthlyPayment
         .collect(3)
         .sink { actualMonthlyPayment in
             XCTAssertEqual(actualMonthlyPayment.map { $0.roundTo(places: 2) }, expectedPayments)
             expectation.fulfill()
         }
         .store(in: &subscriptions)
     
     // Initialized with 30 year fix with 20% down
     // Change term to 20 years
     sut.mortgageType = .twentyYear
     
     // Change the financedAmount
     sut.downPaymentAmount.value = 0.0
     
     waitForExpectations(timeout: 5, handler: nil)     
}
Adrian
  • 16,233
  • 18
  • 112
  • 180
  • 1
    I don't see the unit test. Let's make sure you know how to unit test Combine; it isn't trivial. "I'm not making any asynchronous calls" Wrong. Everything about Combine is asynchronous, and only an asynchronous test can test it. Hence my suspicions. You might find my https://stackoverflow.com/questions/66036397/how-to-unittest-combine-cancellables/66056209?r=SearchResults&s=1%7C40.5611#66056209 educational... – matt Sep 22 '21 at 03:58
  • You might also try `zip()`. – Cristik Sep 22 '21 at 15:26
  • @matt Thank you. I'll check against your comments. – Adrian Sep 22 '21 at 17:14
  • @Cristik Thank you. Where would you put `zip()`? – Adrian Sep 22 '21 at 17:14
  • 2
    The problem is that the three signals you are combining are each on their own "timeline". What you need is a signal that says "this value is ready to be calculated". Do all the source values change each time? Then you need a publisher that counts how many values have changed. However if only one or two of the values might change... then you'll need some other signal to say "ok... the values have stabilized and now the monthlyPayment may be calculated. – Scott Thompson Sep 23 '21 at 01:16
  • @ScottThompson Yeah. I was fiddling with marbles and that's the direction I'm going. I think another issue I might bump into is the `@Published` values don't send a signal indicating they're finished, which is a bit of a PITA. Might have to re-think that part, too. – Adrian Sep 23 '21 at 02:28
  • @matt his unit test looks fine. – pronebird Sep 30 '21 at 08:58

1 Answers1

3

The problem is caused by the fact that numberOfPayments and monthlyRate publishers are co-dependent, and both follow the mortgageTerm publisher. Thus, when $mortgageTerm emits an event, you end up with two other independent events emitted by the follower publishers, and this breaks your flow.

This also indicates you're using too many publishers for things that can be easily solved with computed properties, but I assume you want to experiment with publishers, so, let't give it a go with this.

One solution is to use only one publisher for the two problematic pieces of information, a publisher that emits tuples, and which makes use of some helper functions that calculate the data to emit. This way, the two pieces of information that should be emitted at the same time are, well, emitted at the same time :).

func annualRate(mortgageTerm: MortgageTerm) -> Double {
    switch mortgageTerm {
    case .tenYear:
        return rates.tenYearFix
    case .fifteenYear:
        return rates.fifteenYearFix
    case .twentyYear:
        return rates.twentyYearFix
    case .thirtyYear:
        return rates.thirtyYearFix
    }
}
    
func monthlyRate(mortgageTerm: MortgageTerm) -> Double {
    annualRate(mortgageTerm: mortgageTerm) / 12
}
    
func numberOfPayments(mortgageTerm: MortgageTerm) -> Double {
    Double(mortgageTerm.rawValue * 12)
}

lazy var monthlyDetails: AnyPublisher<(monthlyRate: Double, numberOfPayments: Double), Never> = {
    $mortgageTerm
        .map { (monthlyRate: self.monthlyRate(mortgageTerm: $0), numberOfPayments: self.numberOfPayments(mortgageTerm: $0)) }
        .eraseToAnyPublisher()
}()

With the above setup in place, you can use the combineLatest that you attempted first:

func monthlyPayment(financedAmount: Double, monthlyRate: Double, numberOfPayments: Double) -> Double {
    let numerator = monthlyRate * pow((1 + monthlyRate), Double(numberOfPayments))
    let denominator = pow((1 + monthlyRate), Double(numberOfPayments)) - 1
    
    return financedAmount * (numerator / denominator)
}
    
lazy var monthlyPayment: AnyPublisher<Double, Never> = {
    financedAmount.combineLatest(monthlyDetails) { financedAmount, monthlyDetails in
        let (monthlyRate, numberOfPayments) = monthlyDetails
        return self.monthlyPayment(financedAmount: financedAmount,
                                   monthlyRate: monthlyRate,
                                   numberOfPayments: numberOfPayments)
    }
    .eraseToAnyPublisher()
}()

Functions are a powerful tool in Swift (and any other language), as clearly defined and specialized functions help with:

  • code structure
  • redability
  • unit testing

In your particular example, I'd go even one step further, and define this:

func monthlyPayment(principalAmount: Double, downPaymentAmount: Double, mortgageTerm: MortgageTerm) -> Double {
    let financedAmount = principalAmount - downPaymentAmount
    let monthlyRate = self.monthlyRate(mortgageTerm: mortgageTerm)
    let numberOfPayments = self.numberOfPayments(mortgageTerm: mortgageTerm)
    let numerator = monthlyRate * pow((1 + monthlyRate), Double(numberOfPayments))
    let denominator = pow((1 + monthlyRate), Double(numberOfPayments)) - 1

    return financedAmount * (numerator / denominator)
}

The above function clearly describes the problem domain of your screen, as its main feature is to compute a monthly payment based on three inputs. And with the function in place, you can resume the whole set of publishers, to only one:

lazy var monthlyPayment = $principalAmount
    .combineLatest($downPaymentAmount, $mortgageTerm, self.monthlyPayment)

You get the same functionality, but with less amount of, and more testable, code.

Cristik
  • 30,989
  • 25
  • 91
  • 127
  • I like the direction this is going. I'm getting a compiler error on `mergeLatest` in the `monthlyPayment` publisher.`merge(with:)` is an option, so I went with that. Additionally, `Cannot convert value of type 'AnyPublisher<(monthlyRate: Double, numberOfPayments: Double), Never>' to expected argument type 'AnyPublisher'` and `Contextual closure type '(Publishers.MergeMany>.Output) -> Double' (aka '(Double) -> Double') expects 1 argument, but 2 were used in closure body` – Adrian Sep 24 '21 at 09:52
  • @Adrian updated the answer, should compile now – Cristik Sep 24 '21 at 12:07
  • Thanks again. I learned a bit on this one. I got a new hammer and everything looked like a nail with my original implementation. LOL. I think the first go I had with this I tried to do the boring stuff as computed vars, but I never tried using functions. Gonna commit what I have and move onto other stuff, will figure out why that didn't work later. Thanks again :) – Adrian Sep 25 '21 at 22:49
  • @Adrian yeah, functions are a powerful tool, small and specialized functions help a lot with the code structure, readability, and unit testing. Not to mention that it keeps publisher pipelines clean, as everything is forwarded to functions instead of writing large amounts of computation code within the pipeline. I'll try to update my answer with such an example. – Cristik Sep 26 '21 at 09:32
  • @Adrian Just to reinforce this, see my https://www.apeth.com/UnderstandingCombine/tricksandtips.html which explains my style of writing Combine code. The last tip is to move everything off into utility functions so that the pipeline itself is extremely simple. – matt Sep 26 '21 at 11:40
  • @matt Thank you for posting this. Extremely informative and greatly appreciated. – Adrian Sep 26 '21 at 23:58
  • 1
    @Adrian Thanks, I keep meaning to add a chapter on testing but I haven't gotten around to it. :) – matt Sep 27 '21 at 00:13