Which comparison loop is easier to reason about and read?

fcarney

I am learning to use a more functional approach for certain operations. I am also learning about what is in the <algorithm> library. I wrote 2 ways to solve a problem of comparing values in a vector that uses precomputed indexes from another vector. I did the first loop as a traditional for loop and the second using the ~~std::any_of~~ std::all_of algorithm. The for loop is slightly less efficient in that it compared the first board location to itself, but it makes for much cleaner code with few variables (in my program version it returns true or false from a function call). The second uses a lambda.

    // which one is easier to read?
    std::vector<int> board(9,0); // gets filled with data elsewhere
    std::vector<int> sequence = {2,4,6}; // precomputed indexes into board
    // this one?
    bool match=false;
    for(auto item: sequence){
        if(board[sequence[0]] == board[item])
            match = true;
    }
    qInfo() << match;
    // or this one?
    match = std::all_of(++sequence.begin(), sequence.end(), [&](int item){
        return board[sequence[0]] == board[item];
    });
    qInfo() << match;

I like the idea of ~~std::any_of~~ std::all_of which theoretically should reduce complexity. However, I find using a lambda syntax to be a bit messy. Maybe using a predefined function for the function would be more explicit as to intent.

I like them both, but am not completely satisfied with either. So, which one is easier to read and why?

fcarney

@fcarney Interesting. I reasoned incorrectly about how the first loop operates:

    // this one?
    for(auto item: sequence){
        if(board[sequence[0]] == board[item])
            return false;
    }
    return true;
    qInfo() << match;
    // or this one?
    return std::all_of(++sequence.begin(), sequence.end(), [&](int item){
        return board[sequence[0]] == board[item];
    });
    qInfo() << match;

So maybe that answers my question. It was easier to reason about the second version when trying to describe the problem. 😀

Chris Kawa

Discussing which is nicer is kinda pointless considering what the functions do.

The first one is always gonna match on the first item as it's basically doing board[sequence[0]] == board[sequence[0]], yet it keeps going on all the other items. This function is useless.

The second one for some reason skips the first item, so the behavior is not the same as the first one. It then does the same pointless iteration and the only difference is that the first one will always succeed and the other will fail if sequence[0] == 0.

Other than that both of these will crash if sequence contains number larger than 8.

All in all I think you're getting blinded by the forced functional style, which not only makes both of these hard to read but also easy to sneak subtle errors like the ones I pointed out.

If you look closely what you're doing (at least in the first version) is essentially:

bool match = std::find(board.begin(), board.end(), board[sequence[0]]) != board.end();

Which of course is gonna return true (unless it crashes on out of bound read), and IMO this is the code that's easier to read.

EDIT: You changed what the first function does so my comment is not valid anymore. That being said I'd go with the std algorithm as the name suggests what it does.

kshegunov

I fail to grasp the idea. If I were to code something like this, I'd use the old and tested way, which is rather simple and explicit:

for (size_t i = 1, size = sequence.size(); i < size; i++)  {
    if (board[sequence[0]] == board[sequence[i]])
        return true;
}

Chris Kawa

@kshegunov said in Which comparison loop is easier to reason about and read?:

I fail to grasp the idea

The idea is (if I got it correctly) to check if all elements satisfy a predicate. Yours (and mine above) does a find of the first one that does, which is not the same.

kshegunov

@Chris-Kawa said in Which comparison loop is easier to reason about and read?:

The idea is (if I got it correctly) to check if all elements satisfy a predicate. Yours (and mine above) does a find of the first one that does, which is not the same.

Ah, okay. I see how the above wouldn't work as expected ^_^

JonB

@kshegunov
It doesn't help that the OP's code uses std::all_of while his comment has "I like the idea of std::any_of"....

Which is just as easy using your "old and tested way, which is rather simple and explicit":

for (size_t i = 1, size = sequence.size(); i < size; i++)  {
    if (board[sequence[0]] != board[sequence[i]])
        return false;
}
return true;