8
\$\begingroup\$

We have to print the last word of the given string. How to improve and optimise this code?

#include <iostream>

std::string get_last_word(const std::string& str)
{
    if (str.length() == 0)
    {
        std::cerr << "No string\n";
        return 0;
    }

    int len = str.length();
    int i = len - 1;
    while (i >= 0 && str[i] != ' ')
    {
        i--;
    }
    std::string last_word;
    for (int j = i + 1; j < len; j++)
    {
        last_word += str[j];
    }
    return last_word;
}

int main()
{
    std::string str;
    std::cout << "Enter string :\n";
    std::getline(std::cin, str);
    std::cout << "Last word :\n";
    std::cout << get_last_word(str);
    std::cout << std::endl;
}
\$\endgroup\$
13
\$\begingroup\$

Be careful with types. For example, your get_last_word function is meant to return a string but in your error case you return 0. If you want to return an empty string, use return "";

I'm guessing that you might be working under restrictions about what library functions you're allowed to use here, but you don't say so.

If you're allowed to use anything in the standard library, have a look at std::string::find_last_of as a replacement for your first loop.

Similarly, look at std::string::substr as a replacement for your second loop. Building a string one character at a time is generally discouraged because it's quite inefficient. Something like substr will be more efficient because it can, for example, allocate all the memory needed by your string in one go.

\$\endgroup\$
  • \$\begingroup\$ Is there a notable difference between find_last_of and rfind? I'm fairly certain the entire task can be done with something like line.substr(line.rfind(" ") + 1) \$\endgroup\$ – yuri Apr 15 '18 at 12:14
  • \$\begingroup\$ rfind searches for a character sequence (which can be of length one if you just want one character) find_last_of does not do sequences, it searches for a character matching a pattern. If you want to find the last time the word 'apple' appears in a text, you want rfind. If you want the last time a full stop, exclamation mark, or question mark appears in a text, you want find_last_of. Either should work if you want a single option for a single character. \$\endgroup\$ – Josiah Apr 15 '18 at 20:38
  • \$\begingroup\$ As to the single line solution, that might suffice for many cases but I suspect needs special handling for the case where there are no spaces in the string. \$\endgroup\$ – Josiah Apr 15 '18 at 20:41
  • \$\begingroup\$ I was more interested in performance differences; The code in the OP uses space as delimiter so for that case it'd be fine. \$\endgroup\$ – yuri Apr 15 '18 at 21:05
  • \$\begingroup\$ It looks to me that at least in libcxx, calling find_last_of with a single char falls back to rfind. On that basis, I feel comfortable saying the performance should be identical. Obviously different libraries could differ. \$\endgroup\$ – Josiah Apr 15 '18 at 22:17
10
\$\begingroup\$

Looking at the code, it seems like you are reinventing the wheel. See the answer of @Josiah for some alternatives to use the standard library. Assuming this ain't possible for some reason, lets say its implemented badly, I would improve some smaller things.

Before giving some improvements, make sure to measure! Is this code only used once? Does it only take 1% of the time? Is the example code the only use? Than don't spend time on this!

Add preconditions

Currently your function accepts an empty string. When this is encountered, you always get an error message. If you somewhere want to handle this differently, it should be checked twice. Or in case you know it can't be empty, it still is checked when not needed.

Instead, you could write:

assert(!str.empty() && "An empty string should not be passed");

If your specific example, you'll have to write the check in the main function. However, in that case, you could use an alternative way of handling it. For example:

std::string str;
do
{
    std::cout << "Enter string :\n";
    std::getline(std::cin, str);
} while (str.empty());

Memory allocations

std::string can have short string optimization. Although, for large strings, this might allocate memory. If you append character by character, this might reallocate.

To prevent this reallocation, you can reserve the string:

last_word.reserve(str.size() + 1 /*Null terminator*/);

The code above would be wonderful for a vector, when you have at least 1 element. However, as we have short string optimization, this might allocate, while the result won't require it.

last_word.reserve(len - j); // To be verified with null terminator.

If really performance critical, you might want to check the implementation of the standard library as they are allowed to reserve more characters than you pass. So, they can add 1 for the null terminator. (libstdc++ and MSVC do so, I read)

Output argument

In order to not recreate a string, you can manipulate the original string. With the erase method, you can (in bulk) remove all previous characters at once.

This will work when you don't need that argument any more, however, this might add unwanted overhead if you do or don't have a std::string instance.

string_view

std::string_view was added in a recent c++ version, this will behave as a string, although, doesn't store the content in it. Returning a string_view might prevent copying over the characters into the std::string. Same can be said for the input argument.

Warning: This is error prone in case you work with temporaries.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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