Is there a way to improve this code? (specially how I built the recursion and the breaking out of the loops and conditionals)

# Returns sum of present value of a sequence of cash flows  
# rate = interest rate (month)
# initial amount = balance at the beginning
# *amounts = sequence of cash flows (1 or more)

def present_value_of_series(rate, initial_amount, *amounts)
  present_value = initial_amount
  i = 0
  while i < amounts.length
    present_value += amounts[i] / ( (1+rate)**(i+1) )
    i +=1   
  return present_value

# Returns the IRR (Internal Rate of Return) for a sequence of cash flows
# IRR definition: the IRR is the rate at which the present value of a sequence of cash flows equals zero (or close to zero). It is reached by trial and error, using the steps below:
# 1. Define lower limit and upper limit for the rate - this is the range that will be tested
# 2. Define the rate increment at each iteration
# 3. Calculate the present value of a sequence of cash flows starting at the lower limit and keep incrementing according to defined increment
# 4. When the present value found is below zero, it means that a new lower and upper limit have been found
# 5. Increase calculation precision using a recursive function, this time incrementing at 10x more finer increments.

def irr(lower_guess, higher_guess, increment, *amounts)
  rate = lower_guess
  precision_level = 1
  while rate <= higher_guess
    if present_value_of_series(rate, *amounts) < 0
      if present_value_of_series(rate, *amounts) >= -precision_level
        #debug puts "present value = #{present_value_of_series(rate, *amounts)}"
        puts "rate = #{rate}"
        puts "rate = #{(rate*100).round(2)}% (rounded)"
        # returns annual rate
        annual_rate = (1+rate)**12-1
        puts "annual rate = #{annual_rate}"
        puts "annual rate = #{(annual_rate*100).round(2)}% (rounded)"
        irr((rate-increment), rate, increment/10, *amounts)
      rate += increment

# Usage Example:
# 30000 loan to be paid in 12 installments of different values
# lower_guess = 0.005 (0.5%)
# higher_guess = 0.5 (50%)
# increment = 0.01 (1%)

puts irr(0.005, 0.50, 0.01, -30000.00, 2986.55, 2954.11, 2921.68, 2889.25, 2856.82, 2824.38, 2791.94, 2759.51, 2727.07, 2694.65, 2662.21, 2629.77)
  • Sanity checking
    You should probably add some sanity checks. It's entirely possible to provide a range that'll never produce a result.

  • Separate output from calculation
    Your test code ends up printing an extra blank line because #irr doesn't actually return anything. It'd be nicer to let #irr actually return the rate it found; you can do whatever you want with it afterward. Right now it just prints.

  • Too many splats
    Variadic arguments are neat, but aren't really that necessary here. It'd be simpler and more straightforward to pass an actual array for the amounts.

  • Repetition
    No need to do call present_value_of_series two-three times in a row, like you do here:

    if present_value_of_series(rate, *amounts) < 0
      if present_value_of_series(rate, *amounts) >= -precision_level
        #debug puts "present value = #{present_value_of_series(rate, amounts)}"

    Just stick it in a variable.

  • #present_value_of_series
    This can be simplified by not treating the initial value as a separate argument, and using #reduce to do the sum:

    def present_value_of_series(rate, amounts)
      amounts.each_with_index.reduce(0) do |sum, (amount, index)|
        sum + amount / (rate + 1)**index

    Note that this also matches your other code where you do treat the amounts as an array. No need for * splats.

    As a general note, you rarely need while or for loops when iterating arrays in Ruby. There's almost always a neater way.

  • #irr
    Since you're dealing with a range, it'd be nice to use, well, a Range. A range also gives you a #step iterator, which is useful here.

Here's what I came up with. It works the same as yours (recursion), but I can't say I'm super happy with it - returning from inside the #detect block feel wrong, but it avoids recalculating the present value afterward to check it against -1.

def irr(range, step, amounts)
  rate = range.step(step).detect do |rate|
    present_value = present_value_of_series(rate, amounts)
    if present_value < 0
      return rate if present_value >= -1

  if rate.nil?
    raise "No solution"
    irr((rate-step..rate), step / 10, amounts)

Still needs extra sanity checks, though.

A possible optimization (trading some memory for computation) would be to use Array#bsearch instead of #detect. It does however mean that the range has to be made into an array first (e.g. range.step(step).to_a.bsearch ...).

Speaking of binary searching, you could also do:

def irr(min_rate, max_rate, amounts)
  range = max_rate - min_rate
  raise "No solution" if range <= Float::EPSILON * 2

  rate = range.fdiv(2) + min_rate
  present_value = present_value_of_series(rate, amounts)

  if present_value > 0
    irr(rate, max_rate, amounts)
  elsif present_value < -1
    irr(min_rate, rate, amounts)

Basic idea is that it keeps guessing in the direction of the answer by testing a rate midway between min_rate and max_rate. It gives up if the difference between min_rate and max_rate would introduce floating point precision loss.

It's the fastest solution of the bunch, though it could perhaps be smarter about checking the initial min/max before starting the iteration (it could do that up front by calculating present values for max and min, and only then, if the range makes sense, handle the iteration in separate method). It should probably have a precision argument as well, but that's trivial.


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.