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.

Whenever I find myself writing the same logic more than once, I usually stick it in a function so there is only one place in my application I have to maintain that logic. A side effect is that I sometimes end up with one or two line functions such as:

function conditionMet(){
   return x == condition;
}

OR

function runCallback(callback){
   if($.isFunction(callback))
     callback();
}

Is this lazy or a bad practice? I only ask because this results in a greater number of function calls for very tiny pieces of logic.

share|improve this question
3  
No, not too short. In C# I would do 'condition met' as a property, which is elegant, and would consider using Assert.AreEqual<int>(expected, actual, message, arg1, arg2, arg3, ...);. The second one is fine as is. I would potentially include an optional bool flag which would dictate whether to throw an exception/etc. in case the callback is not a function. –  Job Apr 1 '11 at 20:04
35  
Yes, only in one case: function myFunction() {} –  Spooks Apr 1 '11 at 20:43
3  
def yes(): return 'yes' –  dietbuddha Apr 2 '11 at 1:03
3  
@Spooks - I'm splitting hairs here but empty functions are valid for at least adapter classes e.g. MouseMotionAdapter in download.oracle.com/javase/6/docs/api/java/awt/event/… . Of course this only a way to work around language limitations. –  Aleksi Yrttiaho Apr 2 '11 at 6:55
2  
@dietbuddha: The requirements just changed - now it has to support two languages for a demo next week. The guy who wrote "def yes()" felt very glad he had before changing it to "def yes( : (IsFrench() ? 'Oui' : 'Yes')" –  Andrew Shepherd Apr 6 '11 at 10:57

18 Answers 18

up vote 151 down vote accepted

Hehe, oh Mr Brown, if only I could persuade all the developers I meet to keep their functions as small as this, believe me, the software world would be a better place!

1) Your code readability increases ten fold.

2) So easy to figure out the process of your code because of the readability.

3) DRY - Don't Repeat Yourself - You're conforming to this very well!

4) Testable. Tiny functions are a million times easier to test than those 200 line methods we see way too often.

Oh and don't worry about "function hopping" in terms of performance. "Release" builds and compiler optimisations take care of this for us very nicely, and performance is 99% of the time somewhere else in the systems design.

Is this lazy? - Very much the opposite!

Is this bad practice? - Absolutely not. Better to pulled this way of making methods than the tar balls or "God Objects" that are oh so too common.

Keep up the good work my fellow craftsman ;)

share|improve this answer
29  
You're not answering the question, you're just preaching the rule whose more extreme applications are questioned here. –  delnan Apr 1 '11 at 19:29
3  
Is my answer really hard to tell? Here, I'll make an edit.. –  Martin Blore Apr 1 '11 at 19:31
4  
It's not very often that I find the limitation of just 1 upvote frustrating. But, for this answer, +1 doesn't seem to do it justice. –  Bevan Apr 1 '11 at 19:52
8  
Very likable answer, but I disagree with most of it: 1) Extremely small functions result in more code overall, because of all the plumbing necessary for each function. This may reduce readability, especially for a really big project. Also, language dependent. 2) Depends entirely on point 1, and therefore has nothing to do with the question. 3) What? runCallback repeats "callback" four times, and three times refers to the same variable to run a single function. 4) 200+ line methods are of course not testable, but there's a long way from that to one line. –  l0b0 Jun 9 '11 at 14:04
7  
-1: The most scarying thing here is the number of up-votes given to this answer. –  Luca Jun 6 '12 at 21:39

Can a function be too short? In general no.

In fact the only way to ensure that:

  1. You have found all the classes in your design
  2. Your functions are doing only one thing.

Is to keep your functions as small as possible. Or, in other words, extract functions from your functions until you can't extract any more. I call this "Extract till you drop."

To explain this: A function is a scope with chunks of functionality that communicate by variables. A class is also a scope with chunks of functionality that communicate by variables. So a long function can always be replaced by one or more classes with small method.

Also, a function that is big enough to allow you to extract another function from it, is doing more than one thing by definition. So if you can extract a function from another, you should extract that function.

Some folks worry that this will lead to a proliferation of functions. They're right. It will. That's actually a good thing. It's good because functions have names. If you are careful about choosing good names, then these functions act as sign posts that direct other people through your code. Indeed, well named functions inside of well named classes inside of well named namespaces are one of the best ways to make sure that your readers do NOT get lost.

There's a lot more about this in Episode III of Clean Code at cleancoders.com

