Which one of these if() statements is best?...

Hi,

I am wondering which one of these is better to use in my code:

Code:

---

//this one:

void handleMouseClick(const int X, const int Y)
{
        if(SelectedObject==NULL)
                return;
               
        SelectedObject->Move(X, Y);
        SelectedObject->Update();
        //...
}

//or this one:

void handleMouseClick(const int X, const int Y)
{
        if(SelectedObject!=NULL)
                {
                        SelectedObject->Move(X, Y);
                        SelectedObject->Update();
                        //...
                }
}

---

They do the same thing. So: Is it totally up to me, or does each of these methods have its pros and cons?

Thanks.

From an optimization perspective.. you should stack the deck against the case you think will occur more frequently.. I like the first option because it provides you an opportunity to break out of the function right away and move on to bigger and better things if necessary.

It depends on if you really care about having a single return point from your function or not. Having extra returns scattered throughout a function can lead to some missteps during debugging, similar to good ole goto.

As mentioned it really depends, but generally I prefer the first one. You get less nesting and the program flow is easier to follow. I also like the exclusion paradigm that you do something, return if it fails, do something, return if it fails etc...

As an example, the classic point-inside-region check, I prefer this way:

Code:

---

bool PointInRegion(Point point, Region region)
{
  if(point.x <  region.left)  return false;
  if(point.x >= region.right)  return false;
  if(point.y <  region.top)    return false;
  if(point.y >= region.bottom) return false;

  return true;
}

---

instead of this:

Code:

---

bool PointInRegion(Point point, Region region)
{
  return (point.x >= region.left)  &&
        (point.x <  region.right) &&
        (point.y >= region.top)  &&
        (point.y <  region.bottom);
}

---

well i personally prefer example 2. Its infinately more readable and concise. Its obvious to any newbie c++ programmer that if the if condition isnt met then the end of the function is reached. A good compiler will generate the same object code for both but the second just looks better.Even with your example magos I still prefer the second example. Ok its not as obvious as mathfans example but again anyone with fair exp. of c++ will be able to read that line with no problem at all and see exactly whats going on.

If prefer the first, for the same reasons as Magos. But then, I never bought into the "don't have multiple return positions" rule.


As you see, it's entirely up to you and the coding standards of your work environment.

In all honesty, the only time the "don't have multiple return positions" rule has come in handy for me was in patching up this little bug Microsoft left for us.
http://ng.csharpfriends.com/top/ng/g...lem/index.aspx

I don't see the bug in this link. C++ boolean values are marshalled as BOOL, not bool. That's annoying if you don't know about it, but probably not a bug as in "oops, made a mistake while coding this".

I prefer multiple return statements, because it's easier to read and easier to debug. There is nothing worse than coming to a function end with a large expression and having to figure out what it might return.

I'm not a fan of the first to be honest, but this is more to do with cleaning up than anything. If you have a function that allocates some memory temporarily, then you need to make sure that you clean it up before the function exits. In this case you'd end up with not just a bunch of return statements, but with a bunch of clean up statements aswell. eg:

Code:

---

int* x = new int[100];
// do stuff
if(!isvalid)
{
  delete [] x;
  return false;
}

// do more stuff
if(!isValid)
{
  delete [] x;
  return false;
}

// .. etc.

---

Yes, I realise the example is bad, bit it should get the point across :)

That's why you should use RAII, TheColonial:

Code:

---

boost::scoped_array<int> x(new int[100]);
if(!isvalid) {
  return false;
}
// do
if(!isvalid) {
  return false;
}
// do
if(!isvalid) {
  return false;
}

---

Quote:

---

Originally Posted by nvoigt

I don't see the bug in this link.

---

Bah...probably because I posted the wrong link. :rolleyes:
http://groups-beta.google.com/group/...d9c1e3015207f0

CornedBee: as I said, not the best example, but it does make the point valid. Yes, there are workarounds for that issue (i'm aware of boost and it's contents), but let's say you're building a DirectX game/application, and adding more and more layers of abstraction on top of something like an index/vertex buffer isn't feasible for performance reason, and you're trying to construct a combination of vertex/index buffers in a single function. First bit fails, second bit doesn't, you then have to release before exiting for fear of memory leaks!?

I'm sure there are arguments either way, at the end of the day, I personally feel that a few levels of nesting make the code easier, and trim down the amount of duplicated code that you may have to write to make sure things are cleaned up.

Cheers :)