One-line functions that are called only once

Question

Consider a parameterless (edit: not necessarily) function that performs a single line of code, and is called only once in the program (though it is not impossible that it'll be needed again in the future).

It could perform a query, check some values, do something involving regex... anything obscure or "hacky".

The rationale behind this would be to avoid hardly-readable evaluations:

if (getCondition()) {
    // do stuff
}

where getCondition() is the one-line function.

My question is simply: is this a good practice? It seems alright to me but I don't know about the long term...

Answer 1

Depends on that one line. If the line is readable and concise by itself, the function may not be needed. Simplistic example:

void printNewLine() {
  System.out.println();
}

OTOH, if the function gives a good name to a line of code containing e.g. a complex, hard to read expression, it is perfectly justified (to me). Contrived example (broken into multiple lines for readability here):

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

Answer 2

Yes, this can be used to satisfy best practices. For instance, it is better to have a clearly-named function do some work, even if it is only one line long, than to have that line of code within a larger function and need a one-line comment explaining what it does. Also, neighbouring lines of code should perform tasks at the same abstraction level. A counterexample would be something like

startIgnition();
petrolFlag |= 0x006A;
engageChoke();

In this case it is definitely better to move the middle line into a sensibly-named function.

Answer 3

I think that in many cases such a function is good style, but you may consider a local boolean variable as alternative in cases when you don't need to use this condition somewhere in other places e.g.:

bool someConditionSatisfied = [complex expression];

This will give a hint to code reader and save from introducing a new function.

Answer 4

In addition to Peter's answer, if that condition may need to be updated at some point in the future, by having it encapsulated the way you suggest you would only have a single edit point.

Following Peter's example, if this

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

becomes this

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isMutant() 
        && (taxPayer.getNumberOfThumbs() > 2 
        || (taxPayer.getAge() > 123 && taxPayer.getEmployer().isXMan()));
}

You make a single edit and it's updated universally. Maintainability wise, this is a plus.

Performance wise, most optimizing compilers will remove the function call and inline the small code block anyway. Optimizing something like this can actually shrink the block size (by deleting the instructions needed for the function call, return, etc) so it's normally safe to do even in conditions that might otherwise prevent inlining.

Answer 5

In addition to readability (or in complement of it) this allows to write functions at the proper level of abstraction.

Answer 6

It depends. Sometimes it's better to encapsulate an expression in a function/method, even if it's only one line. If it's complicated to read or you need it in multiple places, then I consider it a good practice. In the long term it's easier to maintain, as you've introduced a single point of change and better readability.

However, sometimes it's just something you don't need. When the expression is easy to read anyway and/or just appears in one place, then don't wrap it.

Answer 7

I've done this exact thing just recently in an application that I've been refactoring, to make explicit the actual meaning of the code sans comments:

protected void PaymentButton_Click(object sender, EventArgs e)
    Func<bool> HaveError = () => lblCreditCardError.Text == string.Empty && lblDisclaimer.Text == string.Empty;

    CheckInputs();

    if(HaveError())
        return;

    ...
}

Answer 8

I think if you only have a few of them then it is okay but the issue comes up when there are a lot of them in your code. And when the compiler runs or the interpitor (depending on the language you use) It is going to that function in memory. So lets say you have 3 of them I dont think the computer will notice but if you start having 100's of those little things then the system has to register functions in memory that are only called once and then not destroyed.

Answer 9

If the language supports it, I usually use labeled anonymous functions to accomplish this.

someCondition = lambda p: True if [complicated expression involving p] else False
#I explicitly write the function with a ternary to make it clear this is a a predicate
if (someCondition(p)):
    #do stuff...

IMHO this is a good compromise because it gives you the readability benefit of not having the complicated expression cluttering the if condition while avoiding cluttering the global/package namespace with small throw-away labels. It has the added benefit that the function "definition" is right where its being used making it easy to modify and read the definition.

It doesn't have to only be predicate functions. I like to encase repeated boiler-plate in small functions like this as well (It works particularly well for pythonic list generation without cluttering the bracket syntax). For example, the following oversimplified example when working with PIL in python

#goal - I have a list of PIL Image objects and I want them all as grayscale (uint8) numpy arrays
im_2_arr = lambda im: array(im.convert('L')) 
arr_list = [im_2_arr(image) for image in image_list]

Answer 10

Moving that one line into a well named method makes your code easier to read. Many others have already mentioned that ("self-documenting code"). The other advantage of moving it into a method is that it makes it easier to unit test. When it's isolated into it's own method, and unit tested, you can be sure that if/when a bug is found, it won't be in this method.

Answer 11

There are already a lot of good answers, but there is a special case worth mentioning.

If your one-line statement needs a comment, and you are able to clearly identify (which means: name) its purpose, consider extracting a function while enhancing the comment into API doc. This way you make the function call easier faster and to understand.

Interestingly, the same can be done if there is currently nothing to do, but a comment reminding of expansions needed (in the very near future¹⁾), so this,

def sophisticatedHello():
    # todo set up
    say("hello")
    # todo tear down

could as well changed into this

def sophisticatedHello():
    setUp()
    say("hello")
    tearDown()

---

¹⁾ you should be really sure about this (see YAGNI principle)

Answer 12

Here is a pretty good rule of thumb I use:

Would it be a good (or at least reasonable) idea to add a comment explaining that line? If yes, then refactor to a function with a name that gives the same information as the comment you planned to write.

This approach works perfectly for mathematical code, like

fun distanceSquared(p1, p2) {
    return (p1.x - p2.x)**2 + (p1.y - p2.y)**2;
}
fun distance(p1, p2) {
    return sqrt(distanceSquared(p1, p2));
}

You might wonder about the first function. It's actually quite useful sometimes for performance reasons. For instance, if(distance(p1, p2) > distance(p3, p4)) gives the same result if you switch to distanceSquared, but you get to skip two sqrt calculations. So you might want to write this:

fun distanceCompare(p1, p2, p3, p4) {
    return distanceSquared(p1, p2) - distanceSquared(p3, p4);
}

That will return a negative number if the distance (p1,p2) is smaller than (p3,p4), zero if equal, and a positive number otherwise.