# Checking if a string is a number with STL

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)));
}

• It's not elegant if it has code for special cases (that's part of my definition of elegant). Commented Jun 22, 2012 at 20:30
• For somebody unfamiliar with stl, this is unreadable. Simple for loop is easy, and understood by anybody
– elmes
Commented Jun 22, 2012 at 20:31
• It seems pretty darn hard to read to me... I would never do so many things in one statement.
– jahroy
Commented Jun 22, 2012 at 20:31
• @elmes: Even for somebody extremely familiar with STL, this is damned hard to read. Commented Jun 22, 2012 at 20:35
• I have nothing against the algorithms. But would be better if you did not try and do it in one line. One list does not equal elegant. Easy to read (at a glance) equal elegant. Commented Jun 23, 2012 at 0:41

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.

• if i compiled and ran in my head correctly second transformation returns true for "42-1729" Commented Jun 22, 2012 at 22:04
• @NoSenseEtAl: Edited, please take a look. Commented Jun 22, 2012 at 22:45
• i guess you also need to remove minus from find_first_not_of, but it is just a minor thing Commented Jun 22, 2012 at 22:58
• if (…) return true; else return false; is a huge antipattern in my book. Commented Jun 23, 2012 at 10:17
• return ( m.find_first_not_of("0123456789", pos) == m.end() ); would be preferable, imho. Commented Jun 23, 2012 at 14:24

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';
}

• Corner case is that std::string allows '\0' within the string. Unlikely to fail but not foolproof. Commented Jun 24, 2012 at 8:32

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();
}

• @3. what variable, string or it returned by m.begin()? Afaik it modifies iterator, so it is legit. std::string s="C++11"; ++s.begin(); ++s.begin(); ++s.begin(); char c=*s.begin(); assert (c='S'); Commented Jun 23, 2012 at 13:58
• @NoSenseEtAl I take that objection back partially. However, the operation is still not really meaningful, since ++ modifies its argument. But we discard that modified value, since it’s a temporary. This still works since prefix ++ also returns the modified value but it’s still not an entirely meaningful operation – the side-effect was just ignored. Commented Jun 23, 2012 at 14:08
• still I dont get your objection, that temporary lives long enough to be used as input param (begin iterator for find_if). So to make it clear is it a stye or correctness objection? Commented Jun 23, 2012 at 14:10
• @NoSenseEtAl It’s an objection of style, not of correctness (it was initially, but that was my mistake, because the style confused me). Commented Jun 23, 2012 at 14:15
• Ah, just noticed you posted an answer, going for the sane return nonetheless. if (…) return true; else return false; made me chuckle. Commented Jun 23, 2012 at 14:29