HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Checking if a string is a number with STL

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberstlwithcheckingstring

Problem

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

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#12959, answer score: 4

Revisions (0)

No revisions yet.