6
\$\begingroup\$

I'm a beginning C programmer. I need a function that will take a string and determine if it's a number. The data is coming from files created by different people and the number formats WILL vary. So the function needs to decide if it was Supposed to be a number and if so, I will call the right conversion function (atoi/strtoi, atol/strtol etc etc.) So it needs to consider ints and floats, some with dollar signs, some with commas, some not. If a human would recognize it as a number the function should too.

The function works exactly as I want it to, and I haven't been able to break it no matter what I throw at it, but I'm sure there are better ways to do the job, and I'm equally sure there are problems with the way I'm doing it that I just am too inexperienced to know about yet. So Please, take a look and let me know of any potential pitfalls I may encounter with this function, feel free to suggest alternative ways if you're inclined.

Here's what I have come up with. As I've said it works, the example takes one or more command line args to simplify testing and there is no error checking yet, I will add that to the working program, but for function testing in a controlled setting I'm looking purely for functionality and problems that I might not know about with the way I'm handling the function.

Since the $ is significant to the shell it has to be enclosed in quotes but that won't be a problem when implemented because the data won't be coming from the command line.

Anything I should be aware of, or should consider with this code? More efficient mechanisms for accomplishing this would be appreciated. If there is a library function for this it has escaped my detection but would love to pointed in the right direction.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int check_is_numeric(const char *arr)
{
    //define valid characters for any numeric value
    char numdig[] = {'0', '1', '2', '3', '4', '5',
        '6', '7', '8', '9', ',', '.', '$'};
    int j=0;
    int notnum = 0;
    int hasdec = 0;

    while (arr[j] != '\0')  {
        for(int b = 0; b < 13;b++){
            //use hasdec to indicate a decimal in the string 
            //but don't allow more than one.
            if( arr[j] == numdig[b] && hasdec < 2) {
                if (b == 11) {++hasdec;}
                notnum =0;
                break;
            } else {
                notnum =1;  
            }   
        }
        if(notnum){break;}
        ++j;
    }
    if(!notnum) {
        if(!hasdec){return 1;}
        else {return 2;}        
    } else {
        return 0;       
    }   
}


