Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Best practice - When to evaluate conditionals of function execution

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.

like image 371
Tesserex Avatar asked Jun 03 '10 13:06

Tesserex


2 Answers

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().

like image 144
zildjohn01 Avatar answered Oct 14 '22 06:10

zildjohn01


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.

like image 2
Nick Dandoulakis Avatar answered Oct 14 '22 06:10

Nick Dandoulakis