Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Function: Trim

Returns a new string after removing any white-space characters from the beginning and end of the argument.

string trim(string word) {

    if(startsWith(word, " ")) return removeSpaces(word, "Front");
    if(endsWith(word, " ")) return removeSpaces(word , "Back");
    return word;
}

string removeSpaces(string word , string position){

    if(position == "Front"){
        for(int i =0;  i <word.length();i++){
            if(word[i] != ' '){
                return word;
            }else{
                word.erase(i);
            }
        }
        return word;

    }else if(position == "Back"){
        for(int i =word.length() - 1 ;  i >=0 ; i--){
            if(word[i] != ' '){
                return word;
            }else{
                word.erase(i);
            }
        }
        return word;
    }
}
share|improve this question
add comment

migrated from programmers.stackexchange.com Jul 29 '13 at 2:50

This question came from our site for professional programmers interested in conceptual questions about software development.

5 Answers

up vote 2 down vote accepted

The second argument to removeSpaces should by no means be a string. I suggest an enum:

enum StringPostion {
    BEGINNING_OF_STRING,
    END_OF_STRING,
};

You appear to have a using namespace std; statement in your program. You shouldn't do this. This StackOverflow question explains why.


Your implementation of trim removes spaces from the front or back of word, not both.


You've chosen an O(kn) algorithm (which is in any case incorrect), where k is the number of spaces at the beginning of the string and n is the number of characters in the string. Each call to s.erase(i) causes all of the characters after i to be shifted to the left. It also causes the string to be shortened by one character. Your function will only erase half of the leading or trailing spaces since you shorten the string and increment i.

Try this:

std::string trim(std::string word) {
  removeSpaces(word, BEGINNING_OF_STRING);
  removeSpaces(word, END_OF_STRING);
  return word;
}
void removeSpaces(std::string& word, StringPosition position) {
  switch (position) {
    case BEGINNING_OF_STRING: {
      const auto first_nonspace = word.find_first_not_of(' ');
      word.erase(0, first_nonspace);
      return;
    }
    case END_OF_STRING: {
      const auto last_nonspace = word.find_last_not_of(' ');
      if (last_nonspace != std:string::npos) word.erase(last_nonspace + 1);
      return;
    }
  }
}
share|improve this answer
    
Dude!!!! Eye Opening explanation :) still more insights are needed.. when i was writing this code i thought that it will work fine., but how can we get to know about the hidden fail cases of our code.? and i have not seen this "O(kn) algorithm" where can i get to know similar kind of algo .. ? –  AravinthChandran Jul 29 '13 at 5:34
1  
Unit testing is very important. I think that Google's C++ unit testing library is pretty good: code.google.com/p/googletest . A simple unit test where you passed " foo " and didn't get "foo" in return would have pointed out your errors. –  ruds Jul 29 '13 at 5:56
1  
"O(kn) algorithm" is shorthand for something like "As the size of your input increases, the number of instructions executed by your algorithm will increase linearly in k and n." This is big-O notation en.wikipedia.org/wiki/Big_O_notation, which is widely used by computer scientists to describe the performance of an algorithm. The main point here is that your algorithm performs poorly compared with mine because you remove one character, copy all the other characters one slot to the left, and then repeat. Mine copies all the non-space characters once, instead of once for each space. –  ruds Jul 29 '13 at 6:01
1  
@AravinthChandran In your edit comment, you asked what precisely const auto first_nonspace = word.find_first_not_of(' '); means. auto gives first_nonspace the same type as the expression on the right side of the assignment operator (in this case, std::string::size_type, which is the same as std::size_t in most implementations but not required to be). const auto means that you add a const qualifier to the type, making first_nonspace a const std::string::size_type. auto is a new feature in the 2011 C++ standard. –  ruds Jul 29 '13 at 13:40
    
@AravinthChandran: To add onto that: using std::string::size_type, auto, or std::size_t also guarantees that you can store the string size, no matter how large it is. Related discussion here. –  Jamal Jul 29 '13 at 13:55
show 2 more comments

We can simplify this:

