HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Longest string in a vector — two implementations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationstwolongeststringvector

Problem

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 &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 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?

Solution

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

Context

StackExchange Code Review Q#71877, answer score: 9

Revisions (0)

No revisions yet.