Coding Style – Is It Good Practice to Use Functions to Centralize Common Code?

coding-style

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?

Best Answer

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.

Related Solutions

Coding Style – Best Practices for Setting Return Values

I use both, depending on the method. For example, in a method that decides something, i think that

public boolean isSunday(Calendar cal) {
  if (Calendar.SUNDAY == cal.get(Calendar.DAY_OF_WEEK))
    return true;
  else
    return false;
}

captures the intent of the method better. It is stateless, it is not supposed to be extended, and it is closed in that the if-clause generates all values the method may ever return.

If the method calculates something, or retrieves information, i like the second variant more:

public String getAnswer(int question) {
  if (isAvailable(preciseAnswerService)
    return preciseAnswerService.answer(question);
  return defaultAnswer(question);
}

Here, the fact that one answer is chosen over the other depends on the state of the answerService. We might extend the method later by adding a preciseAnswerCache, or retrieve the answer from somewhere else.

Coding Style – Functions That Only Call Other Functions

This is commonly referred to as functional decomposition and is generally a good thing to do, if done correctly.

Also, the implementation of a function should be within a single level of abstraction. If you take largeReportProcessingFunction, then its role is to define which different processing steps are to be taken in which order. The implementation of each of those steps is on an abstraction layer below and largeReportProcessingFunction shouldn't depend on it directly.

Please note that this however is a bad choice of naming:

void largeReportProcessingFunction() {
    callAllSection1Functions();
    callAllSection2Functions();
    callAllSection3Functions();
    ...        
}

You see callAllSection1Functions is a name that doesn't actually provide an abstraction, because it doesn't actually say what it does, but rather how it does it. It should be called processSection1 instead or whatever is actually meaningful in the context.

Best Answer

Related Solutions

Coding Style – Best Practices for Setting Return Values

Coding Style – Functions That Only Call Other Functions

Related Topic