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

Algorithm (and tests) to parse lists of unsigned 32-bit ints from strings in C

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

Problem

What?

I have an algorithm that parses comma separated lists of unsigned 32-bit ints from strings. Here is an example of what a list could look like:

1302,51,2312,2,52


The algorithm uses a custom data structure called parse_input which is a data structure that is a FIFO of numbers. This structure is passed to another unrelated function that parses some data with the help of this list of numbers. parse_input_push is used to add a number to the queue.

The byte_array structure used in the tests has a separate code review.

I would like feedback on everything. Even the tests!

Files

Source

```
#define MAX_NUM_GLYPHS 11

static bool number_len_ok(const char *literal_num, size_t curr_num_len)
{
if (curr_num_len != MAX_NUM_GLYPHS - 1) return true;

size_t i = 0;
char *max_literal_num = "4294967296";

for (; i max_literal_num[i]) return false;

Solution

-
Having tests is good. I recommend using a test framework, for example googletest.

-
You are obviously using C99 (otherwise parts of your code wouldn't compile). C99 does not require variable instantiation at the beginning of a scope. You should generally create your variables as late as possible, to limit their scope. This includes moving your for loop counters into the loop.

Like this:

for (size_t i = 0; i  max_literal_num[i]) return false;
}


-
Use a const instead of #define. That way you get normal scope rules, easier debugging, more readable compilation errors and so on.

-
Don't use literal constants ("magic numbers"). Why does your function return -2 and -1? Give them names instead: return PARSE_ERROR;. Why do you memset 10 bytes of literals, when its size is MAX_NUM_GLYPHS?

-
I'd probably rename number_len_ok() to something along is_number_too_large() (and inverted the return values).

-
You are assuming that numbers are located together and in order in the character set. It doesn't necessarily have to be that way (although it almost always is).

-
Are the input strings allowed to have spaces? You don't have any tests for that.

-
Your parsing function could probably be written in a bit cleaner and/or more readable way. This is left as an exercise to the reader :-)

-
You're missing #includes for memset, atoi etc.

Code Snippets

for (size_t i = 0; i < curr_num_len; i++) {
    if (literal_num[i] > max_literal_num[i]) return false;
}

Context

StackExchange Code Review Q#28694, answer score: 6

Revisions (0)

No revisions yet.