12

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.

6
  • Possible duplicate of Should I return from a function early or use an if statement?
    – gnat
    Mar 11, 2019 at 11:20
  • 3
    @gnat No, that question is essentially 'What is the preferred syntax in an early exit' whereas mine is 'Should I exit early or should i just not call the function' Mar 11, 2019 at 11:24
  • 4
    you can't trust developers, even yourself, to correctly check the pre-conditions of the function everywhere that it's called. for this reason, I'd suggest that the function validate internally any necessary conditions if it has the ability to do so.
    – TZHX
    Mar 11, 2019 at 12:10
  • "I only want the status updated iff the status has changed" - you want a status updated (=changed) if the same status has changed? Sounds pretty circular. Can you please clarify what you mean by this, so I can add a meaningful example to my answer on this?
    – Doc Brown
    Mar 12, 2019 at 6:00
  • @DocBrown Lets for instance say I want to keep two different objects status properties in sync. When either object changes I call syncStatuses() - but this could be triggered for many different field changes (not only the status field). Mar 12, 2019 at 7:31

6 Answers 6

18

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.

2
  • Awesome answer. I like the fact it gives me a clear way to think about this. The if should be outside as it is 'part of the decision making process' as to whether the function should be called in the first place. And inherently has nothing to do with the function itself. It feels weird to mark an opinion answer as correct, but I'll have a look again in few hours and do so. Mar 12, 2019 at 7:36
  • If it helps, I don't regard this as an "opinion" answer. I note that it "feels" wrong, but that's because the different levels of abstraction are not separate. What I got from your question is that you can see that it isn't 'right' but because you're not thinking about levels of abstraction it is hard to quantify, hence it's something you struggle to put into words.
    – Baldrickk
    Mar 12, 2019 at 14:17
7

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.

3
  • 1
    Have to admit, the "tryXXX()" idiom always seemed a bit off and it's inappropriate here. You aren't trying to do something expecting a likely error. You are updating if it is dirty.
    – user949300
    Mar 12, 2019 at 2:37
  • 1
    @user949300: picking a good name or naming scheme depends on the real use case, the real function names, not some contrived name like runFunction. A function like updateStatus() may be accompanied by another function like updateIfStatusHasChanged(). But this is 100% case dependend, there is no "one-size-fits-all" solution to this, so yes, I agree, the "try" idiom is not always a good choice.
    – Doc Brown
    Mar 12, 2019 at 5:54
  • There's not something named "dryRun"? More or less comes to be a regular execution without side effects. How to disable the side effects is another story
    – Laiv
    Mar 12, 2019 at 12:33
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()
2

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.

2

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.
}
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.