Find and print all numbers in a comma-delimited string using sscanf

Question

I want to find all of the whole numbers in a string that starts with { and ends with }, and uses , to separate each number.

I use sscanf to find and match each item in the string—for example:

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

int main(void)
{

    const char *str =  "{0,1,22,333,4,55,6,77777,8}";
    int tmp, tmp2, tmp3, number;

    printf("filtered number:");

    tmp = 0;
    while(1)
    {
        if( *str == '{' )
        {
            /* begin of current str */
            tmp2 = 0;
            tmp3 = 0;
        }

        if( *str == ',' || *str == '}' )
        {
            /* found a number now */
            tmp++;

            /* get how far the ptr move */
            //printf("ptr moved step: %d\n", tmp2);

            /* get the digit len */
            tmp3 = tmp2 - tmp3 - 1;

            /* get the number */
            sscanf(str-tmp3, "%d", &number);
            printf("%d ", number);

            /* record current ptr possition */
            tmp3 = tmp2;
        }

        if ( *str == '}' ) break;

        str++;
        tmp2++;
    }
    printf("\n");
    printf("howm any number found: %d\n", tmp);

    return 0;
}

Questions:

1. Any risk in my code?
2. Is there a smarter way to do this? Can my code be optimized?

Answer 1

Some general comments about your current code:

• You just dumped everything in the main method. This makes your code difficult to read, and more importantly, prevents it from being re-used. You should have factored the logic out into a separate method, which you would then call from main. This method would take the input string as a parameter, and then either do the printing directly, or use output parameters to return the parsed data.

• Your variable names are…terrible. tmp, tmp2, tmp3 provide no semantic information whatsoever about the purpose of that variable. All that tells me is that they are "temporary", but all local variables are temporary! What temporary values do these variables hold? Choosing meaningful names for your variables goes a long way towards making your code readable—and bug free!

• Your commenting style is inconsistent. You can use either C89-style comments (/* ... */) or C99-style comments (// ...), but whichever one you use, stick with it! I would prefer the C99-style comments, since these are easier to write, less error-prone when editing, and ubiquitously supported by C compilers (even Microsoft's, which has notoriously incomplete support for other C99 syntax).

• Speaking of comments, it looks like you are using comments to remove your debugging code. A better way to do that would be to use conditional compilation, based on the definition of a DEBUG symbol (defined for debug builds; undefined otherwise). For example:

  #ifdef DEBUG
      printf("ptr moved step: %d\n", tmp2);
  #endif  // DEBUG
  

  You could go one better by defining a TRACE or DEBUG_OUTPUT (or whatever you want to call it) macro, and then using this each time you want to print debugging output. This makes your code cleaner because you don't have to have all the preprocessor statements littered throughout.

• Prefer to declare variables at the point of initialization. This allows you to limit the scope and prevents the inadvertent use of uninitialized variables.

  Speaking of uninitialized variables, if you had been compiling with warnings enabled (by passing -Wall or a similar option, depending on your compiler), you would have seen a warning that tmp2 and tmp3 may be uninitialized at the point of use. Since the use of uninitialized variables is undefined behavior, this is something that you want to avoid at all costs.

• There are typos, grammatical errors, and inconsistent formatting (trailing spaces, etc.) in your output strings. This makes your code—and even your finished application—look unprofessional. Even if English is not your first language, you should carefully proofread any output strings that the user is going to see. Maybe even get someone to look over them. (You know, like in a Code Review! :-p)

• Combine back-to-back calls to printf into a single call. For example, this code:

  printf("\n");
  printf("howm any number found: %d\n", tmp);
  

  should instead be written simply as (grammar fixed, too):

  printf("\nTotal numbers found: %d\n", tmp);
  

  printf is an extremely slow function, so for both readability and performance, you should avoid calling it multiple times when you don't actually need to!

A major improvement using strtod:

You have made this way too difficult! You don't need to write nearly as much of your own parsing code if you leverage the power of the standard library, which has powerful string-manipulation functions.

One of the comments mentions using strtod. That is an option, but the sample input you provided suggests that these are integer values, not floating point values. Therefore, strtol would be a better choice. By passing 0 as the base (third parameter), it will automatically detect the correct representation for the value, which is the most flexible.

Here is my first pass at rewriting your code:

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

void FindAndPrintNumbers(const char *psz)
{
   // The beginning of the list should be marked by an open curly brace.
   assert(*psz == '{');

   // Parse and print out each of the numbers in the list,
   // keeping track of how many were found in all.
   printf("Filtered number(s): ");
   int numbersFound = 0;
   while (*psz++ != '}')
   {
      ++numbersFound;

      char* pszNext;
      printf("%ld ", strtol(psz, &pszNext, 0));
      psz = pszNext;

      // The next character should either be a comma or closed curly brace.
      // If not, the input string is malformed.
      assert((*psz == ',') || (*psz == '}'));
   }

   printf("\nTotal numbers found: %d\n", numbersFound);
}

Note that I tend to use semantic Hungarian prefixes for my variables—in particular, psz means "pointer to a string that is zero-terminated. You can either like this style or hate this style. Rename them if desired; it won't offend me.

Also, I had to introduce a temporary pszNext variable because the strtol function is not const-correct. In particular, although the first argument is a const pointer, the second parameter is a non-const pointer-to-a-pointer. This is a defect in the C standard library, and you just have to work around it, making sure not to inadvertently invoke undefined behavior by modifying the string through that non-const pointer! This makes the code uglier than I'd like, but the only other choice would be a cast, and that's arguably even uglier!

Finally, my code just assumes that the input string will be in the format you've shown. That is, it will begin with an open curly brace ({), will contain an arbitrary sequence of integer literals separated by commas (,) with no spaces, and will end with a closed curly brace (}). There are assertions here to enforce these assumptions, but the behavior is left formally undefined if a malformed input string is passed in. If you want to be more robust in what you handle, you will need to add extra error checks. Your original code wasn't robust in the face of invalid inputs, either, so I assumed this wasn't important.