Tail-recursive factorial

Question

Is there anything what could be improved on this code?

def factorial(n: Int, offset: Int = 1): Int = {
    if(n == 0) offset else factorial(n - 1, (offset * n))
}

The idea is to have tail-recursive version of factorial in Scala, without need to define internal function or alias.

This one is callable like this:

factorial(4)

So final solution looks like this:

import scala.annotation._

@tailrec 
def factorial(n: Int, accumulator: Long = 1): Long = {
    if(n == 0) accumulator else factorial(n - 1, (accumulator * n))
}

Answer 1

Looks ok in its basic form, but here are some points to consider:

1. You may want to consider using at least Long, as factorials tend to get large quickly.

2. Whenever you write a function that you believe to be tail-recursive, then do add @tailrec (from scala.annotations) to it. This has two major advantages: First, it corrects your thoughts and tells you immediately, if it isn't tail-recursive although you thought so, and second, it tells everyone else and stops them from making a "fix" that causes the function to no longer be tail-recursive. May not be a big deal for such a small one, but in general tail-recursive functions can be more complex and then it's much harder to see at a glance that the function is tail-recursive if you do not annotate it as such.

3. Naming convention: The semantics of what you labelled offset is not really an offset as we would usually think about it. If you say offset most people have a certain meaning in mind that they associate with this term. The traditional meaning that resembles your semantics would be given by the term accumulator. Especially, when writing recursive functions we run into this intermediate-result-variable-thing very often and you will almost always see it referred to as an accumulator, so for clarity's sake you should just name the variable as such.