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));
}
}
-
\$\begingroup\$ It would be nice to add a description of what the code is supposed to perform. \$\endgroup\$301_Moved_Permanently– 301_Moved_Permanently2019年06月24日 08:19:44 +00:00Commented Jun 24, 2019 at 8:19
-
\$\begingroup\$ @MathiasEttinger I'd go as far as to say that the method name is sufficient in this case. \$\endgroup\$BrainStone– BrainStone2019年06月24日 10:46:11 +00:00Commented Jun 24, 2019 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\$301_Moved_Permanently– 301_Moved_Permanently2019年06月24日 12:14:23 +00:00Commented Jun 24, 2019 at 12:14
1 Answer 1
Suggestions on your code:
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.
You always use
input.at(x)
instead ofinput[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.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'
.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), andinput[0] != ' '
andinput[0] != '\t'
(which are equality-expression s). I don't think people will take it wrong.Your use of
size_t
instead ofint
is great. In fact,std::string::size_type
is guaranteed to besize_t
. However, you should usestd::size_t
instead of the unqualifiedsize_t
. (See When using C headers in C++, should we use functions fromstd
or the global namespace?)s.substr(i)
is equivalent tos.substr(i, s.size() - i)
, so instead ofreturn input.substr(pos, (input.length() - pos));
It suffices to write
return input.substr(pos);
You store the value of
i
topos
, break the loop, and then construct the return value based onpos
. This looks unnecessary to me. Why don't you just directly return inside the loop when the desiredi
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.
-
2\$\begingroup\$
trim_left()
should first check the value ofinput.find_first_not_of(" \t")
and ensure it isn'tstd:string::npos
. Otherwise, it'll throw an exception when the input is nothing but spaces and tabs. \$\endgroup\$Josh Townzen– Josh Townzen2019年06月24日 21:08:12 +00:00Commented Jun 24, 2019 at 21:08 -
\$\begingroup\$ @JoshTownzen Good point, updated. \$\endgroup\$L. F.– L. F.2019年06月25日 04:12:17 +00:00Commented Jun 25, 2019 at 4:12
Explore related questions
See similar questions with these tags.