Is it good practice to use functions just to centralize common code?

Question

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?

Answer 1

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.

Answer 2

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

Answer 3

IMO, code snippets are worth being moved to their own functions whenever this makes the code more readable, regardless of whether 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.

Answer 4

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.

Answer 5

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.

Answer 6

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

Answer 7

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.

Answer 8

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?

Answer 9

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.