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.

I run across this problem a lot. For example, I currently write a read function and a write function, and they both check if buf is a NULL pointer and that the mode variable is within certain boundaries.

This is code duplication. This can be solved by moving it into its own function. But should I? This will be a pretty anemic function (doesn't do much), rather localized (so not general purpose), and doesn't stand well on its own (can't figure out what you need it for unless you see where it is used). Another option is to use a macro, but I want to talk about functions in this post.

So, should you use a function for something like this? What are the pros and cons?

share|improve this question
1  
Single line stuff needs more than being used in multiple places to warrant moving into a separate function. –  user1249 Nov 17 '11 at 15:12
1  
Great question. There've been so many times I've wondered the same. –  rdasxy Nov 17 '11 at 20:35
add comment

9 Answers

up vote 28 down vote accepted

This is a great use of functions.

This will be a pretty anemic function (doesn't do much)...

That's good. Functions should only do one thing.

rather localized...

In an OO language, make it private.

(so not general purpose)

If it handles more than one case, it is general purpose. Moreover, generalising isn't the only use of functions. They are indeed there to (1) save you writing the same code more than once, but also (2) to break code up into smaller chunks to make it more readable. It this case it is achieving both (1) and (2). However, even if your function was being called from just one place, it might still help with (2).

and doesn't stand well on its own (can't figure out what you need it for unless you see where it is used).

Come up with a good name for it, and it stands fine on its own. "ValidateFileParameters" or something. Now stands fine on its own.

share|improve this answer
5  
Well goods points. I would add (3) prevent you from hunting for a duplicated bug in your whole code base when you can fix at one place. Code duplication can lead to a maintenance hell pretty fast. –  deadalnix Nov 17 '11 at 10:09
2  
@deadalnix: Good point. As I was writing I realised that my points were a gross over-simplification. Writing and Easier debugging is certainly a benefit of splitting things into functions (as is the ability to unit test separate functions). –  Kramii Nov 17 '11 at 10:30
add comment

Centralizing the code is usually always a good idea. We must re-use the code as much as possible.

However, it is important to note how to do that. For example, when you have a code that does compute_prime_number() or check_if_packet_is_bad() it is good. Chances are that the algorithm of functionality itself will evolve that will be benefited.

However, any piece of code which repeats as a prose doesn't qualify to get centralized right away. This is bad. You may hide arbitrary lines of code inside a function just to hide a code, over time, when multiple parts of the application begins to use, they all must remain compatible to the needs of all callee of the function.

Here are some questions you should ask before asking

  1. Has the function you are creating its own inherent meaning or is it just a bunch of lines?

  2. Which other context will require the use of the same functions? Is it likely that you may require slightly generalize the API before using this?

  3. What will be the expectation of (different parts of the) applications when you throw exceptions?

  4. What are the scenarios to see that functions are going to evolve?

You should also check whether there already exists stuff like this. I have seen so many people always tending to redefine their macros MIN, MAX rather than searching for what already exists.

Essentially, the question is, "Is this new function really worthy of re-use or is this just a copy-paste?" If it is first, it is good to go.

share|improve this answer
1  
Well, if the duplicated code doesn't have it's own meaning, your code is telling you that it need refactoring. Because places where the duplication occurs probably don't have their own meaning either. –  deadalnix Nov 17 '11 at 10:21
add comment

It's almost always good if following conditions are met:

  • improves readability
  • used within limited scope

In larger scope you must carefully weigh duplication vs dependency trade-offs. Examples of scope limiting techniques: hiding in private sections or modules without exposing them public.

share|improve this answer
add comment

DRY is about simplifying code manipulation. You've just touched on a fine point about this principle: It's not about minimizing the number of tokens in your code, but rather creating single points of modification for semantically equivalent code. It sounds like your checks always have the same semantic, so they should be put into a function in case you need to modify them.

share|improve this answer
add comment

Code dupliaction should be avoided. Each time you anticipate it, you should avoid code duplication. If you didn't anticipate it, apply the rule of 3 : refactor before the same piece of code is duplicated 3 times, anotate the quirk when its duplicated 2 times.

What is a code duplicattion ?

  • A routine that is repeated at several places in the codebase. This tourine must be more complex than a simple function call (otherwize, you'll gain nothing by factorizing).
  • A very common task, even a trivial one (often a test). This will improve encapsulation and semantic in the code.

Consider the exemple below :

if(user.getPrileges().contains("admin")) {
    // Do something
}

becomes

if(user.isAdmin()) {
    // Do something
}

You improved the encapsulation (now you can change the conditions to be an admin transparently) and the semantic of the code. If a bug is discovered in the way you check that the user is an admin, you don't need to go throw your whole codebase and make a fix everywhere (whith the risk of forgotting one and get a security flaw in your application).

share|improve this answer
1  
Btw Example illustrates Law of Demeter. –  MaR Nov 17 '11 at 15:08
add comment

It so totally should be a function.

if (isBufferValid(buffer)) {
    // ...
}

Much more readable, and more maintainable (if the check logic ever changes, you only change it in one place).

Besides, things like these get inlined easily so you don't even need to worry about function call overheads.

Let me ask you a better question. How is not doing this a good practice?

Do the right thing. :)

share|improve this answer
2  
If isBufferValid is simply return buffer != null; then I think you're hurting readability there. –  pdr Nov 17 '11 at 10:36
5  
@pdr: In that simple case it only hurts readability if you have a mindset of a control freak and really, really, REALLY want to know how the code checks if the buffer is valid. So it's subjective in those simple cases. –  Spoike Nov 17 '11 at 11:08
3  
@pdr I don't know how it hurts readability by any standard. You're exempting other developers from caring about how you're doing something, and focusing on what you're doing. isBufferValid is definitely more readable (in my book) than buffer != null, because it communicates the purpose more clearly. And again, not to mention it saves you from duplication as well here. What more do you need? –  Yam Marcovic Nov 18 '11 at 18:01
add comment

IMO, code snippets are worth being moved to their own functions whenever this makes the code more readable, regardless of wether the function will be very short or will be used only once.

There are of course limits dictated by common sense. You don't want to have the WriteToConsole(text) method with the body being simply Console.WriteLine(text), for example. But erring in the side of readability is a good practice.

share|improve this answer
add comment

It's generally a good idea to use functions to remove duplication in code.

However it can be taken too far. This is a judgement call.

To take the example of your null buffer check, I'd probably say that the following code is clear enough and shouldn't be extracted into a separate function, even if the same pattern is used in a few places.

if (buf==null) throw new BufferException("Null read buffer!");

If you include the error message as a parameter to a generic null checking function, and also consider the code required to define the function, then it's not a net LOC saving to replace this with:

checkForNullBuffer(buf, "Null read buffer!");

Additionally, having to dive into the function to see what it is doing when debugging means that the function call is less "transparent" to the user, and hence can be considered to be less readable / maintainable.

share|improve this answer
    
Arguably, your exemple says more about the lack of contract support in the language than a real need of duplication. But good points anyway. –  deadalnix Nov 17 '11 at 10:07
1  
You kind of missed the point the way I see it. Not having to step over or have to consider the logic every time you debug is what I'm looking for. If I want to know how it's done, I check it once. Having to do it every time is simply silly. –  Yam Marcovic Nov 17 '11 at 10:16
    
If the error message ("Null read buffer") is repeated, that duplication should definitely be eliminated. –  kevin cline Nov 17 '11 at 14:10
    
Well it's only an example but presumably you'll have a different message at each function call site - e.g. "Null read buffer" and "Null write buffer" for example. Unless you want the same log/error messages for each situation you can't de-duplicate it....... –  mikera Nov 17 '11 at 17:14
    
-1 Preconditions.checkNotNull is a good practice, not a bad one. No need to include the string message though. google-collections.googlecode.com/svn/trunk/javadoc/com/google/… –  ripper234 Nov 20 '11 at 23:31
add comment

If you see it duplicated, you should find a way to centralize it.

Functions are a good way ( maybe not the best but that depends on the language ) Even if the function is anemic as you say that doesn't mean it will stay that way.

What if you have to check for something else as well?

Are you going to find all the places you have to add the extra check or just change a function?

share|improve this answer
    
...on the other hand, what if there's a very high likelihood that you will never add something to it. Is your suggestion still the best course of action in that case as well? –  EpsilonVector Nov 17 '11 at 9:47
1  
@EpsilonVector my rule of thumb is that if I have to change it once then I might as well refactor it. So, in your case I would leave it and if I had to change it, it would become a function. –  Thanos Papathanasiou Nov 17 '11 at 9:53
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.