Best practice - When to evaluate conditionals of function execution

Question

If I have a function called from a few places, and it requires some condition to be met for anything it does to execute, where should that condition be checked? In my case, it's for drawing - if the mouse button is held down, then execute the drawing logic (this is being done in the mouse movement handler for when you drag.)

Option one says put it in the function so that it's guaranteed to be checked. Abstracted, if you will.

public function Foo() {
    DoThing();
}

private function DoThing() {
    if (!condition) return;
    // do stuff
}

The problem I have with this is that when reading the code of Foo, which may be far away from DoThing, it looks like a bug. The first thought is that the condition isn't being checked.

Option two, then, is to check before calling.

public function Foo() {
    if (condition) DoThing();
}

This reads better, but now you have to worry about checking from everywhere you call it.

Option three is to rename the function to be more descriptive.

public function Foo() {
    DoThingOnlyIfCondition();
}

private function DoThingOnlyIfCondition() {
    if (!condition) return;
    // do stuff
}

Is this the "correct" solution? Or is this going a bit too far? I feel like if everything were like this function names would start to duplicate their code.

About this being subjective: of course it is, and there may not be a right answer, but I think it's still perfectly at home here. Getting advice from better programmers than I is the second best way to learn. Subjective questions are exactly the kind of thing Google can't answer.

Answer 1

According to DRY, I'd go with the first one.

public function Foo() {
    DoThing();
}

private function DoThing() {
    if (!condition) return;
    // do stuff
}

Once you get used to the pattern, it's not so unnerving seeing a lone DoThing() in your code. You'll start to read it like a EnsureThingDone().

Answer 2

I like to check preconditions inside the function,

public function DoThing()
{
    ValidatePreconditions();
    DoWork();
}

private function DoWork()
{
    //Do the actual work;
}

This way i'm certain all the proper preconditions are met before the execution of my function and there's no need for a consumer to add unnecesary code every time my function is called.

Answer 3

You could use the type system. Make the parameter to DoThing an object that you can only instantiate if the preconditions are passed.

A neat-ish way to do this would be to make DoThing an instance method on that object.

Answer 4

Option four, wrap the predicate and the actual call in a 3rd function.

function DoThing() {
    // do stuff
}

function DoThingOnlyIfCondition() {
    if (!condition) return;
    DoThing();
}

function Foo() {
    DoThingOnlyIfCondition();
}

// Foo version 2
function FooBar() {
    DoThing();
}

Now Foo, or whatever function, can use the most appropriate DoXXX() version.