current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 7 down vote favorite

I want to implement the following function:

// Return true if and only if 's' is numeric including
// leading positive/negative sign, decimal point.
bool isnumeric( const char * s );

It is somewhat similar to strtol() but I don't need to return the number.

My approach is to count various things unless I can bail out:

bool isnumeric( char const * str ) {
  if( !str ) { return false; }
  int signs = 0;
  int decimals = 0;
  int digits = 0;
  int digitsAfterDecimal = 0;

  for( char const * p = str; *p; ++p ) {

    if( (*p == '+') || (*p == '-') ) {
      if( (decimals > 0) || (digits > 0) ) { return false; }
      signs += 1;
      if( signs == 2 ) { return false; }
    }
    else if( *p == '.' ) {
      decimals += 1;
      if( decimals == 2 ) { return false; }
    }
    else if( ! isdigit( *p ) ) { 
      return false;
    }
    else {
      digits += 1;
      if( decimals > 0 ) {
        digitsAfterDecimal += 1;
      }
    }
  }
  return (decimals > 0) ? ((digits > 0) && (digitsAfterDecimal > 0))
                        : (digits > 0) ;
}

I also have the following tests:

void test_isnumeric() {
  assert( isnumeric( "42" ) );
  assert( isnumeric( "42.0" ) );
  assert( isnumeric( "42.56" ) );
  assert( isnumeric( "+42" ) );
  assert( isnumeric( ".42" ) );
  assert( isnumeric( "+.42" ) );

  assert( ! isnumeric( "42." ) );
  assert( ! isnumeric( "++42" ) );
  assert( ! isnumeric( "+." ) );
  assert( ! isnumeric( "4+" ) );
}

int main( void ) {
  test_isnumeric();
}

To make it easy to clone and modify, the full code is available here.

Please comment on design, structuring, test coverage etc. Mentioning failing tests are most welcome.

share|improve this question
1  
Looks like you're using your ints mainly as bools, might want to consider to use bools there for that. –  Bobby Apr 28 at 21:18
    
@Bobby: Which int variable you are referring to? The following int variables (signs, decimals, digits, digitsAfterDecimal) are used as counters. –  Arun Apr 28 at 21:50
    
Yes, but you're only testing them for greater than 0, that's basically what a boolean does. F.e. decimals might be bool called hasDecimalPoint, *p == '.' ... if (hasDecimalPoint) return false; hasDecimalPoint = true;. –  Bobby Apr 28 at 22:26
    
@Bobby: Thanks for the followup comment and clarification, I now understand what you are saying. When I applied it, I could simplify the code. [Please feel free to enter your comment as an answer.] With counters, I retained more information as obtained from the scan than it is necessary for this particular problem, which might be helpful if the problem is modified. –  Arun Apr 28 at 22:59
add comment

2 Answers

up vote 4 down vote

State variables are bad. Keep the state explicit, along the lines of:

if (*p == '+' || *p == '-') p++;
if (!isdigit(*p)) return False;
while (isdigit(*p)) p++;
if (*p == 0) return True;
if (*p != '.') return False;
p++;
while (isdigit(*p)) p++;
return *p == 0;

Update: few fixes thanks to Edward

share|improve this answer
    
+1; Thanks for reviewing my code. I would love to write a compact form such as yours. But, in that approach, I had difficulty covering the malformed inputs, for e.g. "42.", "+." etc. Perhaps I need to think harder :-) –  Arun Apr 28 at 22:39
    
@Arun I find it strange that you consider 42. not to be a numeric value. In C, 42. == 42.0. –  Morwenn Apr 29 at 8:15
1  
I didn't mean the snippet to be working. In fact, it has certain problems in correctness (e.g. returning True for a suspicious "+.") and design. It is here only to demonstrate a stateless approach. –  vnp Apr 29 at 18:34
    
On line 4 you mean (*p == 0) rather than (*p == '0') to test for end of string rather than a zero digit and line 2 could be if (!isdigit(*p)) return False; –  Edward May 1 at 3:17
    
@Edward You are absolutely right. Fixing. –  vnp May 1 at 3:49
add comment
up vote 4 down vote

Over-all no major problems - just nits.

  1. Minor inefficiency. Test for digits first.

  2. A simple test for leading sign would do.

  3. Decimal point is locale-sensitive.

    char dp = localeconv()->decimal_point[0].

  4. Pedantic (meaning only the crazy care): When counting elements of an array, recommend type size_t rather than int for digits, digitsAfterDecimal. Either that or fail if digits is to exceed INT_MAX. The main issue here is security. If a user can break your code by pasting in insane long string (when INT_MAX < SIZE_MAX) it represents a remote possibility.

  5. Pedantic: The bool approach (@Bobby) will not overflow the digit count for insanely high number of digits like the present code.

Critique of coding goals - not code

  1. Do not understand why the coding goal does not allow "123", "123.", ".123".

  2. strtol() will accept leading white-space.

  3. Indicating the address of the fail location sounds like a useful enhancement. E. g. return p on failure, NULL on success.

share|improve this answer
    
+1: Good Comments; Thank you. I like the pedantic (A.1) and strtol (B.2) and return NULL (B.3). On B.1, "123" and ".123" are expected to be accepted, only "123." is not. –  Arun May 1 at 4:39
    
Corrected. My mis-read of the program flow, though my mistake, was mis-inteprettting decimals > 0) ? ((digits > 0) && (digitsAfterDecimal > 0)) : (digits > 0). I like @Bobby idea to use boolean when only 2 states matter. OTOH the end may be decimals ? (digits && digitsAfterDecimal) : digits; or even (decimals && digitsAfterDecimal) || digits. OT3H, if "123." was OK, then return digits. Thanks for the feedback. –  chux May 1 at 14:37
    
Suggest: To test cases add those pesky empty cases of assert( ! isnumeric( "" ) );, assert( ! isnumeric(NULL) ); –  chux May 1 at 14:40
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.