share|improve this answer
5  
Uncle Bob! Great to see you on board here. As expected, fantastic advice. I've never heard the term "Extract till you drop" before and will certainty be using this term around the office on Monday ;). –  Martin Blore Apr 2 '11 at 0:40

I would say that a refactored method is too short if either:

  • It duplicates a primitive operation, for no other purpose than to make it a method:

Ex:

boolean isNotValue() {
   return !isValue();
}

or...

  • The code is only used once, and its intent is easy to understand at a glance.

Ex:

void showDialog() {
    Dialog singleUseDialog = new ModalDialog();
    configureDialog(singleUseDialog);
    singleUseDialog.show();
}

void configureDialog(Dialog singleUseDialog) {
    singleUseDialog.setDimensions(400, 300);
}

This could be a valid pattern, but I would just inline the configureDialog() method, in this example, unless I intended to override it or reuse this code elsewhere.

share|improve this answer
6  
Separating a single line method in the second case can be beneficial to keep the calling methods abstraction level consistent. The example provided is on the fence if this reasoning is used: is exact pixel width and height too much detail for showing a dialog? –  Aleksi Yrttiaho Apr 2 '11 at 5:34
2  
The "isNotValue()" method is badly named and should be refactored to name the actual domain question. –  user1249 Apr 2 '11 at 12:34
4  
@Aleksi: I disagree; the details are already hidden within the showDialog() method in this example; there's no need to double-refactor it. The example is intended to show an isolated use case; if a pattern is needed with different dialog configurations in different cases, it would make sense to go back and refactor once the pattern is established. –  RMorrisey Apr 4 '11 at 22:30
1  
@Thorbjorn: I could see a case for it, when you're answering a business domain question whose logic might not be obvious. I'm just pointing out that there are -some- cases where it's overkill. –  RMorrisey Apr 4 '11 at 22:32
3  
Maybe this is nitpicking, but i would argue that in your examples the problem is not that the function is too short, but that they do not add descriptive value. –  keppla Jun 9 '11 at 15:12

Wow, most of these answers aren't very helpful at all.

No function should be written whose identity is its definition. That is, if the function name is simply the function's code block written out in English, then don't write it as a function.

Consider your function conditionMet and this other function, addOne (forgive me for my rusty JavaScript):

function conditionMet() { return x == condition; }

function addOne(x) { return x + 1; }

conditionMet is a proper conceptual definition; addOne is a tautology. conditionMet is good because you don't know what conditionMet entails just by saying "condition met", but you can see why addOne is silly if you read it out in English:

"For the condition to be met is for x to equal condition" <-- explanatory! meaningful! useful!

"To add one to x is to take x and add one." <-- wtf!

For the love of anything that might still be holy, please, don't write tautological functions!

