FizzBuzz with debugging output

Question

This is a pretty simple FizzBuzz variant. I'm more interested in my use of C (this is my first bit of C code) than I am in my actual FizzBuzz implementation, although I'm always open to suggestions.

In particular I'm pretty certain that my string manipulations are horrific, but I don't really know where to start improving.

I've also added in a little debug utility that I've created, with the purpose of implementing some stuff with header files, as those are completely new to me. All it does is toggle whether a print method will do anything, based on a global flag; I'm not sure if this is actually useful, or if I'm overlooking a better equivalent.

---

FizzBuzz Source

#include <string.h>
#include "debug_util.c"
void evaluate(char*, int, int, char*);
const int MAX_STRING_LEN = 17;
const int MAX_WORD_LEN = 7;
const int MAX_DIGIT_COUNT_LEN = 4;
const bool LOCAL_DEBUG_FLAG = false;
int main()
{
 DEBUG = LOCAL_DEBUG_FLAG;
 int i;
 char output[MAX_STRING_LEN];
 char intVal[MAX_DIGIT_COUNT_LEN];
 printDebug("Initial Loop Start\n");
 for (i = 1; i <= 100; i++)
 {
 printDebug("Setting initial output to null\n\n");
 /* Set first value in array to null termination,
 * effectively clearing the array,
 * if it is only read as a sequential string */
 output[0] = '0円';
 snprintf(intVal, MAX_DIGIT_COUNT_LEN,"%d",i);
 evaluate(output, 3, i, "Fizz");
 evaluate(output, 4, i, "Buzz");
 evaluate(output, 5, i, "Sizzle");
 output[sizeof(output)] = '0円';
 if (output[0] == '0円')
 {
 strncat(output, intVal, MAX_DIGIT_COUNT_LEN);
 }
 printf("%s\n", output);
 }
}
void evaluate(char *buf, int factor, int value, char *result)
{
 printDebug("Evaluating || Factor: %d, Value: %d, Success Result: %s\n", factor, value, result);
 char temp[MAX_WORD_LEN];
 strncpy(temp, ((value % factor == 0) ? result : ""), MAX_WORD_LEN -1);
 printDebug("Evaluating complete || Result: \"%s\"\n\n", temp);
 //Check array is not empty
 if (value % factor == 0)
 {
 strncat(buf, temp, MAX_STRING_LEN -1);
 }
}

---

Debug Util Header

#ifndef C_EXAMPLES_DEBUG_UTIL_H
#define C_EXAMPLES_DEBUG_UTIL_H
#ifndef _STDIO_H_
#include <stdio.h>
#endif
#include <zconf.h>
#ifndef _STDBOOL_H
#include <stdbool.h>
#endif
extern bool DEBUG;
#endif //C_EXAMPLES_DEBUG_UTIL_H

---

Debug Util C Source

#include "debug_util.h"
bool DEBUG = false;
//Only prints if the global DEBUG flag has been set to true
//Replicates code from printf()
int printDebug(const char *format, ...)
{
 int done = 0;
 if (DEBUG)
 {
 va_list arg;
 va_start (arg, format);
 done = vfprintf (stdout, format, arg);
 va_end (arg);
 }
 return done;
}

Answer 1

#includeing source files

A very big issue - especially if you're planning on writing a library:

You include a C source file to use printDebug. You shouldn't do that to avoid possible name collisions between the #includeing file and the #included file, as all of the advantages you had from the header guards are absent when you import the source file itself instead of the header.

The header represents the public interface of the code, so always include that to avoid name collision issues and not depend on implementation details, like internal functions (not that you do that now, but for the future).

