while loop with comma operator verses duplicate code verses “break;”

Question

After reading a great answer about the comma operator in C/C++ (What does the comma operator do - and I use the same example code), I wanted to know which is the most readable, maintainable, preferred way to implement a while loop. Specifically a while loop whose condition depends on an operation or calculation, and the condition might be false the first time (if the loop were to always pass at least once then the do-while would work fine).

Is the comma version the most preferred? (how about an answer for each, and the rest can vote by upvoting accordingly?)

Simple Implementation

This code has duplicate statements, that (most likely) must always be the same.

string s;
read_string(s);     // first call to set up the condition
while(s.len() > 5)  // might be false the first pass
{
   //do something
   read_string(s);  // subsequent identical code to update the condition
}

Implementation using break

string s;
while(1)                  // this looks like trouble
{
   read_string(s);
   if(s.len() > 5) break; // hmmm, where else might this loop exit
   //do something
}

Implementation using comma

string s;
while( read_string(s), s.len() > 5 ) 
{
   //do something
}

Answer 1

I would say none of the above. I see a couple of options. The choice between them depends on your real constraints.

One possibility is that you have a string that should always have some minimum length. If that's the case, you can define a class that embodies that requirement:

template <size_t min>
class MinString{
    std::string data;
public:
    friend std::istream &operator>>(std::istream &is, MinString &m) {
        std::string s;
        read_string(is, s); // rewrite read_string to take an istream & as a parameter
        if (s.length() >= min)
            m.data = s;
        else
            is.setstate(std::ios::failbit);
        return is;
    }

    operator std::string() { return data; }

    // depending on needs, maybe more here such as assignment operator
    // and/or ctor that enforce the same minimum length requirement

};

This leads to code something like this:

Minstring<5> s;
while (infile >> s)
    process(s);

Another possibility is that you have normal strings, but under some circumstances you need to do a read that must be at least 5 characters. In this case the enforcement should be in a function rather than the type.

bool read_string_min(std::string &s, size_t min_len) { 
    read_string(s);
    return s.length() >= min_len;
}

Again, with this the loop can be simple and clean:

while (read_string_min(s, 5))
    process(s);

It's also possible to just write a function that returns the length that was read, and leave enforcement of the minimum length in the while loop:

while (read_string(s) > 5)
    process(s);

Some people like this on the idea that it fits the single responsibilty principle better. IMO, "read a string of at least 5 characters" qualifies perfectly well as a single responsibility, so it strikes me as a weak argument at best though (but even this design still makes it easy to write the code cleanly).

Summary: anything that does input should either implicitly or explicitly provide some way of validating that it read the input correctly. Something that just attempts to read some input but provides no indication of success/failure is simply a poor design (and it's that apparent failure in the design of your read_string that's leading to the problem you've encountered).

Answer 2

There is a fourth option that seems better to me:

string s;
while( read_string(s) && s.len() > 5 ) 
{
   //do something
}