snippetcMinor
Algorithm (and tests) to parse lists of unsigned 32-bit ints from strings in C
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:
The algorithm uses a custom data structure called
The
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;
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,52The 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
Like this:
-
Use a
-
Don't use literal constants ("magic numbers"). Why does your function return
-
I'd probably rename
-
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
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.