The problem with doing this right now would be that you have not put your function declarations in the header. Declare printDebug in the header (I guess you already know how to do this, as you've already done it for evaluate), and you're ready to use it from your main source - just include debug_util.h instead of debug_util.c.

Weird and missing #includes

What is that #include <zconf.h> doing in debug_util.h? as far as I could find out, it's for the compression library zlib, which you don't seem to be using. Also, you should #include <stdarg.h> in debug_util.h to get standards-compliant behavior and compiler portability for the va_* macros you use in debug_util.c.

You should #include <stdio.h> in your main file too, as you're using printf. The header guard is in debug_util.h to save you from the multiple-include problem.

Style and Convention

• Your coding style is fine, but you should use int main(void) to indicate that main takes no parameters, as main() indicates (counterintuitively) that main can take any number of parameters.

• return 0; at the end of main is only optional in C99 mode and above, so you might want to document that.

• ALL_CAPS names are generally reserved for macro definitions - use normal variable naming for constants.

• DEBUG as declared in debug_util.c and debug_util.h could be const bool - don't assign it in debug_util.c and just leave it for the user to do it.

• Or you could use a DEBUG macro definition and #ifdef/#ifndef checks to do this, as every debug library does by convention; using #ifdef checks means you can even choose to leave out the compilation of printDebug in release builds (it can be an empty function in release builds given its conditional definition, which would cause the call to printDebug to be optimized away.

  An example of this idea:

  debug_util.h

  ---

  #ifndef DEBUG_UTIL_H
  #define DEBUG_UTIL_H
  #ifndef _STDIO_H_
  #include <stdio.h>
  #endif
  #ifndef _STDBOOL_H
  #include <stdbool.h>
  #endif
  int printDebug(const char*, ...); 
  #endif //DEBUG_UTIL_H
  

  debug_util.c

  ---

  #include "debug_util.h"
  //Only prints if the global DEBUG flag has been set to true
  //Replicates code from printf()
  int printDebug(const char *format, ...)
  {
  #ifdef DEBUG
   int done = 0;
   va_list arg;
   va_start (arg, format);
   done = vfprintf (stdout, format, arg);
   va_end (arg);
  #else
   return 1;
  #endif
  }
  

  In the main code, just do a #define DEBUG 1 after #include "debug_util.h", or pass the -DDEBUG=1 option to gcc when compiling the main code.

• J Lewis: Could you clarify what you meant by that last point? I've implemented all the other changes

Answer 2

At least 4 C string coding mishaps

Writing outside output[]. The value range of setting a char array element is0 to sizeof(output) - 1.

char output[MAX_STRING_LEN];
output[sizeof(output)] = '0円'; // bad

With a[0]=0, then strncat(a,s,n) can write up to n+1 characters, the last being a 0円'.

char output[MAX_STRING_LEN];
output[0] = '0円';
...
if (output[0] == '0円') {
 // strncat(output, intVal, MAX_DIGIT_COUNT_LEN); // bad
 strncat(output, intVal, MAX_DIGIT_COUNT_LEN - 1);
}

Failure to insure temp[] is a string. strncpy(a,b,n) does not insure that a '0円' is written. Since char temp[MAX_WORD_LEN] does not initialize the elements of the array, by the time code gets to printDebug(), temp[] may lack a null character. Code needs to add it.

char temp[MAX_WORD_LEN];
strncpy(temp, ..., MAX_WORD_LEN -1);
temp[sizeof temp - 1] = '0円'; // ** add **
printDebug("Evaluating complete || Result: \"%s\"\n\n", temp);

Wrong value passed as n in strncat(s1,b,n)

(strncat) the maximum number of characters that can end up in the array pointed to by s1 is strlen(s1)+n+1.

evaluate(char *buf, int factor, int value, char *result) {
 ...
 // strncat(buf, temp, MAX_STRING_LEN -1);
 size_t len = stlen(buf);
 strncat(buf, temp, MAX_STRING_LEN - len -1);

---

Improvement idea: pass into evaluate() the size of the buffer available to buf rather than use MAX_STRING_LEN

void evaluate(char *buf, /* add */ size_t size, int factor, int value, char *result)

Improve naming: In C, use ...LEN as the length of the string which does not include the string's null character. Use ...SIZE, ...SZ to denote the size of the string which does include the string's null character.

// char output[MAX_STRING_LEN];
// Alternatives
char output[MAX_STRING_LEN + 1];
char output[MAX_STRING_SIZE];

Improve debug: When printing a string, add sentinel characters. Easier to see any unexpected leading/trailing whitespace.

//printDebug("Evaluating || Factor: %d, Value: %d, Success Result: %s\n",
printDebug("Evaluating || Factor: %d, Value: %d, Success Result: <%s>\n",

Use const when able. As the string pointer to by result is not modified, add const to allow wider application and potential performance improvements.

// void evaluate(char *buf, int factor, int value, char *result);
void evaluate(char *buf, int factor, int value, const char *result);

Unclear why the indirect string empty test

//Check array is not empty
// if (value % factor == 0) // strange test.
// alternative
if (buf[0] == '0円')

---

In general, OP is attempting to use strncat()/strncpy() where it is not the best tool. Although named "str-n-cpy" and sounds like it should do string copy respecting a buffer size of n, it does not. Although named "str-n-cat" and sounds like it should form a string by appending 2 string respecting a buffer size of n, it does not.

Certainly this is to be "safer" code, yet with incorrect usage code would have been as "safe" with strcpy()/strcat().

Consider other approaches. In particular, when a string operation encounters an insufficient buffer, simply truncating the result is weak. Better to fail or report the error.

strncpy(command, "Launch the rocket not", 17);

Answer 3

in C, an array index is in the range: 0...number of items in array -1. So this statement:

output[sizeof(output)] = '0円';

will write past the end of the upper bound of the array. This results in undefined behavior and can lead to a seg fault event.

this kind of line:

snprintf(intVal, MAX_DIGIT_COUNT_LEN,"%d",i);

would be better written (and more flexible) as:

snprintf(intVal, sizeof( intVal ),"%d",i);

The system header files already have 'include guards' and header files should only #include other header files that are needed inside the current header file. so this file:

#ifndef C_EXAMPLES_DEBUG_UTIL_H
#define C_EXAMPLES_DEBUG_UTIL_H
#ifndef _STDIO_H_
#include <stdio.h>
#endif
#include <zconf.h>
#ifndef _STDBOOL_H
#include <stdbool.h>
#endif
extern bool DEBUG;
#endif //C_EXAMPLES_DEBUG_UTIL_H

should be reduced to:

#ifndef C_EXAMPLES_DEBUG_UTIL_H
#define C_EXAMPLES_DEBUG_UTIL_H
#include <stdbool.h>
extern bool DEBUG;
#endif //C_EXAMPLES_DEBUG_UTIL_H

Then those other header files should be included in the source file(s) where their contents are actually being used.