std::string trim(std::string const& input)
{
    std::string::size_type start = input.find_first_not_of(" ");
    std::string::size_type end   = input.find_last_not_of(" ");

    start = (start == std::string::npos ? 0 : start);
    end   = (end   == std::string::npos ? 0 : end  );

    return input.substr(start, end-start);
}

Notice a couple of things:

We don't copy the string. Pass it in by const reference.

We work out where in the input string the start and end are. Then we use the string substr() method to just hack out the part we need once.

There are a whole bunch of string manipulation functions in string that make this easy.

OK. So I cheated and it does not do white space just space. So think of this is draft one. A tiny bit of extra effort and we can do the same thing but check for white space by using std::isspace().

I would also think about a version that modifies the input string:

void trim_in_place(std::string& input)   // Pass by reference.
{
    std::string::size_type start = input.find_first_not_of(" ");
    std::string::size_type end   = input.find_last_not_of(" ");

    start = (start == std::string::npos ? 0 : start);
    end   = (end   == std::string::npos ? 0 : end  );

    // Create the result
    std::string result = input.substr(start, end-start);

    // Swap it into the input variable (once we know everything has worked).
    swap(result, input)
}
share|improve this answer
add comment

Since there is no common code between the two code paths within removeSpaces you should have two separate functions:

string removeSpacesFromFront(string word) 
string removeSpacesFromBack(string word)
share|improve this answer
    
Same idea I had earlier (also the main cause for the confusion in my answer), but I never thought about separate functions. Good find. –  Jamal Jul 29 '13 at 18:00
add comment
  • Your absence of std:: before your strings lead me to believe that you're using using namespace std;. Don't do that.

  • If-statements don't look too clear as just one line. Try this instead:

    if (startsWith(word, " "))
    {
        return removeSpaces(word, "Front");
    }
    
  • With std::string, it's best to use its iterators instead of indices in a for-loop:

    for (auto iter = str.begin(); iter != str.end(); ++iter)
    {
        // do something with string
    }
    

    However, this won't work well in this particular function since you're reducing the size. For that, I would prefer a while-loop.

  • In case you ever want to remove all whitespace from an std::string:

    str.erase(remove_if(str.begin(), str.end(), isspace), str.end());
    

    It's quite clear and safe since it uses the STL. Include <algorithm> to use remove_if().

share|improve this answer
    
Your erase-remove will work to remove all the spaces from a string, but doesn't fulfill the stated contract of the function (remove whitespace from the beginning and end of the string). So for example, it will do the wrong thing for " hi there ". –  ruds Jul 29 '13 at 4:05
    
@ruds: I see. I somehow thought it said starting from (and the function name isn't specific enough). I'll keep it here for generality. –  Jamal Jul 29 '13 at 4:12
    
I disagree with one-line ifs. They are common, concise and often more readable than their multiline counterparts. (Also, Bjarne uses them.) –  Lstor Jul 30 '13 at 9:35
    
@Lstor: He does? That's good enough for me, then. –  Jamal Jul 30 '13 at 11:57
add comment

I think if I were doing this, I'd treat the string like a normal collection, and use standard algorithms to do the searching. Instead of comparing directly with ' ', I'd also rather see std::isspace (or something similar) used to determine whether something is a space or not.

Since the standard algorithms all use iterators, we can pretty easily make one piece of code work for trimming either the beginning or end of a string by passing normal or reverse iterators.

With that idea in place, things fall together pretty simply:

template <class Iter>
Iter ltrim(Iter b, Iter e) {
    return std::find_if_not(b, e, isspace);
}

std::string ltrim(std::string const &in) {
    return std::string(ltrim(in.begin(), in.end()), in.end());
}

std::string rtrim(std::string const &in) {
    return std::string(in.begin(), ltrim(in.rbegin(), in.rend()).base());
}

std::string trim(std::string const &in) {
    return ltrim(rtrim(in));
}

For the full trim, you might prefer to avoid the intermediate result, and instead search for the positions, and create one new string from both positions:

std::string trim(std::string const &in) {
    auto b = std::find_if_not(in.begin(), in.end(), isspace);
    auto e = std::find_if_not(in.rbegin(), in.rend(), isspace).base();
    return std::string(b, e);
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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