current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite

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?

share|improve this question
1  
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. – Morwenn Dec 6 '14 at 15:28
    
@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 Dec 6 '14 at 15:45
    
@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. – nhaarman Dec 6 '14 at 15:47
1  
@Morwenn That comment looks like it should be an answer. – 200_success♦ Dec 6 '14 at 15:54
    
@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. – Morwenn Dec 6 '14 at 16:15
up vote 7 down vote accepted

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.

share|improve this answer
up vote 4 down vote

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();
}
share|improve this answer
    
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. – nwp Dec 6 '14 at 23:19
    
@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). – Mehrdad Dec 7 '14 at 0:14
    
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. – Morwenn Dec 7 '14 at 10:32
    
@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. – Mehrdad Dec 7 '14 at 10:50
    
@Mehrdad But will there ever that many places where it will be needed? That's the whole point. – Morwenn Dec 7 '14 at 10:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.