FizzBuzz, but with provided start and end of sequence, results stored in array, and a separate function to convert integers to strings

Question

To learn C, I'm doing the traditional FizzBuzz challenge with extra restrictions:

The start and end of sequence is provided by the user
Instead of directly printing the results, the results must be stored in an array
There must be a separate function that converts integers to strings

For reference, the original challenge is:

Write a program that prints the integers from 1 to 100. For multiples of three, print Fizz. For multiples of five, print Buzz. For multiples of both three and five, print FizzBuzz. For the rest, print the number.

The idea is to practice string and array processing, since as I understand it, they are not first-class citizens in C, so they are treated differently when compared to more modern languages. These extra restrictions will also help me practice pointers.

The range is provided as command line arguments (two integers, start and end). Here is the code:

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

#define FBSIZE 9

// Convert an integer to its corresponding FizzBuzz string
char* numToFizzBuzz(int num) {
    char* fizzBuzz = malloc(FBSIZE);
    bool isDivBy3 = num % 3 == 0;
    bool isDivBy5 = num % 5 == 0;

    if (isDivBy3 && isDivBy5) {
        strcpy(fizzBuzz, "FizzBuzz");
    } else if (isDivBy3) {
        strcpy(fizzBuzz, "Fizz");
    } else if (isDivBy5) {
        strcpy(fizzBuzz, "Buzz");
    } else {
        sprintf(fizzBuzz, "%d", num);
    }

    return fizzBuzz;
}

// Returns a sequence in the specified range, with each integer processed
// according to the FizzBuzz rules
char** fizzBuzzRange(int from, int to) {
    size_t length = to - from + 1;
    size_t arraySize = length * FBSIZE;
    char** fbRange = malloc(arraySize);

    for (int index = 0; index < length; index++) {
        fbRange[index] = numToFizzBuzz(index + from);
    }

    return fbRange;
}

// Run program and process command line arguments
int main(int argc, char** argv) {
    if (argc < 3) {
        printf("please enter two arguments: start and end of range\n");
        return 1;
    }

    int from = strtol(argv[1], NULL, 10);
    int to = strtol(argv[2], NULL, 10);
    if (from > to) {
        printf("end of range must be greater than the start\n");
        return 1;
    }

    char** fbRange = fizzBuzzRange(from, to);
    for (int index = 0; index < to - from + 1; index++) {
        printf("%s\n", fbRange[index]);
        free(fbRange[index]);
    }
    free(fbRange);

    return 0;
}

To compile and run with gcc:

gcc -o fb.o fizz-buzz.c
./fb.o 1 100

This will print FizzBuzz strings from 1 to 100 (the answer to the original challenge), but any start and end of the range can be provided, like

./fb.o -30 25

I'm looking for anything that you think can be improved, from memory management (which I'm not sure if I'm doing correctly), to best practices, bugs, code smells, optimizations, alternatives, etc. Anything that you think can help improve the program in any way is very welcome!

Do you want pretty or fast code? Most of the ways to optimize this goes against best coding practices like using built-in functions etc. — jdt, Commented Oct 10, 2021 at 19:34

Toby Speight · Accepted Answer · 2021-10-10 20:48:11Z

The calls to malloc() fail to check whether a null pointer was returned by using it. This gives undefined behaviour.

