Making the code cleaner

Question

Sorry if this question is not suited for SO.

I have a C++ function that approximately looks like MyFun() given below.

From this function I am calling some(say around 30) other functions that returns a boolean variable (true means success and false means failure). If any of these functions returns false, I have to return false from MyFun() too. Also, I am not supposed to exit immediately (without calling the remaining functions) if an intermediate function call fails.

Currently I am doing this as given below, but feel like there could be a more neat/concise way to handle this. Any suggestion is appreciated.

Many Thanks.

bool MyFun() // fn that returns false on failure
{
    bool Result = true;

    if (false == AnotherFn1()) // Another fn that returns false on failure
    {
        Result = false;
    }

    if (false == AnotherFn2()) // Another fn that returns false on failure
    {
        Result = false;
    }

     // Repeat this a number of times.
    .
    .
    .


    if (false == Result)
    {
         cout << "Some function call failed";
    }

    return Result;
}

`Sorry if this question is suited for SO` God I know I hate it when that happens — Lightness Races in Orbit, Dec 10 '13 at 13:06
This question appears to be off-topic because it belongs on http://codereview.stackexchange.com/ — stefan, Dec 10 '13 at 13:22
@stefan : Did you actually suggest this? Sorry, I am not much familiar with SO. — NeonGlow, Dec 10 '13 at 13:24
@NeonGlow Yes, I'm serious. I'd suggest that you wait for moderators to move your questions. Stackoverflow is for coding problems (compile errors, runtime errors, language questions), but not for "what are better ways to do this?". — stefan, Dec 10 '13 at 13:26
Ok. I hope moderators will do this. Thanks stefan and lightness for suggesting this — NeonGlow, Dec 10 '13 at 13:27

HAL9000 · Answer 1 · 2013-12-10T13:20:31.793

14

I would replace each if statement with a more coincise bitwise AND assignment:

bool MyFun() // fn that returns false on failure
{
    bool Result = true;

    Result &= AnotherFn1(); // Another fn that returns false on failure

    Result &= AnotherFn2(); // Another fn that returns false on failure

     // Repeat this a number of times.
    .
    .
    .
    if (false == Result)
    {
       cout << "Some function call failed";
    }

    return Result;
}

edited Dec 10 '13 at 13:20

answered Dec 10 '13 at 12:11

HAL9000

3,562
3
25
47

I was looking for some thing like this. This will help to reduce the lines of code. Thanks. – NeonGlow Dec 10 '13 at 12:14
@HAL9000 - Better use &&= not &=. Please update the answer – egur Dec 10 '13 at 12:19
@egur: I totally agree with you! I will edit my answer, thanks. – HAL9000 Dec 10 '13 at 12:21
2

@egur: By looking here http://stackoverflow.com/questions/2488406/why-doesnt-c-have-or-for-booleans I change my mind and I leave my original answer. – HAL9000 Dec 10 '13 at 12:22
@phresnel In this case the only difference between bitwise and logical `and` is that the latter does short-circuit evaluation while the former does not. In this respect, the intent is expressed quite clearly. Could you elaborate your concerns regarding maintainability? – ComicSansMS Dec 10 '13 at 12:43
@HAL9000 - what you did is perfectly legal and not buggy. Just using bitwise ops instead of logical. – egur Dec 10 '13 at 12:47
@egur: I tried compiling a&&=b; with g++ and I get this compilation error: prova.cpp:8:5: error: expected primary-expression before ‘=’ token Then, are you sure that &&= actually exists? – HAL9000 Dec 10 '13 at 12:53
Would be nice if you could point out the bits you've changed and explain why you changed them. – Lightness Races in Orbit Dec 10 '13 at 13:07
@LightnessRacesinOrbit I changed nothing. egur suggested to use &&= instead of &=, and initially I was agreeing with him. But after some research and after getting an error when trying to compiling his suggestion, I decided to leave my answer unchanged. – HAL9000 Dec 10 '13 at 13:10
@HAL9000: No, I mean, what you changed from the code in the question. To save me having to scroll up and down to try to spot the differences. – Lightness Races in Orbit Dec 10 '13 at 13:11
@LightnessRacesinOrbit I replaced each if statement with a simpler bitwise assignment. – HAL9000 Dec 10 '13 at 13:14
@HAL9000: Yes, I figured that out, but I'm asking you to include that detail in your answer so that the OP and other readers don't have to perform line-by-line diffing in their head. – Lightness Races in Orbit Dec 10 '13 at 13:18
2

I think this answer is perfect and using bitwise & makes more sense, especially if the OP prefers to consolidate all the &= statements into something like `Result &= AnotherFn1() & AnotherFn2() & AnotherFn3();` – galdin Dec 10 '13 at 13:26
`*=` would also work. ;) – Yakk - Adam Nevraumont Dec 10 '13 at 13:58

Johan · Accepted Answer · 2013-12-10T12:34:07.687

11

Use something like a std::vector of std::function. It is a lot more maintenable.

Example: http://ideone.com/0voxRl

