Which of these functions is the most proper in terms style and cleanliness?

Question

I'm having an internal dispute about the right way to do something. Maybe you can help :)

Let's say I have the functions

foo(x, y) 
{  
  if (x satisfies condition c1)
    if (y satisfies condition c2)
      bar(x)
  else
    bar(x)

  ...
}

bar(x)
{
  ...
}

An alternative to this would be

foo(x, y) 
{ 
  doBarFlag = false
  if (x satisfies condition c1)
    if (y satisfies condition c2)
      doBarFlag = true
  else
    doBarFlag = true

  if (doBarFlag)
  {
    ...code from bar() goes in here
  }

  ...
}

And another alternative (slight variation of above)

foo(x, y) 
{ 
  doBarFlag = true
  if (x satisfies condition c1)
    if (y DOES NOT satisfy condition c2)
      doBarFlag = false

  if (doBarFlag)
  {
    ...code from bar() goes in here
  }

  ...
}

Assume that bar() is less than ten lines. Which style would you prefer? Why? I'm leaning toward the first example, but I'd like to know what others think.

Answer 1

First of all, one of the reasons for defining functions is to be able to refer to (and not have to repeat) a bunch of code, using a short name. Hence, using that short name repeatedly isn't necessarily a bad thing. Thus, your option #1 is perfectly reasonable, and the least redundant.

Second of all, if that's really all that's going on in foo(), why not just do this instead?

foo(x, y) 
{  
  if (x satisfies condition c1) && !(y satisfies condition c2)
    return

  bar(x)
}

or even just

foo(x, y) 
{  
  if !(x satisfies condition c1) || (y satisfies condition c2)
    bar(x) 
}

In most languages, the && and || operators (or their equivalents) are short-circuited, and thus won't try to evaluate their right-hand side if they can determine their truth value from just the left. As such, you often don't need to worry about things like "property compared in y only exists if x is true" or the like.

Answer 2

How about

foo( x, y ) {
  if ( ! c1(x) || c2(y) )
    bar(x)
}

There's a name for this logic transformation, which I forget…

Answer 3

The other answers have good suggestions on how to improve the code, I just have a styling suggestion. I would have braces on all code blocks regardless of length:

// C# style
foo(x, y) 
{ 
    if (condition) 
    {
        // do something
    }
    else 
    {
        // do something else
    }
}

// What I normally use
foo(x, y) { 
    if (condition) {
        // do something
    } else {
        // do something else
    }
}

That way if you want to add another statement later it won't be added in the wrong place. But don't get too religious on styling, just be consistent with whatever you use.

Answer 4

how about

foo(x,y)
{
    if(x satisfies c1 && y does not satisfy c2)
        return;
    bar(x);
}

this way you can easily see that when bar(x) is not executed.