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:

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.
–
JobApr 1 '11 at 20:04

@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 ShepherdApr 6 '11 at 10:57

18 Answers
18

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.

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.
–
BevanApr 1 '11 at 19:52

9

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.
–
l0b0Jun 9 '11 at 14:04

7

-1: The most scarying thing here is the number of up-votes given to this answer.
–
LucaJun 6 '12 at 21:39

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

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 BloreApr 2 '11 at 0:40

1

Would you be willing to elaborate on your point #1? Explaining that by example would be great.
–
Daniel KaplanFeb 7 '13 at 23:11

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 YrttiahoApr 2 '11 at 5:34

2

The "isNotValue()" method is badly named and should be refactored to name the actual domain question.
–
user1249Apr 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.
–
RMorriseyApr 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.
–
RMorriseyApr 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.
–
kepplaJun 9 '11 at 15:12

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!

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 MiyasakaApr 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 :)
–
hugomgNov 12 '11 at 3:22

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.
–
hstoerrJan 2 '12 at 19:06

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.

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.

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:

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.

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

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.

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.

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.

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.
–
AnmApr 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 MiyasakaApr 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 MyersApr 3 '11 at 7:25