Style Question: if block in or around function?

Question

Let's say that I have a function that should only execute if some constant is defined. which of the following would be better

option 1: wrap all the function calls in an if block:

if(defined('FOO_BAR_ENABLED')) {
   foobar();
}

I figure this way the intent is more clear, but it requires checking the constant every time the function is called.

option 2: check the constant in the function itself:

function foobar() {
  if(!defined('FOO_BAR_ENABLED')) {
    return;
  }
  //do stuff 
}

This way requires less lines of code, and the constant is sure to get checked. However, I find it confusing to see calls to this function when it's not actually doing anything. Thoughts?

Answer 1

Option 2, less code and it ensures the constant is defined, as you suggested.

Answer 2

Does the condition of the if fall within the function's responsibility? Is there a use case for calling the function without the if?

If the condition always needs to be checked, I'd put it in the function. Follow the DRY principle here: Don't Repeat Yourself. Another quip that might be helpful is the SRP - the Single Responsibility Principle - do one thing, and do it well.

Answer 3

May I suggest renaming the function to FoobarIfEnabled(), then doing the check in the function?

Stealing liberally from a great language-agnostic answer to one of my own questions, when programming we have the following concerns:

1. Make it correct.
2. Make it clear.
3. Make it concise.
4. Make it fast. ... in that order.

If you do the check outside the function, you might end up missing it in one place. And if you want to change the behavior, you'll have to find all the places it gets called and fix it. That's a maintenance nightmare which violates principle 1. By adding "IfEnabled" or something like that to the name, now it is not just correct but also is clear. How can you beat that?

Performance is not to be worried about unless the final speed is unsatisfactory and you have identified this as the bottleneck (unlikely).

I recommend you follow the link above and read as it was a very useful answer that gave me much to think about.

Answer 4

Option 3:

void maybe_foobar() {
   if(defined('FOO_BAR_ENABLED')) really_foobar();
}

void really_foobar() {
    // do stuff
}

On a good day I'd think of better names than "maybe" and "really", but it depends what the function does and why it's turn-off-and-onable.

If there is no circumstance under which anyone could validly "do stuff" when FOO_BAR_ENABLED isn't defined, then I'd go with your option 2 (and perhaps call the function do_stuff_if_possible rather than foobar, if the name foobar was causing confusion as to whether calling it entails actually doing anything). If it's always valid to "do stuff", but some users just so happen do so conditionally, then I'd go with my option 3.

Option 1 is going to result in you copy-and-pasting code around, which is almost always a Bad Sign.

[Edit: here's Option 4, which I suspect is over-engineering, but you never know:

void if_enabled(string str, function f) {
    if (defined(str + '_ENABLED')) f();
}

Then you call it with:

if_enabled('FOO_BAR', foobar);

Obviously there's some issues there to do with how your language handles functions, and whether there's any way to pass arbitrary parameters and a return value through if_enabled.]

Answer 5

In the header file, if foobar always takes the same number of arguments,

#ifdef ENABLE_FOOBAR
#define maybe_foobar(x) foobar(x)
#else
#define maybe_foobar(x)
#endif

Not sure how to do that in C++ or older C dialects if foobar can take a variable number of arguments.

(Just noticed language-agnostic tag. Well, the above technique is what I'd suggest in languages where it works; maybe use an inline function for languages which have those but lack macros).

Answer 6

Since this is apparently only used with the foobar() function, then option 2 should be your choice. That means the test is located in only one place and your code is more readable.