Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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...

share|improve this question
7  
Unless your code fragment is from a OOP language with implicit receiver (e.g. this) this certainly would be a bad practice, as getCondition() most probably relies on global state ... –  Ingo Sep 12 '11 at 14:38
1  
He may have meant to imply that getCondition() could have arguments? –  user606723 Sep 12 '11 at 16:25
12  
@Ingo -- some things really have Global state. Current time, hostnames, port numbers etc. are all valid "Globals". The design error is making a Global out of something that is inherently not global. –  James Anderson Sep 13 '11 at 1:42
1  
Why don't you just inline getCondition? If it's as small and infrequently used as you say it is then giving it a name is not accomplishing anything. –  davidk01 Sep 13 '11 at 4:22
8  
davidk01: Code readability. –  wjl Sep 13 '11 at 6:02
show 3 more comments

10 Answers 10

up vote 190 down vote accepted

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()));
}
share|improve this answer
29  
+1 on considerations on read-ability of the code. –  aggietech Sep 12 '11 at 14:37
69  
+1. The magic word here is "Self-documenting code". –  Konamiman Sep 12 '11 at 15:17
6  
A good example of what Uncle Bob would call "encapsulating conditionals." –  Anthony Pegram Sep 12 '11 at 18:07
1  
Isn't it better to make this a private method of the taxPayer class, rather than rely on a global taxPayer variable? –  Aditya Sep 12 '11 at 19:03
6  
@Aditya: Nothing says that taxPayer is global in this scenario. Perhaps this class is TaxReturn, and taxPayer is an attribute. –  Adam Robinson Sep 12 '11 at 22:14
show 7 more comments

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.

share|improve this answer
9  
That 0x006A is lovely :D A well known constant of carbonized fuel with additives a, b and c. –  Coder Sep 12 '11 at 15:33
1  
+1 I'm all for self-documenting code :) by the way, I think I agree on keeping a constant abstraction level through blocks, but I couldn't explain why. Would you mind expanding that? –  vemv Sep 12 '11 at 17:18
3  
If you're just saying that petrolFlag |= 0x006A; without any kind of decision making, it would be better to simply say petrolFlag |= A_B_C; without an additional function. Presumably, engageChoke() should only be called if petrolFlag meets a certain criteria and that should clearly say 'I need a function here.' Just a minor nit, this answer is basically spot on otherwise :) –  Tim Post Sep 12 '11 at 18:56
6  
+1 for pointing out that code within a method should be in the same abstraction level. @vemv, it is good practice because it makes the code easier to understand, by avoiding the need for your mind to jump up and down between different levels of abstraction while reading the code. Tying abstraction level switches to method calls/returns (i.e. structural "jumps") is a nice way to make the code more fluent and clean. –  Péter Török Sep 13 '11 at 6:55
6  
No, it's a completely made-up value. But the fact that you wonder about it just emphasizes the point: if the code had said mixLeadedGasoline(), you wouldn't have to! –  Kilian Foth Sep 13 '11 at 8:22
show 2 more comments

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

 bool someConditionSatisfied = [complex expression]; 
This will give hint to code reader and save from introducing new function.

share|improve this answer
1  
The bool is particularly beneficial if the condition function name would be difficult or possibly misleading (e.g. IsEngineReadyUnlessItIsOffOrBusyOrOutOfService). –  dbkk Sep 12 '11 at 20:01
add comment

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.

share|improve this answer
2  
I'm already aware of the benefits of keeping a single edit point - that's why the question says "...called once". Nonetheless it's great to know about those compiler optimizations, I always thought they followed this kind of instructions literally. –  vemv Sep 12 '11 at 17:16
4  
+1 for the lovely edits :-D –  Péter Török Sep 13 '11 at 6:53
add comment

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

share|improve this answer
1  
I'm afraid I don't understand what you meant... –  vemv Sep 12 '11 at 20:28
1  
@vemv: I think he means by "proper level of abstraction" that you don't end up with code that has different levels of abstraction mixed (i.e. what Kilian Foth already said). You don't want your code to handle seemingly insignificant details (e.g. the formation of all bonds in fuel) when the surrounding code is considering a more high-level view of the situation at hand (e.g. running an engine). The former clutters the latter and makes your code less easy to read and maintain as you will have to consider all levels of abstraction at once at any time. –  Egon Sep 12 '11 at 22:02
add comment

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.

share|improve this answer
add comment

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;

    ...
}
share|improve this answer
    
Just curious, what language is that? –  vemv Sep 12 '11 at 20:27
3  
@vemv: Looks like C# to me. –  Scott Mitchell Sep 12 '11 at 20:37
add comment

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.

share|improve this answer
    
According to Stephen's answer this might not always happen (although trusting blindly in compilers' magic can't be good anyway) –  vemv Sep 13 '11 at 5:59
1  
yeah it should get cleared out depending on a lot of facts. If its an interpated language the system will fall for the single line functions everytime unless you install something for cache which still may not do the trick. Now when it comes to compilers it just matters onn the day of the weak and the placment of the planates if the compiler will be your friend and clear out your little mess or if its going to thinkk you really need it. I remeber that if you know the exact amoutn of times a loop will always run its some times better to just copy and paste it that many times then to loop. –  WojonsTech Sep 13 '11 at 6:07
    
+1 for alignment of the planets :) but your last sentence sounds totally nuts to me, do you really do that? –  vemv Sep 13 '11 at 7:41
    
It really depends Most of the time no i do not unless i am getting the correct amount of payment to check if it does speed incress and other things like that. But in older compilers it was better to copy and paste it then to leave the compilere to figure it out 9/10. –  WojonsTech Sep 14 '11 at 0:58
add comment

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 while avoiding clutering the global/package namespace with small throwaway 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 clutering 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]
share|improve this answer
    
Why " lambda p: True if [complicated expression involving p] else False " instead of " lambda p: [complicated expression involving p] " ? 8-) –  hstoerr Sep 13 '11 at 6:42
    
@hstoerr its in the comment right below that line. I like to make it explicitly known that we someCondition is a predicate. While its strictly unnecessary, I write a lot of scientific scripts read by people who don't code that much, I personally think its more appropriate to have the extra terseness rather than have my colleagues confused because they don't know that [] == False or some other similar pythonic equivalence that isn't 'always' intuitive. Its basically a way to flag that someCondition is, in fact, a predicate. –  crasic Sep 13 '11 at 6:56
    
Just to clear my obvious mistake [] != False but [] is False as a when cast to a bool –  crasic Sep 13 '11 at 7:03
    
@crasic: when I don't expect my readers to know that [] evaluates to False, I prefer doing len([complicated expression producing a list]) == 0, rather than using True if [blah] else False which still requires the reader to know that [] evaluates to False. –  Lie Ryan Sep 13 '11 at 7:33
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.