Calculating IRR (Internal Rate of Return) in Ruby using recursion

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 = -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)"
        break
      else
        irr((rate-increment), rate, increment/10, *amounts)
        break
      end
    else
      rate += increment
    end
  end
end

# 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) = -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
  end
end

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 = -1
      true
    end
  end

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

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  0
    irr(rate, max_rate, amounts)
  elsif present_value < -1
    irr(min_rate, rate, amounts)
  else
    rate
  end
end

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.

StackExchange Code Review Q#92508, answer score: 3