Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. Special Interest Groups
  3. C++ Gurus
  4. Which comparison loop is easier to reason about and read?
Forum Updated to NodeBB v4.3 + New Features

Which comparison loop is easier to reason about and read?

Scheduled Pinned Locked Moved Solved C++ Gurus
7 Posts 4 Posters 1.3k Views 3 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • fcarneyF Offline
    fcarneyF Offline
    fcarney
    wrote on last edited by fcarney
    #1

    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?

    C++ is a perfectly valid school of magic.

    fcarneyF 1 Reply Last reply
    0
    • fcarneyF 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?

      fcarneyF Offline
      fcarneyF Offline
      fcarney
      wrote on last edited by
      #2

      @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. 😀

      C++ is a perfectly valid school of magic.

      kshegunovK 1 Reply Last reply
      0
      • Chris KawaC Offline
        Chris KawaC Offline
        Chris Kawa
        Lifetime Qt Champion
        wrote on last edited by Chris Kawa
        #3

        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.

        1 Reply Last reply
        2
        • fcarneyF 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. 😀

          kshegunovK Offline
          kshegunovK Offline
          kshegunov
          Moderators
          wrote on last edited by
          #4

          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;
          }

          Read and abide by the Qt Code of Conduct

          Chris KawaC 1 Reply Last reply
          2
          • kshegunovK 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 KawaC Offline
            Chris KawaC Offline
            Chris Kawa
            Lifetime Qt Champion
            wrote on last edited by Chris Kawa
            #5

            @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.

            kshegunovK 1 Reply Last reply
            0
            • Chris KawaC 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.

              kshegunovK Offline
              kshegunovK Offline
              kshegunov
              Moderators
              wrote on last edited by
              #6

              @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 ^_^

              Read and abide by the Qt Code of Conduct

              JonBJ 1 Reply Last reply
              0
              • kshegunovK 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 ^_^

                JonBJ Offline
                JonBJ Offline
                JonB
                wrote on last edited by
                #7

                @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;
                
                1 Reply Last reply
                1

                • Login

                • Login or register to search.
                • First post
                  Last post
                0
                • Categories
                • Recent
                • Tags
                • Popular
                • Users
                • Groups
                • Search
                • Get Qt Extensions
                • Unsolved