Checking if a string is a number with STL

Question

Recently me and colleague had a discussion about the following piece of code (simple bool function that checks if string is a number, +1000 not allowed, -1000, 1234 ... allowed).

He felt that it was hackish, while I thought it was nice, clean and elegant (since it uses STL, not hand made loops).
So is this a judgement call or one of us is wrong? Is this code elegant or hack?

 bool validate(const std::string& m)  
 {
   if (m.empty()) 
     return false;
   return m.end() == find_if(*m.begin() == '-'? ++m.begin() : m.begin(),
                                    m.end(), not1(ptr_fun(isdigit)));
 }

Answer 1

I had to really look at your code and think about what it was doing before I figured it out. That tells me that this code is not elegant by my definition of "elegant". My definition of elegant is something like,

Easy to understand, easy to maintain, efficient in execution, robust.

I would suggest something like this as a start:

if( m.empty() )
  return false;

string::iterator start = m.begin();
if( *start == '-' )
  ++start;

if( m.end() == find_if( start, m_end(), not1(ptr_fun(isdigit)) ))
  return true;
else
  return false;

Not much caring for the use of find_if, I'd refine it to:

if( m.empty() )
  return false;

string::size_type pos = 0;
if( m[0] == '-' )
  pos = 1;

if( m.find_first_not_of("0123456789", pos) == m.end() )
  return true;
else
  return false;

I find this to be much more elegant than the 1-liner you wrote. It's more code, sure. But it is exceedingly simple to understand and maintain. You found a bug yourself just by brain-compiling it. That's what code should look like.

Answer 2

I think in this case, pretty much pure C does the job as well as anything:

bool check_num(std::string const &in) {
    char *end;

    strtol(in.c_str(), &end, 10);
    return !in.empty() && *end == '\0';
}

Answer 3

If you insist on using find_if (fine by me), and not mutating variables (fine by me as well!), your code still suffers from three deficiencies:

1. Too much happening in a single statement. Separate.
2. Why *m.begin() instead of m.front() or even m[0]?
3. ++ first and foremost mutates a value. ++m.begin(), even though it compiles, smells, since we discard the mutated value and just use the value returned by the expression. This definitely qualifies as a hack.

But since string::iterator is a random-access iterator, you can just write + 1. In cases where that doesn’t work, there’s now std::next which calls std::advance internally and returns the modified result.

bool validate(const std::string& m)  
{
    if (m.empty()) 
        return false;

    auto begin = m[0] == '-' ? std::next(m.begin()) : m.begin();
    return find_if(begin, m.end(), not1(ptr_fun(is_digit))) == m.end();
}