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.
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...*/);
}
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.
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 ;)
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.
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.
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.
Can a function be too short? In general no.
In fact the only way to ensure that:
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
I would say that a refactored method is too short if either:
Ex:
boolean isNotValue() {
return !isValue();
}
or...
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.
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!
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!)
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.
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;
}
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.
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);
I would say they are too short, but this is my subjective opinion.
Because:
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:
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.
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.
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.
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.
Yes dear function could be smaller and smaller:
data => console.log(data)
is an anonymous function in ES 2017 JavaScript and Typescript
getMarketSegments() {
this.marketService.getAllSegments(this.project.id)
.subscribe(data => this.segments = data, error => console.log(error.toString()));
}
in the above code you can see 3 Function declaration and 2 function calls this is a simple Service Call in Angular 4 with Typescript
([] 0)
([x] 1)
([x y] 2)
The above are 3 Anonymous functions in clojure Language
(def hello (fn [] "Hello world"))
The above one is a functional declaration in clojure
So yes FUNCTIONS can be as smaller as you want them to be.