int main(int argc, char *argv[])
{

    char *argvarray[argc];  
    for(int j=0; j< argc; j++){
        argvarray[j] = malloc(strlen(argv[j] + 1));
        strcpy(argvarray[j], argv[j]);
    }

    int i = 0;
    for(int j=1; j< argc; j++){
        //pass each arg to the function
        //function should accept anything a human would recognize as a number
        //and return 0 for non number, 1 for int and 2 for float
        i = check_is_numeric(argvarray[j]);
        switch(i) {
            case 0:
                printf("%s is not a number.\n", argvarray[j]);
                break;
            case 1:
                printf("%s is an integer.\n", argvarray[j]);
                break;
            case 2:
                printf("%s is a floating point number.\n", argvarray[j]);
                break;
        }
    }           
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ there are other valid number types that have an e|E which can be followed by a +|- or leading 'b|B for binary or leading 0x|0X for hexidecimal (the leading 0x number can also contain: a|A b|B c|C d|D e|E and f|F for digits and what about a leading +|-? And there is a trailing H|h for hexidecimal numbers. \$\endgroup\$ – user3629249 Feb 22 '16 at 23:58
  • \$\begingroup\$ good stuff has been mentioned but it's bound to be said: Have a style, but use it consistently. Sometimes you use whitespace in parenthesis or before braces, sometimes not. If you can't decide on a style, just read one and try to stick to it for the start. Lateron you can change it to what I want. There's K&R, Linux, GNU, ... just google ;-) \$\endgroup\$ – larkey Feb 23 '16 at 22:21
3
\$\begingroup\$
  1. Using strtol() and strtod() would be a preferable way to determine if a string converts to a number. By using various locale settings, code can work with thousands separators too. This is a advanced task for a learner, so we will stick with your code.

  2. Avoid magic numbers. Rather than code 13, use a derived limit.

    // for(int b = 0; b < 13;b++){
    for(int b = 0; b < sizeof numdig / sizeof numdig[0] ;b++){
    
  3. Avoid negated variable names. Rather than notnum --> valid_num

  4. For boolean variables like notnum/valid_num, use tpye bool. Be sure to include stdbool.h

  5. Use standard functions like strspn(). This replaces the entire while loop. Make numdig[] both static and const for better optimization and greater clarity as to its use.

    static const char numdig[] = {"0123456789,$."};
    bool hasdec = false;
    bool valid_num = arr[strspn(arr, numdig)] == '\0';
    
    // search for dp separate
    if (valid_num) {
      char *p = strchr(arr, '.');
      // dp found, see if another exist.
      if (p) {
        hasdec = true;
        valid_num = strchr(p+1, '.') == NULL;
      }
    }
    
  6. Good use of const in function signature.

    int check_is_numeric(const char *arr)
    
  7. Surprised signs '-' and '+' were not allowed.

  8. Surprised optional leading whitespace was not allowed; very idiomatic in C.

  9. Tests. I would have like to see the coding goals specifics and some of your sample test data to assess how thorough the claim "function works exactly as I want it to". I suspect input like "1,234,,456.0" will pass.

  10. Advanced: '.' as decimal point and ',' as thousands separators is locale dependent. Research <local.h> for details to make a function that works outside the "C" locale.

    char dp = localeconv()->decimal_point[0];
    
  11. Code looks like it ends with no return. Drop else. Avoid !.

     if(valid_num) {
       if(hasdec) return 2;
       return 1;
     }
     return 0;       
    
\$\endgroup\$
  • \$\begingroup\$ WOW! Much more thorough than I ever imagined I'd get. TONS of fantastic pointers there. THANK YOU so much for taking the time to give such a terrific review and lesson. #9 is correct, and I knew about that when I posted but had planned to address that later. #8 Leading whitespace isn't much of an issue with the data I'm getting, but now that you mention it, probably a good idea to allow for. I've got quite a few things to research now. Thanks again!! \$\endgroup\$ – Jack Galt Feb 22 '16 at 13:20
3
\$\begingroup\$

Handling decimals

Here, the comment gives the impression that you allow one decimal in the input, but the code allows two:

//use hasdec to indicate a decimal in the string 
//but don't allow more than one.
if( arr[j] == numdig[b] && hasdec < 2) {

For inputs . .. ... the program gives:

. is a floating point number.
.. is a floating point number.
... is not a number.

I don't think any of these is a floating point number.

Simplify

A simpler way to write numdig:

char * numdig = "0123456789,.$";

And in the main function I don't understand why you copied argv to argvarray. You can drop the copying and use argv directly.

Clarity

The code doesn't make it clear where the numbers come from in b < 13 and b == 11. You could make them clear by putting in variables with descriptive names. For example numdig_len for 13.

For the number 11, there's no really good name. When it's difficult to find a good name, that usually indicates a problem. What is this 11 anyway? It's the position of . inside numdig. That's a bit obscure way to refer to a .. Why not just use '.'?

And this brings us to another point: why are unrelated things mixed in numdig? It would be better to separate:

  • digits
  • things to ignore, such as $ and ,
  • decimal point

That brings us to other issues as well...

Logical issues

Why allow $ anywhere in the input? That isn't right. If we ignore as first and last character, that could make sense. Ignoring anyway, and ignoring multiple of it doesn't make much sense.

The program is a bit forgiving with respect to commas. For example it reports ,,, as integer. That isn't right.

Using constants

numdig could be a constant, defined at the top, outside of functions:

const char * numdig = "0123456789,.$";

More importantly, it would be good to move the return values 0, 1, 2 to constants with descriptive names.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your time and well thought critique. This is and CHUX review both exceeded my wildest expectations. I'm indebted to you both for the valuable insight. I never thought to try only .. but you're right when entered that way it does accept two decimals. Great catch. Several more items I definitely need to research, but I'm encouraged by the support I've gotten here. I hope I can give back in kind to the community in the future. \$\endgroup\$ – Jack Galt Feb 22 '16 at 13:27
  • \$\begingroup\$ @JackGalt glad you liked the reviews. If you haven't done it already, you can upvote all the answers that you liked, to reward them. In addition, you can mark one of the answers as accepted, which will give an extra reward. Choose wisely, but feel free to pick your favorite, the one that helped you the most. \$\endgroup\$ – janos Feb 22 '16 at 13:51
3
\$\begingroup\$

As far as the C programming language goes, check_is_numeric is a quite good function name. I could spend some time talking about a few variable names through your code that could stand to be a little bit better (as people around here know, I'm a stickler for writing readable code).

But we can also use types as a way to improve the readability and self-documentation of our code base.

Your check_is_numeric function has a return type of int. Based on reading the function name, I'd assume a bool return type: true or false (and be disappointed that you're not using a bool). There's no comments to go along with the function, and I'm not sure where I'd find any technical documentation. So, it might be extraordinarily easy for me to miss the fact that your function even differentiates between integer and floating point numbers. I could very easily have written the following...

if (check_is_numeric(3.14159)) {

And I'd never notice that this is returning 2, which will still behave as expected here if we are working under the assumption that the function simply returns true or false.

Even if we do spend some time playing with the function and see that we sometimes get 1 and sometimes get 2 and sometimes get 0, it might be difficult to notice what these values mean (especially if there are some bugs). So, importantly, I could incorrectly make the assumption that your function returns 0 for non-numbers, and non-zero for numbers, and that number returned is insignificant.

Or we could make our code self-documenting with an enum which clearly self-describes what the return value means:

typedef enum { 
    NON_NUMBER, 
    INTEGER_NUMBER, 
    FLOATING_POINT_NUMBER
} number_type;

And now, our function returns this type:

number_type check_is_numeric(const char * string) {
    // ...
    return NON_NUMBER;
    // or...
    return INTEGER_NUMBER;
    // or...
    return FLOATING_POINT_NUMBER;
}
\$\endgroup\$
  • \$\begingroup\$ I really like the use of the enum. I see what you're saying about the self-documenting and that's something I definitely need to work on. Thank you for the pointers, and mostly thank you for taking the time to help out a newcomer. It is appreciated more than you know. \$\endgroup\$ – Jack Galt Feb 22 '16 at 13:36
2
\$\begingroup\$

Consider reading about Finite-State Machines. You basically keep track of a state, which can be represented as a flow-chart, with various ways of changing state from one node to another. For example, to parse a number, you'd have a chart that looks something like this:

A simple FSM

By following the arrows, you'll note that you can only have a -, . or a digit at the beginning. If you chose the -, the next character must be a digit. If you chose a digit, you can have any number of digits after that, followed by a . or \0 at the end of the string. Note that there are two states for digits, one of them before and one after the . state. This ensures that there can be at most one . in the number. If at any point you receive a character that is not valid (there is no arrow) from the current state, you can immediately conclude that the string is not a valid number.

This is a very simple example. For example, it's optional whether you want to allow a \0 immediately after a ., or allow the first character to be a .. You could also add another state if you don't want to allow leading 0s, or any other number rules you want to enforce.

This is, of course, a completely different approach to text parsing than the code you've provided, but you asked for suggestions for other mechanisms, so hopefully this can give you some insight into another way, which is commonly used by code parsers for exactly this sort of task.

\$\endgroup\$
  • \$\begingroup\$ +1 Lots of ways to implement a state machine in the answers to this SO question. \$\endgroup\$ – luser droog Feb 23 '16 at 3:46
  • \$\begingroup\$ @Darrel Hoffman - If I could buy you a drink I would! As a 30 year industrial automation engineer, the application of FSM is something I do every single day in every single PLC program I've ever written. In fact nearly all PLC programming can be distilled down to FSM. It just never occurred to me to apply it to generic entities like strings and numbers in C (or any other language) programming. My mind is abuzz with ideas on how to change the way I think about and approach coding in general now. Thank you!! You've probably altered the entire direction of my C learning experience. \$\endgroup\$ – Jack Galt Feb 23 '16 at 10:07
  • \$\begingroup\$ @JackGalt - I had to write a pseudo-Pascal compiler as a school assignment back in the day, and at its core was a FSM parser very much like the one above, though more complicated as it also handled every possible symbol, keyword, etc. that the language supported. This is how many professional compilers work, including for C/C++. There's other steps afterwards, of course, but that's a topic for an entire semester at least... \$\endgroup\$ – Darrel Hoffman Feb 23 '16 at 14:36

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.