[C/C++] recommended branching in functions?

This topic is 3490 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

Hello all! I'm wondering what is the better way of programming (in the sense of readability, performance, code generation...) the type of functions that do out-of-bound checking and must exit, if the data is indeed out-of-bound: #1
int getElement( const unsigned int id ){
if( id < 0 || id > max_elements )
return SOME_ERROR_NO;

return array[id];
}


or #2
int getElement( const unsigned int id ){
if( id >= 0 && id < max_elements )
return array[id];

return SOME_ERROR_NO;
}


Note: I don't have a specific problem, but I was often wondering what way I should code in the future.. Thank you!

Share on other sites
Whichever makes most sense to you. The compiler may well end up optimising both to the same code anyway, and even if it doesn't, the speed difference will be a couple of clock cycles.

Share on other sites
Well there's definitely something wrong with #1. Take a look at the type of "id" and then look at the first part of the if statement.

Actually...this really applies to both options.

Share on other sites
For readability, the first option is better, especially when you have several tests for error handling. A general recommendation if to test exceptional cases first : if (exceptional) ... else ...

About efficiency, compilers are able to optimize both methods very efficiently anyway, so use what you prefer.

Share on other sites
Your functions don't handle the problem. They leave it to the caller.

1) What happens if you go out of bounds? Assert? Exception? Default value? If so, which default value? Is OOB fatal error? Why? Why not?

2) Who is the caller? Should they expect a failure (file, network I/O), or assume operation succeeds (memory buffer)?

3) Do you need to check? Or can you encapsulate access in such a way that buffer can never overflow, so you have just use a debug-time assert? Network handler, for example, can pass buffer length into recv(), and we'll assume that function will never overflow.

4) Which language? Memory managed languages handle this condition already

5) Why are you checking unsigned int for < 0?

5) As it stands, your accessor functions have almost no added value. You could almost expose the array directly. Why not encapsulate:
bool valid_index(unsigned int id){  return (array) && (id < length); // unsigned, so only 1 test, check if array is valid}bool remove(unsigned int id){  bool valid_index = check_bounds(id);  if (valid_index) {       if (length > 1) array[id] = array[length-1]; // remove the element    length--;                                    // decrease array size  }  return valid_index;}...if (remove(17)) {  // we removed the element}...remove(200); // we don't care if it was removed...if (!remove(42)) {  std::cout << "couldn't remove element";}

Share on other sites
thank you all for commenting, i understand the problem much clearer now!

Share on other sites
I would also recommend that you use only one return statement in a function.

Share on other sites
testing whether an unsigned int is >=0 is pointless....

int getElement(const unsigned id){    return (id < max_elements) ? array[id] : SOME_ERROR_NO;}

Share on other sites
Quote:
 Original post by murdockI would also recommend that you use only one return statement in a function.

why is that?

Share on other sites
Quote:
Original post by Thirthe
Quote:
 Original post by murdockI would also recommend that you use only one return statement in a function.

why is that?

Some people prefer it that way, as they believe it makes the control flow more obvious. I dislike it, the code often ends up being unnecessarily complex trying to avoid using additional return statements. It is a personal style, try it out and see if you like it.

Share on other sites
That's what we were taught in school. It was said to make debugging easier as you would have one entry point and one exit point to a function.

Share on other sites
Quote:
 Original post by murdockThat's what we were taught in school. It was said to make debugging easier as you would have one entry point and one exit point to a function.

It makes thing easier if you're into printf debugging. Any vaguely decent debugger can handle this without much problem.

The only good argument I've heard for single exit functions is that if you have returns in random places you might miss one when modifying the function later and cause a bug because you didn't trace the control flow correctly. However I find that this is really only true for large, poorly constucted in the first place type functions.

I find early-return typically helps readability because you can immediately throw out that branch and not have to keep looking to see if something else happens later.

Share on other sites
Quote:
 Original post by murdockThat's what we were taught in school. It was said to make debugging easier as you would have one entry point and one exit point to a function.
I had a lecturer who said the same thing and criticized the use of early returns and breaks/continues in loops due to the program flow being "hard to follow". Then a few lectures later he introduced exceptions as the one true way of doing error handling, so lets just say that I didn't take his advice too seriously after that..
Frankly I think this is mostly just an extension of goto-phobia in academia.

As for performance the branch prediction in modern processors usually default to backwards branches being taken (i.e. loops) while forward branches aren't. Also the code directly after the branch is more likely to be in the cache. And by default C compilers almost always translate this in the straightforward way (well, I've occasionally seen conditional moves).

However ordering after readability is almost always a better idea as you won't save more than a few measly cycles this way. And techniques like profiler-guided optimizations and GCC's __builtin_expect mechanism are far better ways to deal with this problem than manually shuffling code around.

Actually what might have a real impact on performance is if you can make the compiler keep rarely executed error handling code completely out of the way, i.e. so those code paths normally don't even have to be swapped in from disk or pollute the instruction cache. But again this is what profiler-guided optimizations are for.

Share on other sites
In my experience adhering to the principal of one exit/return per function leads to more bugs than it prevents. I've seen many times where after the error is detected the code ends up continuing on to perform some erroneous action before exiting with the error code.

There are ways around this (error unwinding with gotos seems to be the standard and the canonical example of why goto does have its place) but they all strike me as a little ...ugly. Not to say you shouldn't do it if you like it, just my experience.

Share on other sites

This topic is 3490 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Create an account

Register a new account

• Forum Statistics

• Total Topics
628758
• Total Posts
2984535

• 12
• 25
• 12
• 10
• 17