// List all the function you want to evaluate
std::vector<std::function<bool()>> functions = {
    my_func1,
    my_func2,
    my_func3,
    my_func4
  };

// Evaluate all the function returning the number of function that did fail.
unsigned long failure =
    std::count_if(functions.begin(), functions.end(),
        [](const std::function<bool()>& function) { return !function(); });

If you want to stop when a function fail, you just have to use std::all_of instead of std::count_if. You dissociate the control flow from the function list and that is, in my opinion, a good thing.

You can improve this by using a map of function with name as key that will allows you to output which function failed:

std::map<std::string, std::function<bool()>> function_map;

edited Dec 10 '13 at 12:34

answered Dec 10 '13 at 12:19

Johan

3,728
16
25

Very nice way. I didn't know that this was possible. So I can abandon my ugly func. pointers. – magu_ Dec 10 '13 at 13:02
This is what I would have recommended. I hope this gets picked as the correct answer. It would be interesting if a solution with foldLeft exists for C++. I don't know if accumulate can be used. – North Dec 10 '13 at 13:57
@RobertBiter Something like `std::accumulate(functions.begin(), functions.end(), [](const std::function& function, bool value) { return function() && value; }, true);` can be used but it does not make the code clearer. – Johan Dec 10 '13 at 14:03

score 5 · Answer 3 · answered Dec 10 '13 at 12:17

bool MyFun() // fn that returns false on failure
{
    bool Result = true;

    // if need to call every function, despite of the Result of the previous
    Result = AnotherFn1() && Result;
    Result = AnotherFn2() && Result;

    // if need to avoid calling any other function after some failure
    Result = Result && AnotherFn1();
    Result = Result && AnotherFn2();

    return Result;
}

Sebastian Mach · Answer 4 · 2013-12-10T13:27:10.863

Instead of

if (false == AnotherFn1()) // Another fn that returns false on failure
{
    Result = false;
}

if (false == AnotherFn2()) // Another fn that returns false on failure
{
    Result = false;
}

if (false == AnotherFn3()) // Another fn that returns false on failure
{
    Result = false;
}

begin to use booleans as what they are, truth values:

if (!AnotherFn1()) // Another fn that returns false on failure
{
    Result = false;
}

if (!AnotherFn2()) // Another fn that returns false on failure
{
    Result = false;
}

if (!AnotherFn3()) // Another fn that returns false on failure
{
    Result = false;
}

Then, all those conditions have the same code; they are basically part of one big condition:

if ( !AnotherFn1()
   | !AnotherFn2()
   | !AnotherFn3())
{
    Result = false;
}

For your specific problem, where you want all functions be called, even if you know early you'll return false, it is important to not use the short circuiting operators && and ||. Using the eager bitwise operators | and & is really a hack, because they are bitwise and not boolean (and thus hide intent), but work in your situation iff AnotherFn? return strict bools.

You can negate what you do inside; less negations yield better code:

Result = false;


if ( AnotherFn1()
   & AnotherFn2()
   & AnotherFn3())
{
    Result = true;
}

and then you can rid these assignments and instead return straightly:

if ( AnotherFn1()
   & AnotherFn2()
   & AnotherFn3())
{
    return true;
}

cout << "something bad happened";
return false;

Summary

Old:

bool MyFun() // fn that returns false on failure
{
    bool Result = true;

    if (false == AnotherFn1()) // Another fn that returns false on failure
    {
        Result = false;
    }

    if (false == AnotherFn2()) // Another fn that returns false on failure
    {
        Result = false;
    }

     // Repeat this a number of times.
    .
    .
    .


    if (false == Result)
    {
         cout << "Some function call failed";
    }

    return Result;
}

New:

bool MyFun() // fn that returns false on failure
{
    if (AnotherFn1() &
        AnotherFn2() &
        AnotherFn3())
    {
        return true;
    }
    cout << "Some function call failed";
    return false;
}

There are more possible improvements, e.g. using exceptions instead of error codes, but don't be tempted to handle "expections" instead.

This is not the same behavior. In the old version all the functions are always evaluated, not in your version. — Johan, Dec 10 '13 at 12:29
@Johan: I missed that this is a requirement. I changed my answer. — Sebastian Mach, Dec 10 '13 at 13:32

score 0 · Answer 5 · answered Dec 10 '13 at 12:11

! can be used as a cleaner alternative to false

Like this:

bool MyFun() // fn that returns false on failure
{
bool Result = true;

if (!AnotherFn1()) // Another fn that returns false on failure
{
    Result = false;
}

if (!AnotherFn2()) // Another fn that returns false on failure
{
    Result = false;
}

 // Repeat this a number of times.
.
.
.


if (!Result)
{
     cout << "Some function call failed";
}

return Result;

}

score 0 · Answer 6 · answered Dec 10 '13 at 12:12

0

how about using exceptions to handle failure:a neat exemple

the main question is, are the function call interdependant or not? can some be skipped if a previous one failed? ...

answered Dec 10 '13 at 12:12

Julien Lopez

1,021
6
14

Making the code cleaner

6 Answers6

Summary