Longest string in a vector — two implementations

Question

I was given this piece of code, which calculates the length of the longest string in a vector of strings:

static size_t max_line_length(std::vector<std::string> &lines) {
 size_t max = 0;
 for (auto it = lines.begin(); it != lines.end(); it++) {
 if (it->length() > max) {
 max = it->length();
 }
 }
 return max;
}

I am new to C++ with a significant background in Java. If I would write such a function myself, I would come up with something like:

static size_t max_line_length(std::vector<std::string> lines) {
 size_t max = 0;
 for (auto line : lines) {
 if (line.length() > max) {
 max = line.length();
 }
 }
 return max;
}

The difference is in the use of pointers, and the way in iterating the strings.

Is there a significant performance difference between these implementations, or are there best practices involved in this?

• Morwenn: The first loop is probably a C++03-era loop. The second one is a C++11 one and is better. However, change auto to const auto& in the loop, otherwise every line will be needlessly copied.
• nhaarman: @Morwenn Thanks! I have read up on the 'pass by value vs const reference' as well. Does this mean that I should use const std::vector<std::string> &lines as well? Also, if you could write your comment in a nice little answer, I can accept it :)
• nhaarman: @Morwenn On second thought, the vector would be a constant, but not the strings themselves, right? So this would allow the function to manipulate the strings, whereas the original method would be pure.
• 200_success: @Morwenn That comment looks like it should be an answer.
• Morwenn: @200_success I wasn't sure whether the question was on-topic or not (the form seemed strange), so I chose to let a comment instead of answering.

Answer 1

The first version of the algorithm uses a traditional C++03-era for loop with iterators. The only modern element is the auto used to deduce an std::vector<std::string>::iterator. The second loop uses a C++11 range-based for loop whose syntax is close to the one used in Java. The range-based for loop is now preferred for several reasons:

• Its terse syntax (when iterators are fully spelled out, a traditional for loop is ugly).

• Its genericity: a range-based for loop also works with C arrays and std::valarray.

• end is automatically cached instead of being evaluated at each iteration.

---

That's it for the for loops: the range-based for loop is preferred. However, there are some things that could be improved in your second version:

• You should pass lines as a const reference. When you don't need to modify an instance, a const reference is good since it avoids a useless copy.

• In your range-based for loop, you should write const auto& since you don't modify line either. It currently performs many useless copies of full std::strings just to get their size. Actually, the best thing to write in range-based for loops is generally auto&&, but the explanation may be a little complicated since it involves lvalues, rvalues, const-correctness and type deduction.

Answer 2

This is more C++-like:

struct size_less
{
 template<class T> bool operator()(T const &a, T const &b) const
 { return a.size() < b.size(); }
};
static size_t max_line_length(std::vector<std::string> const &lines)
{
 return std::max_element(lines.begin(), lines.end(), size_less())->size();
}

• nwp: What is Base? Why use a template? Why not use a lambda? lines should be const. Free functions begin and end should be favored over member functions begin and end. Using the standard library is a very good point though.
• user541686: @nwp: Sorry for the Base, it was left over from another suggestion I was going to give but then decided not to. I improved the function with const, thanks. As for why not lambdas?: because they bloat the generated code for no good reason. Every lambda has a new type, even if it has the same body -- thus every lambda generates new code, which increases compile times and bloats the output file for no good reason. Not to mention that this is C++03-compatible which I prefer (and that's why I didn't use the free versions of begin and end).
• Morwenn: Lambdas bloat the generated code no more than a size_less class created to be used in one specific place and they allow for more expressive local function objects.
• user541686: @Morwenn: No, the entire point of size_less is that it can be used in more than one specific place. Although I'm sure you already knew this.
• Morwenn: @Mehrdad But will there ever that many places where it will be needed? That's the whole point.