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

Repeating a std::string n times

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

Problem

I recently posted an answer to this SO question as I did not think any of the current answers were satisfactory (most don't even answer the question!). I was wondering if anyone could offer any improvements on my solution to this problem? I'm most interested in performance here. Also note this algorithm does not require any additional copies of the input string (if passed an r-value), ideally I'd like to keep this property.

#include 
#include   // std::size_t
#include     // std::floor, std::log2, std::pow
#include  // std::cbegin, std::cend, std::next

std::string repeatstring(std::string str, std::size_t n)
{
    if (n == 0) {
        str.clear();
        str.shrink_to_fit();
        return str;
    }

    if (n == 1 || str.empty()) return str;

    const auto repeat_size = str.size();

    if (repeat_size == 1) {
        str.append(n - 1, str.front());
        return str;
    }

    str.reserve(repeat_size * n);

    const auto m = static_cast(std::floor(std::log2(n)));

    for (auto i = m; i > 0; --i) str += str;

    n -= static_cast(std::pow(2, m));

    str.append(std::cbegin(str), std::next(std::cbegin(str), n * repeat_size));

    return str;
}


Here is an example:

#include 
#include 

int main()
{
    auto start = std::chrono::system_clock::now();

    auto str = repeatstring("helloworld", 190238011);

    auto end = std::chrono::system_clock::now();

    //std::cout (end - start);

    std::cout << "time: " << duration.count() << "ms" << std::endl;

    return 0;
}

Solution

Input as reference-to-const

Change your signature to:

std::string repeat_string(std::string const& str, std::size_t N);


First, this avoids an unnecessary copy when users don't pass their strings in by rvalue (which seems more typical?). Secondly, this allows for copy elision on the returned object, as you can't have copy elision from a function parameter. So if I called repeat_string with an lvalue, I just saved two copies.

Also, note the name change. Either use camelCase or snake_case or function names, don't just jamlotsofwordstogether. It's much harder to read.

The new signature makes some of the other code easier.

if (n == 0) {
    return {};
}

...

if (str.size() == 1) {
    return std::string(n str[0]);
}


repeat_size

This is a weird variable at best. Once you make str a reference to const, you can just access str.size() which makes much more sense. repeat_size isn't really the size of the repeat - that's what n is...

Powers of 2

std::log2 and std::pow aren't cheap, and you need neither. For the former, you can just multiply by two until you're done:

std::string result = str;
result.reserve(str.size() * n);

std::size_t m = 2;
for (; m <= n; m *= 2)
{
    result += result;
}


Now m is the first power of 2 larger than n. So you can replace

n -= static_cast(std::pow(2, m));


with:

n -= m/2;


And:

str.append(std::cbegin(str), std::next(std::cbegin(str), n * repeat_size));


is a really complicated way of writing what now becomes:

result.append(result.c_str(), n * str.size());

Code Snippets

std::string repeat_string(std::string const& str, std::size_t N);
if (n == 0) {
    return {};
}

...

if (str.size() == 1) {
    return std::string(n str[0]);
}
std::string result = str;
result.reserve(str.size() * n);

std::size_t m = 2;
for (; m <= n; m *= 2)
{
    result += result;
}
n -= static_cast<decltype(n)>(std::pow(2, m));
str.append(std::cbegin(str), std::next(std::cbegin(str), n * repeat_size));

Context

StackExchange Code Review Q#114295, answer score: 7

Revisions (0)

No revisions yet.