Is it better to guard the method call or the method itself?

Question

I am writing an application and I got to this point:

private void SomeMethod()
{
    if (Settings.GiveApples)
    {
        GiveApples();
    }

    if (Settings.GiveBananas)
    {
        GiveBananas();
    }
}

private void GiveApples()
{
    ...
}

private void GiveBananas()
{
    ...
}

This looks pretty straight-forward. There are some conditions and if they are true the methods are being called. However, I was thinking, is it rather better to do like this:

private void SomeMethod()
{
    GiveApples();
    GiveBananas();
}

private void GiveApples()
{
    if (!Settings.GiveApples)
    {
        return;
    }

    ...
}

private void GiveBananas()
{
    if (!Settings.GiveBananas)
    {
        return;
    }

    ...
}

In the second case, each of the methods guards itself, so even if any of those methods GiveApples or GiveBananas is called from outside SomeMethod, they are going to be executed only if they have the correct flag in Settings.

Is this something that I should actually consider as a problem?

In my current context, it is very unlikely that those two methods will be called from outside this method, but no one can ever guarantee that.

Answer 1

I think of guards as something the method must obey. In your example, the method must not give apples if Settings.GiveApples is false.

If that is the case then the guard definitely belongs inside the method. This prevents you from accidentally calling it from another point in your application without checking the guards first.

On the other hand if the settings only apply to the calling method, and another method somewhere else in your code can GiveApples regardless of the setting then it's not a guard and probably should be in the calling code.

Answer 2

Place the guard within the method itself. The consumer of GiveApples() or GiveBananas() shouldn't be responsible for managing the guards of GiveApples().

From a design standpoint SomeMethod() should only knows that it needs fruit and shouldn't care about what your application needs to do in order to get it. The abstraction of fruit retrieval becomes leaky if SomeMethod() is responsible for knowing that there is a global setting that permits retrieval of certain fruit. This cascades if your guard mechanism ever changes, as now all methods that need to GetApples() or GetBananas() need to be refactored separately to implement this new guard. It's also very easy to forgetfully try and get fruit without that check as you're writing code.

What you should consider in this scenario is how your application should react when Settings does not allow your application to give fruit.

Answer 3

Generally, it is often a good idea to separate the responsibilities of testing something like externally provided settings, and the "core businesss code" like GiveApples. On the other hand, having functions which group together what belongs together is also a good idea. You can accomplish both goals by refactoring your code like this:

private void SomeMethod()
{
    GiveApplesIfActivated();
    GiveBananasIfActivated();
}

private void GiveApplesIfActivated()
{
    if (Settings.GiveApples)
    {
        GiveApples();
    }
}

private void GiveBananasIfActivated()
{
    if (Settings.GiveBananas)
    {
        GiveBananas();
    }
}

private void GiveApples()
{
    ...
}

private void GiveBananas()
{
    ...
}

This gives you a better chance to refactor the code of GiveApples and/or GiveBananas into a separate place without any dependencies from the Settings class. That is obviously beneficial when you want to call those methods in a unit test which does not care for any Settings.

However, if it is always wrong in your program, under any circumstances, even in a testing context, to call something like GiveApples outside a context where Settings.GiveApples is checked first, and you are under impression just providing a function like GiveApples without the Settings check is error-prone, then stick to the variant where you test Settings.GiveApples inside of GiveApples.