Best Practice - Wrapping if around function call vs Adding early exit if guard in function

Question

I know this can be very use-case specific, but I find myself wondering this far too often. Is there a generally preferred syntax.

I'm not asking what is the best approach when in a function, I am asking should I exit early or should I just not call the function.

Wrap if around function call

if (shouldThisRun) {
  runFunction();
}

Have if (guard) in function

runFunction() {
  if (!shouldThisRun) return;
}

The latter option obviously has the potential to reduce code duplication if this function is called multiple times, but sometimes it feels wrong to add it in here as then you may be losing the single-responsibility-ness of the function.

---

Heres an example

If I have an updateStatus() function that simply updates the status of something. I only want the status updated iff the status has changed. I know the places in my code where the status has the potential to change, and I know other places where it definitely has changed.

I'm not sure if its just me but it feels somewhat dirty to check this inside function as I want to keep this function as pure as possible - if I call it I expect the status to be updated. But I can't tell whether it's any better to wrap the call in a check the few places where I know it has the potential to not have changed.

Answer 1

Wrapping an if around a function call:
This is to decide if the function should be called at all, and is part of the decision making process of your program.

Guard clause in function (early return):
This is to protect against being called with invalid parameters

A guard clause used in this way keeps the function "pure" (your term). It exists only to ensure that the function doesn't break with erroneous input data.

The logic about whether to call the function at all is at a higher level of abstraction, even if only just. It should exist outside the function itself. As DocBrown says, you can have an intermediate function that performs this check, to simplify the code.

This is a good question to ask, and falls within the set of questions that lead to recognising levels of abstraction. Each function should operate at a single level of abstraction, and having both the program logic and the function logic in the function feels wrong to you - this is because they are at different levels of abstraction. The function itself is a lower level.

By keeping these seperate, you ensure that your code will be easier to write, read and maintain.

Answer 2

You can have both - a function which does not check parameters, and another one which does, like this (maybe returning some information about if the call was done):

bool tryRunFunction(...)
{
    bool shouldThisRun = /* some logic using data not available inside "runFunction"*/;
    if (shouldThisRun)
        runFunction();
    return shouldThisRun;
}

That way, you can avoid duplicate logic by providing a reusable tryRunFunction and still have your original (maybe pure) function which does not make the check inside.

Note that sometimes you will need a function like tryRunFunction with an integrated check exclusively, so you could integrate the check into runFunction. Or you have no need for reusing the check anywhere in your program again, in which case you can let it stay in the calling function.

However, try to make it transparent to the caller what happens by giving your functions proper names. So callers do not have to guess or look into the implementation if they have to make the checks by themselves, or if the called function already does it for them. A simple prefix like try can often be sufficient for this.

Answer 3

As for who decides whether to run, the answer is, from GRASP, who is the "information expert" that knows.

Once you have decided that, consider renaming the function for clarity.

Something like this, if the function decides:

 ensureUpdated()
 updateIfDirty()

Or, if caller is supposed to decide:

 writeStatus()

Answer 4

I'd like to expand on @Baldrickk's answer.

There's no general answer to your question. It depends on the meaning (contract) of the function to be called and the nature of the condition.

So let's discuss it in the context of your example call updateStatus(). It's contract probably is to update some status because something with an influence on the status has happened. I'd expect calls to that method to be allowed even if there's no real status change, and to be necessary if there's a real change.

So, a calling site can skip a call to updateStatus() if it knows that (inside its domain horizon) nothing relevant changed. That's the situations where the call should be surrounded by an appropriate if construct.

Inside the updateStatus() function, there can be situations where this function detects (from data inside its domain horizon) that there's nothing to do, and that's where it should return early.

So, the questions are:

• From the outside view, when is it allowed / required to call the function, taking into account the contract of the function?
• Inside the function, are there situations where it can return early without any real work?
• Does the condition whether to call the function / whether to return early belong to the internal domain of the function or to the outside?

With an updateStatus() function, I'd expect to see both, calling sites that know nothing has changed, skipping the call, and the implementation checking for "nothing-changed" situations early, even if this way the same condition sometims gets checked twice, both inside and outside.

Answer 5

There are many good explanations. But I want to look unusual way: Assume that you use this way:

if (shouldThisRun) {
   runFunction();
}

runFunction() {
   if (!shouldThisRun) return;
}

And you need to call another function in runFunction method like this:

runFunction() {
   if (!shouldThisRun) return;
   someOtherfunction();
}

What will you do? Do you copy all validations from top to bottom?

someOtherfunction() {
   if (!shouldThisRun) return;
}

I don't think so. Thus, I usually do same approach : Validate inputs and check conditions in public method. Public methods should do its own validations and check required conditions even callers do. But let private methods just do its own business. Some other function may call runFunction without doing any validation or checking any condition.

public someFunction() {
   if (shouldThisRun) {
      runFunction();
   }
}

private runFunction() {
 // do your business.
}

Answer 6

There are two cases: One, you have a function that should either be run or not, depending on the situation. If the function itself cannot decide, the caller must decide.

Two, you have a function that will have to do work or not, depending on the situation. Usually the function will be able to decide itself. And sometimes it is very hard for the caller to decide. In that case, calling the function may be unneeded but not a problem. And in that case, keep the decision making inside the function.