Using local variables vs checking against function return directly.

Question

I have a function definition, where i call multiple functions. Even if one of the function fails i need to go ahead and call the rest of the functions and finally return a single error saying whether any of the function call failed. The approach which i had followed was

int function foo()

{


    int res, res1, res2, res3;

    res1 = function1();
    res2 = function2();
    res3 = function3();

    if (res1 == -1 || res2 == -1 || res3 == -1)
    {
         res = -1;
    }

    return res;
}

The possible another approach is

int function foo()

{


   int res;

   if (function1() == -1)
   {
        res = -1;
   }

   if (function2() == -1)
   {
        res = -1;
   }

   if (function3() == -1)
   {
       res = -1;
   } 

   return res;
}

Which is a better approach? Thanks in advance.

Answer 1

What about:

int foo ()
{
    bool failed = false;
    failed |= (function1() != 0);
    failed |= (function2() != 0);
    failed |= (function3() != 0);
    return failed? -1 : 0;
}

You could also collapse the three calls into a single expression and omit the failed variable altogether (at the expense of readability):

int foo ()
{
    return ((function1() != 0) | (function2() !=0 ) | (function3() != 0))? -1 : 0;
}

I like the first approach when function1 function2 and function3 have the same signature because I can put them in a function pointer table and loop over the entries, which makes adding function4 alot easier.

Answer 2

No difference at all, both will be optimized to same machine code. Preference, maintainability, and that depends on team guidelines, preferences.

Answer 3

If you can define any precise convention about return values you can simply use bitwise or:

int foo() {
  if (function1() | function2() | function3())
    return -1;
  else
    return 0;
}

Answer 4

I like the second approach better. If you want one-liners, you can do something like...

char success = 1;

success &= (foo() == desired_result_1);
success &= (bar() == desired_result_2);

etc.

Answer 5

The 2nd is a "better" approach. However, I'd go more without the needless carrying around of an indicator variable:

if( function2() == -1 ){
    return -1;
}

Suggestion: (no magic numbers)

I'd also not use "magic numbers" like you've used it. Instead:

if( check_fail( function2() ) ){
    return FAILED;
}

more clearly illustrated what you're thinking. Intent is easier to maintain. Magic numbers can sometimes wind up hurting you. For instance, I've known financial guys who couldn't understand why a transaction costing "$-1.00" caused their application to behave abnormally.

Answer 6

You use bitwise operators to make a 'neat' variant that doesn't need temp variables and has other fancyness too(with the more advanced operators): return func1()|func2();(this is the same as using logical or, ||). However, if you require checking a specific function in the callee, you can create a bitset: return func1() << 1 | func2(); (this assumes that they return 1 or zero)

Answer 7

I'd vote for the second one as well.

This question reminded me of something similar I do in one of my projects for form validation.

I pass in a reference to an empty string. With each condition I want to check, I either add a line of text to the string, or I don't. If after every test the string is still empty, then there were no errors, and I continue processing the form. Otherwise, I print the string as a message box (which describes the problems), and ask the user to fix the errors.

In this case I don't really care what the errors are, just that there are errors. Oh, and as a bonus, my validation code documents itself a bit because the errors that the user sees are right there.

Answer 8

Use local variable if you need to reuse the result somewhere. Else, call and compare.

Answer 9

I've seen code like this before which might be a little cleaner:

bool result = true;
result = function1() == -1 && result;
result = function2() == -1 && result;
result = function3() == -1 && result;
return result?-1:0;

Edit: forgot about short circuiting.

Answer 10

int foo() {
    return function1() | function2() | function3();
}

Answer 11

Yet another option: pass a pointer to the status variable to each function and have the function set it only if there is an error.

void function1(int *res)
{
    bool error_flag = false;
    // do work
    if (error_flag && (res != NULL)
    {
       *res = ERROR;
    }
}

// similar for function2, function3, ...

int foo()
{
    int res = OK;
    function1(&res);
    function2(&res);
    function3(&res);
    return res;
}

Answer 12

Since all 3 functions always have to get called first and only then you care about the result, I would go for the first solution, because the order of the statements reflects this. Seems more clear to me. Also, I generally don't like functions that do more than just return a value (i.e. that have side effects) in if-clauses, but that's a personal preference.

Answer 13

This sounds like a job for the abundant Perl idiom "<try something> || die()".
int foo() {
     int retVal = 0;
          function1() != -1 || retval = -1;
          function2() != -1 || retval = -1;
          function3() != -1 || retval = -1;
          // ...
     return retVal;
}

Answer 14

First priority, make the code correct. That's more important than readability and optimization.

That means you need to consider what the function should return in the case where the functions it calls all succeed.

Many of the answers given to this question change the result returned or might return a failure indication if the 'sub-functions' all succeed. you need to take care not to do this.

Personally, I think the overall form of your first option is pretty good - it makes clear that the 3 sub-functions are called regardless of whether one or more of them fail. The one problem is that it returns an indeterminate result if all those functions succeed.

Be wary of answers that use bitwise-or to combine results - there are at least 2 potential problems:

1. as John Marshall pointed out in several comments, the order of evaluation is indeterminate. This means that if you simply string the function calls with bitwise-or the functions may be called in any order. This might not be a problem if there are no ordering dependencies between the functions, but usually there are - especially if you don't care about the returned value except as a s success/fail indicator (if you aren't using the return value, then the only reason to call the function is for its side effects)

2. If the functions can return positive, non-zero values when they succeed, then testing for failure becomes a bit trickier than just checking if the results or'ed together are non-zero.

Given these two potential problems, I think there's little reason to try to do anything much fancier than option 1 (or your second option) - just make sure you set res to a success value (0?) for the situation where none of the sub-functions fail.

Answer 15

I write it this way:

int foo()
{
    int iReturn = 0;

    int res1 = function1();
    if (res1 == -1)
    {
        return  iReturn; 
    }

    int res2 = function2();
    if (res2 == -1)
    {
        return  iReturn; 
    }

    int res3 = function3();
    if (res3 == -1)
    {
        return  iReturn; 
    }

    return res;
}

As a coding rule, you should declare your variables as close to the place where it is used.

It is good to use intermediate variable like your res1, res2, res3. But choose a good name so as you intent is clear when you get the value from the function.

And be careful, in the example you've given us, you never assigned the int res; that may be returned when success. The coding rule is to initialize your variable as soon as you can. So you should also initialize your res1 res2 res3 immidiatbly.

Returning an uninitialized value leads to undefined behaviour.

Answer 16

In the first form you're not checking the status until all 3 calls are completed. I think this signals your intent the clearest. The second form more closely resembles the more usual case, where you return early if an error is detected.

It's a subtle thing either way. You shouldn't be asking us strangers on the internet, you should be asking the rest of your team, because they're the ones who will have to live with it.