6
\$\begingroup\$

I'm learning C++ and wrote a function to remove all the spaces and tabs at the beginning of the input string. It removes them until it find a character different of space and tabs.

In the first version I have mixed C and C++ code and someone has told me that this is a bad coding style, but in the second version I have tried to use only C++ code.

Did I do well?

#include <fstream>
#include <ios>
#include <iostream>
#include <string>

std::string trimLeft(const std::string& input) {

    if ((input.empty()) || 
        ((input.at(0) != ' ') && (input.at(0) != '\t')))
        return input;
    else {
        char * tab2 = new char[input.length() + 1];
        char *trimmed = new char[input.length() + 1];

        strcpy(tab2, input.c_str());

        bool skip = true;
        size_t pos = 0;

        for (size_t i = 0; i < (input.length() + 1); i++) {
            if (skip) {
                if ((tab2[i] == ' ') || (tab2[i] == '\t'))
                    continue;
                else {
                    skip = false;

                    trimmed[pos] = tab2[i];
                    pos++;
                }
            }
            else {
                trimmed[pos] = tab2[i];

                if (tab2[i] == '\0')
                    break;
                else
                    pos++;
            }
        }

        std::string stringTrimmed(trimmed);

        return stringTrimmed;
}

So, I have tried to implement it using only C++ strings:

#include <fstream>
#include <ios>
#include <iostream>
#include <string>

std::string trimLeft(const std::string& input) {

    if ((input.empty()) ||
        ((input.at(0) != ' ') && (input.at(0) != '\t'))) {
        return input;
    }
    else {
        size_t pos = 0;

        for (size_t i = 0; i < input.length(); i++) {
            if ((input.at(i) == ' ') || (input.at(i) == '\t'))
                continue;
            else {
                pos = i;
                break;
            }
        }

        return input.substr(pos, (input.length() - pos));
    }
}
\$\endgroup\$
  • \$\begingroup\$ It would be nice to add a description of what the code is supposed to perform. \$\endgroup\$ – Mathias Ettinger Jun 24 at 8:19
  • \$\begingroup\$ @MathiasEttinger I'd go as far as to say that the method name is sufficient in this case. \$\endgroup\$ – BrainStone Jun 24 at 10:46
  • \$\begingroup\$ @BrainStone Except that without a description we can't know whether OP wants to remove whitespace and checking for tabs and spaces is just an underthought or if it is the whole spec. It's clearer now. \$\endgroup\$ – Mathias Ettinger Jun 24 at 12:14
13
\$\begingroup\$

Suggestions on your code:

  1. Your code currently returns the input string unmodified if it contains nothing but spaces and tabs. I'd say it should return an empty string.

  2. You always use input.at(x) instead of input[x]. The former checks at runtime for out of range errors and in which case throws an exception, while the latter does not. Your code ensures that you are calling them with valid indexes and no additional check is needed.

  3. Your code begins with an if statement.

    if ((input.empty()) ||
        ((input.at(0) != ' ') && (input.at(0) != '\t'))) {
        return input;
    }
    

    This special case looks redundant because your else branch already handles input that is empty or does not start with ' ' or '\t'.

  4. Parentheses are welcome to clarify operator precedence, but you are using too many parentheses IMHO. I would rewrite it like this:

    if (input.empty() || (input[0] != ' ' && input[0] != '\t'))
    

    I removed the parentheses around input.empty() (which is a postfix-expression), and input[0] != ' ' and input[0] != '\t' (which are equality-expressions). I don't think people will take it wrong.

  5. Your use of size_t instead of int is great. In fact, std::string::size_type is guaranteed to be size_t. However, you should use std::size_t instead of the unqualified size_t. (See When using C headers in C++, should we use functions from std or the global namespace?)

  6. s.substr(i) is equivalent to s.substr(i, s.size() - i), so instead of

    return input.substr(pos, (input.length() - pos));
    

    It suffices to write

    return input.substr(pos);
    
  7. You store the value of i to pos, break the loop, and then construct the return value based on pos. This looks unnecessary to me. Why don't you just directly return inside the loop when the desired i is found?

With these applied, your code is already only three lines:

for (std::size_t i = 0; i < input.length(); i++)
    if (input[i] != ' ' && input[i] != '\t')
        return input.substr(i);

(You still need to fix the trim(" ") == " " bug.)

In fact, you can simply make use of the C++ standard library facilities and simplify your code even further:

std::string trim_left(const std::string& input)
{
    if (auto p = input.find_first_not_of(" \t"); p != std::string::npos)
        return input.substr(p);
    else
        return "";
}

If you cannot use C++17, move the declaration of p out of the if statement.

\$\endgroup\$
  • 2
    \$\begingroup\$ trim_left() should first check the value of input.find_first_not_of(" \t") and ensure it isn't std:string::npos. Otherwise, it'll throw an exception when the input is nothing but spaces and tabs. \$\endgroup\$ – Josh Townzen Jun 24 at 21:08
  • \$\begingroup\$ @JoshTownzen Good point, updated. \$\endgroup\$ – L. F. Jun 25 at 4:12

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.