The calculation is wrong here, where fbRange is an array of pointers, not an array of `char[FBSIZE]:

size_t arraySize = length * FBSIZE;
char** fbRange = malloc(arraySize);

To ensure that the calculation is correct, it's usual to ask the compiler for the right size:

char** fbRange = malloc(sizeof *fbRange * length);

On the good side, Valgrind does confirm that the code correctly frees everything it allocates.

There's no checking that the command-line arguments are actually convertible to integers:

int from = strtol(argv[1], NULL, 10);
int to = strtol(argv[2], NULL, 10);

We need to pass an actual end-pointer as second argument, and check that it points to a null character after the conversion (and that we didn't pass an empty string).

Compiling with full analysis reveals some aspects that can easily be improved (some mentioned above; some not):

gcc-11 -std=c17 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion -fanalyzer  -Wstrict-prototypes       268859.c    -o 268859
268859.c: In function ‘fizzBuzzRange’:
268859.c:30:21: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
   30 |     size_t length = to - from + 1;
      |                     ^~
268859.c:34:31: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
   34 |     for (int index = 0; index < length; index++) {
      |                               ^
268859.c: In function ‘main’:
268859.c:48:16: warning: conversion from ‘long int’ to ‘int’ may change value [-Wconversion]
   48 |     int from = strtol(argv[1], NULL, 10);
      |                ^~~~~~
268859.c:49:14: warning: conversion from ‘long int’ to ‘int’ may change value [-Wconversion]
   49 |     int to = strtol(argv[2], NULL, 10);
      |              ^~~~~~
268859.c: In function ‘numToFizzBuzz’:
268859.c:15:9: warning: use of possibly-NULL ‘fizzBuzz’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
   15 |         strcpy(fizzBuzz, "FizzBuzz");
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-6
    |
    |   42 | int main(int argc, char** argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   43 |     if (argc < 3) {
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch (when ‘argc > 2’)...
    |......
    |   48 |     int from = strtol(argv[1], NULL, 10);
    |      |                           ~
    |      |                           |
    |      |                           (3) ...to here
    |   49 |     int to = strtol(argv[2], NULL, 10);
    |   50 |     if (from > to) {
    |      |        ~
    |      |        |
    |      |        (4) following ‘false’ branch (when ‘from <= to’)...
    |......
    |   55 |     char** fbRange = fizzBuzzRange(from, to);
    |      |                      ~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (5) ...to here
    |      |                      (6) calling ‘fizzBuzzRange’ from ‘main’
    |
    +--> ‘fizzBuzzRange’: events 7-10
           |
           |   29 | char** fizzBuzzRange(int from, int to) {
           |      |        ^~~~~~~~~~~~~
           |      |        |
           |      |        (7) entry to ‘fizzBuzzRange’
           |......
           |   34 |     for (int index = 0; index < length; index++) {
           |      |                         ~~~~~~~~~~~~~~
           |      |                               |
           |      |                               (8) following ‘true’ branch...
           |   35 |         fbRange[index] = numToFizzBuzz(index + from);
           |      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                          |
           |      |                          (9) ...to here
           |      |                          (10) calling ‘numToFizzBuzz’ from ‘fizzBuzzRange’
           |
           +--> ‘numToFizzBuzz’: events 11-15
                  |
                  |    9 | char* numToFizzBuzz(int num) {
                  |      |       ^~~~~~~~~~~~~
                  |      |       |
                  |      |       (11) entry to ‘numToFizzBuzz’
                  |   10 |     char* fizzBuzz = malloc(FBSIZE);
                  |      |                      ~~~~~~~~~~~~~~
                  |      |                      |
                  |      |                      (12) this call could return NULL
                  |......
                  |   14 |     if (isDivBy3 && isDivBy5) {
                  |      |        ~
                  |      |        |
                  |      |        (13) following ‘true’ branch...
                  |   15 |         strcpy(fizzBuzz, "FizzBuzz");
                  |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |         |
                  |      |         (14) ...to here
                  |      |         (15) argument 1 (‘fizzBuzz’) from (12) could be NULL where non-null expected
                  |
<built-in>: note: argument 1 of ‘__builtin_memcpy’ must be non-null
268859.c:17:9: warning: use of possibly-NULL ‘fizzBuzz’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
   17 |         strcpy(fizzBuzz, "Fizz");
      |         ^~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-6
    |
    |   42 | int main(int argc, char** argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   43 |     if (argc < 3) {
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch (when ‘argc > 2’)...
    |......
    |   48 |     int from = strtol(argv[1], NULL, 10);
    |      |                           ~
    |      |                           |
    |      |                           (3) ...to here
    |   49 |     int to = strtol(argv[2], NULL, 10);
    |   50 |     if (from > to) {
    |      |        ~
    |      |        |
    |      |        (4) following ‘false’ branch (when ‘from <= to’)...
    |......
    |   55 |     char** fbRange = fizzBuzzRange(from, to);
    |      |                      ~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (5) ...to here
    |      |                      (6) calling ‘fizzBuzzRange’ from ‘main’
    |
    +--> ‘fizzBuzzRange’: events 7-10
           |
           |   29 | char** fizzBuzzRange(int from, int to) {
           |      |        ^~~~~~~~~~~~~
           |      |        |
           |      |        (7) entry to ‘fizzBuzzRange’
           |......
           |   34 |     for (int index = 0; index < length; index++) {
           |      |                         ~~~~~~~~~~~~~~
           |      |                               |
           |      |                               (8) following ‘true’ branch...
           |   35 |         fbRange[index] = numToFizzBuzz(index + from);
           |      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                          |
           |      |                          (9) ...to here
           |      |                          (10) calling ‘numToFizzBuzz’ from ‘fizzBuzzRange’
           |
           +--> ‘numToFizzBuzz’: events 11-17
                  |
                  |    9 | char* numToFizzBuzz(int num) {
                  |      |       ^~~~~~~~~~~~~
                  |      |       |
                  |      |       (11) entry to ‘numToFizzBuzz’
                  |   10 |     char* fizzBuzz = malloc(FBSIZE);
                  |      |                      ~~~~~~~~~~~~~~
                  |      |                      |
                  |      |                      (12) this call could return NULL
                  |......
                  |   14 |     if (isDivBy3 && isDivBy5) {
                  |      |         ~~~~~~~~~~~~~~~~~~~~
                  |      |                  |
                  |      |                  (13) following ‘false’ branch (when ‘isDivBy5 == 0’)...
                  |   15 |         strcpy(fizzBuzz, "FizzBuzz");
                  |   16 |     } else if (isDivBy3) {
                  |      |               ~
                  |      |               |
                  |      |               (14) ...to here
                  |      |               (15) following ‘true’ branch (when ‘isDivBy3 != 0’)...
                  |   17 |         strcpy(fizzBuzz, "Fizz");
                  |      |         ~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |         |
                  |      |         (16) ...to here
                  |      |         (17) argument 1 (‘fizzBuzz’) from (12) could be NULL where non-null expected
                  |
<built-in>: note: argument 1 of ‘__builtin_memcpy’ must be non-null
268859.c:19:9: warning: use of possibly-NULL ‘fizzBuzz’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
   19 |         strcpy(fizzBuzz, "Buzz");
      |         ^~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-6
    |
    |   42 | int main(int argc, char** argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   43 |     if (argc < 3) {
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch (when ‘argc > 2’)...
    |......
    |   48 |     int from = strtol(argv[1], NULL, 10);
    |      |                           ~
    |      |                           |
    |      |                           (3) ...to here
    |   49 |     int to = strtol(argv[2], NULL, 10);
    |   50 |     if (from > to) {
    |      |        ~
    |      |        |
    |      |        (4) following ‘false’ branch (when ‘from <= to’)...
    |......
    |   55 |     char** fbRange = fizzBuzzRange(from, to);
    |      |                      ~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (5) ...to here
    |      |                      (6) calling ‘fizzBuzzRange’ from ‘main’
    |
    +--> ‘fizzBuzzRange’: events 7-10
           |
           |   29 | char** fizzBuzzRange(int from, int to) {
           |      |        ^~~~~~~~~~~~~
           |      |        |
           |      |        (7) entry to ‘fizzBuzzRange’
           |......
           |   34 |     for (int index = 0; index < length; index++) {
           |      |                         ~~~~~~~~~~~~~~
           |      |                               |
           |      |                               (8) following ‘true’ branch...
           |   35 |         fbRange[index] = numToFizzBuzz(index + from);
           |      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                          |
           |      |                          (9) ...to here
           |      |                          (10) calling ‘numToFizzBuzz’ from ‘fizzBuzzRange’
           |
           +--> ‘numToFizzBuzz’: events 11-17
                  |
                  |    9 | char* numToFizzBuzz(int num) {
                  |      |       ^~~~~~~~~~~~~
                  |      |       |
                  |      |       (11) entry to ‘numToFizzBuzz’
                  |   10 |     char* fizzBuzz = malloc(FBSIZE);
                  |      |                      ~~~~~~~~~~~~~~
                  |      |                      |
                  |      |                      (12) this call could return NULL
                  |......
                  |   16 |     } else if (isDivBy3) {
                  |      |               ~
                  |      |               |
                  |      |               (13) following ‘false’ branch (when ‘isDivBy3 == 0’)...
                  |   17 |         strcpy(fizzBuzz, "Fizz");
                  |   18 |     } else if (isDivBy5) {
                  |      |               ~
                  |      |               |
                  |      |               (14) ...to here
                  |      |               (15) following ‘true’ branch (when ‘isDivBy5 != 0’)...
                  |   19 |         strcpy(fizzBuzz, "Buzz");
                  |      |         ~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |         |
                  |      |         (16) ...to here
                  |      |         (17) argument 1 (‘fizzBuzz’) from (12) could be NULL where non-null expected
                  |
<built-in>: note: argument 1 of ‘__builtin_memcpy’ must be non-null
268859.c:21:9: warning: use of possibly-NULL ‘fizzBuzz’ where non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
   21 |         sprintf(fizzBuzz, "%d", num);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-6
    |
    |   42 | int main(int argc, char** argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   43 |     if (argc < 3) {
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch (when ‘argc > 2’)...
    |......
    |   48 |     int from = strtol(argv[1], NULL, 10);
    |      |                           ~
    |      |                           |
    |      |                           (3) ...to here
    |   49 |     int to = strtol(argv[2], NULL, 10);
    |   50 |     if (from > to) {
    |      |        ~
    |      |        |
    |      |        (4) following ‘false’ branch (when ‘from <= to’)...
    |......
    |   55 |     char** fbRange = fizzBuzzRange(from, to);
    |      |                      ~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (5) ...to here
    |      |                      (6) calling ‘fizzBuzzRange’ from ‘main’
    |
    +--> ‘fizzBuzzRange’: events 7-10
           |
           |   29 | char** fizzBuzzRange(int from, int to) {
           |      |        ^~~~~~~~~~~~~
           |      |        |
           |      |        (7) entry to ‘fizzBuzzRange’
           |......
           |   34 |     for (int index = 0; index < length; index++) {
           |      |                         ~~~~~~~~~~~~~~
           |      |                               |
           |      |                               (8) following ‘true’ branch...
           |   35 |         fbRange[index] = numToFizzBuzz(index + from);
           |      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                          |
           |      |                          (9) ...to here
           |      |                          (10) calling ‘numToFizzBuzz’ from ‘fizzBuzzRange’
           |
           +--> ‘numToFizzBuzz’: events 11-17
                  |
                  |    9 | char* numToFizzBuzz(int num) {
                  |      |       ^~~~~~~~~~~~~
                  |      |       |
                  |      |       (11) entry to ‘numToFizzBuzz’
                  |   10 |     char* fizzBuzz = malloc(FBSIZE);
                  |      |                      ~~~~~~~~~~~~~~
                  |      |                      |
                  |      |                      (12) this call could return NULL
                  |......
                  |   16 |     } else if (isDivBy3) {
                  |      |               ~
                  |      |               |
                  |      |               (13) following ‘false’ branch (when ‘isDivBy3 == 0’)...
                  |   17 |         strcpy(fizzBuzz, "Fizz");
                  |   18 |     } else if (isDivBy5) {
                  |      |               ~
                  |      |               |
                  |      |               (14) ...to here
                  |      |               (15) following ‘false’ branch (when ‘isDivBy5 == 0’)...
                  |......
                  |   21 |         sprintf(fizzBuzz, "%d", num);
                  |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  |      |         |
                  |      |         (16) ...to here
                  |      |         (17) argument 1 (‘fizzBuzz’) from (12) could be NULL where non-null expected
                  |
In file included from 268859.c:4:
/usr/include/stdio.h:334:12: note: argument 1 of ‘sprintf’ must be non-null
  334 | extern int sprintf (char *__restrict __s,
      |            ^~~~~~~
268859.c: In function ‘fizzBuzzRange’:
268859.c:35:24: warning: dereference of possibly-NULL ‘fbRange’ [CWE-690] [-Wanalyzer-possible-null-dereference]
   35 |         fbRange[index] = numToFizzBuzz(index + from);
      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-6
    |
    |   42 | int main(int argc, char** argv) {
    |      |     ^~~~
    |      |     |
    |      |     (1) entry to ‘main’
    |   43 |     if (argc < 3) {
    |      |        ~
    |      |        |
    |      |        (2) following ‘false’ branch (when ‘argc > 2’)...
    |......
    |   48 |     int from = strtol(argv[1], NULL, 10);
    |      |                           ~
    |      |                           |
    |      |                           (3) ...to here
    |   49 |     int to = strtol(argv[2], NULL, 10);
    |   50 |     if (from > to) {
    |      |        ~
    |      |        |
    |      |        (4) following ‘false’ branch (when ‘from <= to’)...
    |......
    |   55 |     char** fbRange = fizzBuzzRange(from, to);
    |      |                      ~~~~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (5) ...to here
    |      |                      (6) calling ‘fizzBuzzRange’ from ‘main’
    |
    +--> ‘fizzBuzzRange’: events 7-11
           |
           |   29 | char** fizzBuzzRange(int from, int to) {
           |      |        ^~~~~~~~~~~~~
           |      |        |
           |      |        (7) entry to ‘fizzBuzzRange’
           |......
           |   32 |     char** fbRange = malloc(arraySize);
           |      |                      ~~~~~~~~~~~~~~~~~
           |      |                      |
           |      |                      (8) this call could return NULL
           |   33 | 
           |   34 |     for (int index = 0; index < length; index++) {
           |      |                         ~~~~~~~~~~~~~~
           |      |                               |
           |      |                               (9) following ‘true’ branch...
           |   35 |         fbRange[index] = numToFizzBuzz(index + from);
           |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                        | |
           |      |                        | (10) ...to here
           |      |                        (11) ‘fbRange + (long unsigned int)index * 8’ could be NULL: unchecked value from (8)
           |

Heinzi · Accepted Answer · 2021-10-11 08:12:05Z

3

Just one issue, which has not been mentioned in the other answers yet:

#define FBSIZE 9
...
char* fizzBuzz = malloc(FBSIZE);
...
sprintf(fizzBuzz, "%d", num);

On systems with a large int data type, the size of num can overflow the space allocated with malloc, resulting in an out-of-bounds write. You can protect against that by using snprintf instead:

snprintf(fizzBuzz, FBSIZE, "%d", num);

If you want, you can check the return value to see if an overflow occurred and react accordingly.

answered Oct 11, 2021 at 8:12

Heinzi

5663 silver badges10 bronze badges

1

\$\begingroup\$ Additionally, you could find the correct size for the buffer using sprintf(NULL, 0, "%d", INT_MAX). \$\endgroup\$
– Toby Speight
Commented Oct 11, 2021 at 8:22

Add a comment |

jdt · Accepted Answer · 2021-10-10 20:43:18Z

I’ll start with the basics.

Your code returns an array of strings when it could have simply returned a string. A C-style string is an array that is terminated with a zero character.

You are coping data into fizzBuzz and then appending fizzBuzz into fbRange. You can write most of the data straight to fbRange.

Consider replacing fizzBuzzRange with something like this:

#define FBSIZE 15

char* fizzBuzzRange(size_t from, size_t to) {
    size_t length = to - from + 1;
    size_t arraySize = length * FBSIZE;
    char buf[FBSIZE];
    char* fbRange = (char*)malloc(arraySize);
    char* dst = fbRange;
    for (size_t index = from; index < to; index++) {
        bool isDivBy3 = index % 3 == 0;
        bool isDivBy5 = index % 5 == 0;

        if (isDivBy3 && isDivBy5) {
            memcpy(dst, "FizzBuzz", 8);
            dst += 8;
        }
        else if (isDivBy3) {
            memcpy(dst, "Fizz", 4);
            dst += 4;
        }
        else if (isDivBy5) {
            memcpy(dst, "Buzz", 4);
            dst += 4;
        }
        else {
            sprintf(buf, "%d", index);
            size_t len = strlen(buf);
            memcpy(dst, buf, len);
            dst += len;
        }
        *dst++ = '\n';
    }
    *dst = '\0';
    return fbRange;
}

int main() {
    char* fb = fizzBuzzRange(1, 100);
    printf("%s", fb);
}

There are still lots of optimizations to be made (see here) but that is where it starts to get complicated =)

Stack Exchange Network

FizzBuzz, but with provided start and end of sequence, results stored in array, and a separate function to convert integers to strings

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged
c
strings
array
pointers
fizzbuzz
or ask your own question.

Hot Network Questions

FizzBuzz, but with provided start and end of sequence, results stored in array, and a separate function to convert integers to strings

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged cstringsarraypointersfizzbuzz or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c
strings
array
pointers
fizzbuzz
or ask your own question.