(And for the same reason, don't write comments for every line of code!)

share|improve this answer
3  
Agreed, what exactly should be made into a function would depend largely on the language it's being written in. A function like addOne would be useful if you were in a language like VHDL, where you're actually defining the meaning of adding one on a binary or number theory level. I'd like to think that no language out there is so cryptic that it's hard to read even for practiced programmers (maybe brainf#ck), but if that's the case, the same idea would stand since the function is effectively a translation from English to that language -- so the name isn't the same as the definition. –  Rei Miyasaka Apr 2 '11 at 21:03
1  
I'm talking about things identity function from the Haskell standard library - I don't think you can get more "tautological" then that :) –  hugomg Nov 12 '11 at 3:22
1  
@missingno Bah, that's cheating :D –  Rei Miyasaka Nov 12 '11 at 4:13
3  
If you name the functions by their purpose instead of by their content, they immediately stop being silly. So your examples might be e.g. be function nametoolong() { return name.size > 15; } or function successorIndex(x) { return x + 1; } So the problem with your functions is actually just that they their name is bad. –  hstoerr Jan 2 '12 at 19:06

I'd say that if you think the intention of some code can be improved by adding a comment, then rather than adding that comment, extract the code into its own method. No matter how small the code was.

So for example if your code was going to look like:

if x == 1 { ... } // is pixel on?

make it look like this instead:

if pixelOn() { ... }

with

function pixelOn() { return x == 1; }

Or in other words, it's not about the method length, but about self-documenting code.

share|improve this answer
3  
My esteemed colleague points out that you could also write if x == PIXEL_ON. Using a constant can be as descriptive as using a method, and is a little briefer. –  Tom Anderson Jun 9 '11 at 15:28

I think this is exactly what you want to do. Right now that function might only be one or two lines but over time it could grow. Also having more function calls allows you to read the function calls and understand what is happening inside there. This makes your code very DRY (Don't Repeat Yourself) which is much more maintainable.

share|improve this answer
3  
So what's wrong with factoring it out later, when you can prove you need to, instead of now, when it's just dead weight? –  dsimcha Apr 2 '11 at 20:03

I agree with all the other posts I have seen. This is good style.

The overhead of such a small method may be nil as the optimizer may optimize the the call away and inline the code. Simple code like this is allows the optimizer to do its best work.

Code should be written for clarity and simplicity. I try to limit a method to one of two roles: making decisions; or performing work. This may generate one line methods. The better I am at doing this, the better I find my code is.

Code like this tends to have high cohesion and low coupling which is good coding practice.

EDIT: A note on method names. Use a method name which indicates what the method does not how it does it. I find verb_noun(_modifier) is a good naming scheme. This give names like Find_Customer_ByName rather than Select_Customer_Using_NameIdx. The second case is prone to become incorrect when the method is modified. In the first case, you can swap out the entire Customer database implementation.

share|improve this answer

Refactoring one line of code into a function seems excessive. There might be exceptional cases, such as ver loooooong/comples lines or expessions, but I wouldn't do this unless I know the function will grow in the future.

and your first example hints at use of globals (which may or may not speak of other issues in the code), I'd refactor it further, and make those two variables as parameters:

function conditionMet(x, condition){
   return x == condition;
}
....
conditionMet(1,(3-2));
conditionMet("abc","abc");

The conditionMet example might be useful if the condition was long and repetitive such as:

function conditionMet(x, someObject){
   return x == ((someObject.valA + someObject.valB - 15.4) / /*...whole bunch of other stuff...*/);
}
share|improve this answer
1  
His first example does not hint of globals. If anything, it's a cohesive method in a highly cohesive class where most of the variables are on the object. –  Martin Blore Apr 1 '11 at 19:34
6  
That conditionMet function is just a verbose == operator. That's propably not useful. –  delnan Apr 1 '11 at 19:34
1  
I definitely don't use globals. @MeshMan, thats exactly where the example is from...a method on a class. –  Mark Brown Apr 1 '11 at 19:36
1  
@Mark Brown: @MeshMan: Ok, I guess the code example was too vague to know for sure... –  FrustratedWithFormsDesigner Apr 1 '11 at 19:40

Consider this:

A simple collision detection function:

bool collide(OBJ a, OBJ b)
{
    return(pow(a.x - b.x, 2) + pow(a.y - b.y, 2) <= pow(a.radius + b.radius, 2));
}

If you wrote that "simple" one liner in your code all the time, you might eventually make a mistake. Plus, it'd be really torturous to write that over and over.

share|improve this answer

No, and that is rarely a problem. Now if someone feels no function should be longer than one line of code (if only it could be that simple), that would be a problem and in some ways lazy because they are not thinking about what is appropriate.

share|improve this answer

I would say they are too short, but this is my subjective opinion.

Because:

  • There is no reason to create a function if it's used only once or twice. Jumping to defs suck. Especially with amazingly fast VS and C++ code.
  • Class overview. When you have thousands of small functions, it drives me angry. I enjoy when I can view class definitions and quickly see what it does, not how it SetXToOne, SetYToVector3, MultiplyNumbers, + 100 setters/getters.
  • In most projects these helpers become dead weight after one ore two refactoring phases, and then you do "search all"->delete to get rid of obsolete code, usually ~25%+ of it.

Functions that are long are bad, but functions that are shorter than 3 lines and perform only 1 thing are equally bad IMHO.

So I'd say only write small function if it's:

  • 3+ lines of code
  • Does stuff that junior devs might miss (not know)
  • Does extra validation
  • Is used, or will used at least 3x
  • Simplifies frequently used interface
  • Will not become a dead weight during next refactoring
  • Has some special meaning, for example, template specialization or something
  • Does some isolation job - const references, affects mutable parameters, does private member retrieval

I bet next developer (senior) will have better things to do than to remember all your SetXToOne functions. So they'll turn into dead weight pretty soon either way.

share|improve this answer

There's no code like no code!

Keep it simple and don't over complicate things.

It's not being lazy, it's doing your job!

share|improve this answer

I don't like the example no. 1, bcause of ith generic name.

conditionMet does not seem to be generic, so it stands for a specific condition? Like

isAdult () = { 
  age >= 18 
}

This would be fine. It's a semantic difference, while

isAtLeast18 () { age >= 18; } 

would not be fine for me.

Maybe it is often used, and can be subject for later change:

getPreferredIcecream () { return List ("banana", "choclate", "vanilla", "walnut") }

is fine too. Using it multiple times, you just need to change a single place, if you have to - maybe whipped cream gets possible tomorrow.

isXYZ (Foo foo) { foo.x > 15 && foo.y < foo.x * 2 }

is not atomic, and should give you a nice test opportunity.

If you need to pass a function, of course, pass whatever you like, and write otherwise silly looking functions.

But in general, I see much more functions, which are too long, than functions which are too short.

A last word: Some functions only look appropriate, because they're written too verbose:

function lessThan (a, b) {
  if (a < b) return true else return false; 
}

If you see, that it is the same as

return (a < b); 

you won't have a problem with

localLessThan = (a < b); 

instead of

localLessThan = lessThan (a, b); 
share|improve this answer

Too short never is a problem. Some reasons for short functions are:

Reusability

E.g. if you have a function, like a set method, you might assert it to be sure the parameters are valid. This checking has to be done everywhere the variable is set.

Maintainability

You might use some statement that you think in the future may change. E.g. you now show a symbol in a column, but later that might change to something else (or even a beep).

Uniformity

You are using e.g. the facade pattern and the only thing a function does is exactly passing the function to another without changing arguments.

share|improve this answer

Yes, its ok to have a short code function. In case of methods as "getters", "setters", "accesors" is very common, as previous answers mentioned.

Sometimes, those short "accesors" functions are virtual, because when overriden in subclasses the functions will have more code.

If you want you function not so short, well, in many functions, wheter global or methods, I usually use a "result" variable (pascal style) instead of a direct return, its very helpful when using a debugger.

function int CalculateSomething() {
  int Result = -1;

   // more code, maybe, maybe not

  return Result;
}
share|improve this answer

When you give a code section a name, it is essentially for giving it a name. This can be for any of several reasons, but the crucial point is if you can give it a meaningful name which adds to your program. Names like "addOneToCounter" would not add anything, but conditionMet() would.

If you need a rule for finding out if the snippet is too short or too long, then consider how long time it takes you to find a meaningful name for the snippet. If you cannot within a reasonable amount of time, then the snippet is not a suitable size.

share|improve this answer

No, but it can be too terse.

Remember: Code is written once, but read many times.

Don't write code for the compiler. Write it for future developers who will have to maintain your code.

share|improve this answer

I disagree with almost answer given at this time.

Recently I found a colleague which write all classes members like the following:

 void setProperty(int value){ mValue=value; }
 int getProperty() const { return (mValue); }

This could be a good code in sporadic cases, but surely not if you define many classes having many and many properties. I don't want to say, with this, that methods like the ones above shall not be written. But, if you ends up to write most of the code as "wrapper" of the real logic, there is something wrong.

Probably the programmer miss the overall logic, fear about future changes and about refactoring the code.

The definition of the interface is because a real need; indeed if you need to set and get a simple property (without much logic, which makes the member shorter) it shall be possible. But, if the real need could be analysed in a different way, why don't define a more intuitive interface?

To really answer to the question, of course it is no, and the reason is very obvious: the method/property/whatelse shall be defined for what it needs. Even an empty virtual function has a reason to exists.

share|improve this answer
6  
There is a good reason to add accessor/mutator methods, which you hint at under "fear about future ... refactoring". But you're also right that it doesn't add value to the code. Neither by reducing size (locally or globally) or increasing readability. In my mind, this speaks to poor language design in both Java and the JavaBeans convention. This is one area (of many) where C# successfully improved the language to address both concerns with the automatic property syntax. As a Java programmer, I agree with your raw style for basic struct-like classes. –  Anm in LA Apr 1 '11 at 22:52
1  
Getter/setter functions are good even if all they do right now is read or write a variable. As a practical matter, you can imagine a scenario where later on you decide to print a value to the screen every time the field changes, or to check whether that value is valid or not. (Maybe it's a denominator for a fraction, and you want to check to make sure it's not zero.) –  Rei Miyasaka Apr 2 '11 at 7:33
2  
getters and setters are awful, their valid use notwithstanding. If it's necessary to use them, I consider it a failure of the language. –  Carson Myers Apr 3 '11 at 7:25

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.