3
\$\begingroup\$

As I thought about this question on SO, I realized that I wanted to implement either a string trimmer or a trimmed string inputter.

This is what I came up with, but I have a nagging feeling I could have done better.

Would you critique this, please?

#include <string>
#include <iostream>
#include <sstream>
#include <cassert>

template <typename Ch, typename Tr, typename Alloc>
void
trim(std::basic_string<Ch, Tr, Alloc>& str, const std::locale& loc)
{
  // (b,e)=ltrim(str)
  typename std::basic_string<Ch, Tr, Alloc>::const_iterator b = str.begin();
  typename std::basic_string<Ch, Tr, Alloc>::const_iterator e = str.end();
  while( b != e && std::isspace(*b, loc) )
    b++;

  // (b,e)=rtrim(b,e)
  // Query: this loop looks u-g-l-y. Can it be less ugly?
  while( b != e ) {
    e--;
    if(!std::isspace(*e, loc)) {
      e++;
      break;
    }
  }

  // str=(b,e)
  // Query: this string self-assigns (sort of). Is this UB?
  str.assign(b, e);
}

template <typename Ch, typename Tr, typename Alloc>
std::basic_istream<Ch,Tr>&
getTrimmedLine(std::basic_istream<Ch,Tr>& is,
  std::basic_string<Ch, Tr, Alloc>& str)
{

  // Get raw data from input
  std::getline(is, str);

  // And trim it
  trim(str, is.getloc());

  return is;
}

#define TEST
#ifdef TEST
template<typename T, std::size_t N>
std::size_t countof( T (& a)[N] ) { return N; }

int main () {
  const char * const inputs[] = {
    "simple", " front", "back ", " both ",
    "  front2", "back2  ", "  both2  ",
    "\n", "", "\n\n\n",
    "  \n", "  ", "  \n\n\n"
  };
  const char * const outputs[] = {
    "simple", "front", "back", "both",
    "front2", "back2", "both2",
    "", "", "",
    "", "", ""
  };

  assert(countof(inputs) == countof(outputs));

  for(std::size_t i = 0; i < countof(inputs); i++) {
    std::istringstream is(inputs[i]);
    std::string result;
    getTrimmedLine(is, result);
    assert(result == outputs[i]);
  }
}
#endif
\$\endgroup\$
1
\$\begingroup\$

Some thoughts:

  • Would you lose anything if trim took a single template parameter that would accept the type of the string? You're not doing anything with the individual parameters, after all.
  • I don't think the second loop is particularly ugly. You could use operator- first and then only decrement if it's not a space, but I don't think that's particularly useful.
  • Honestly don't know, about the undefined behaviour, you should ask on StackOverflow (I'm fairly certain that's on-topic there).
  • I think you should check that std::getline left the stream in a good state before applying trim. You may also want to provide the possibility to pass extra parameters to std::getline.
  • countof should have the signature std::size_t countof( T const (& a)[N] ).
\$\endgroup\$
2
\$\begingroup\$
typename std::basic_string<Ch, Tr, Alloc>::const_iterator e = str.end();
...

// Query: this loop looks u-g-l-y. Can it be less ugly?
  while( b != e ) {
    e--;
    if(!std::isspace(*e, loc)) {
      e++;
      break;
    }
  }

This looks weird and tends to go UB.

1) e = str.end();

2) e--; /* it's better to use --e in this case */ you move one more far position after end()!!!

3) std::isspace(*e, loc) <--- UB, accessing the memory that haven't been allocated

If you need to find the first and the last spaces in the string, use this:

b = find_if(str.begin(),  str.end(),  isspace_predicate);
e = find_if(str.rbegin(), str.rend(), isspace_predicate);
\$\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.