1
\$\begingroup\$

Here was my first, simple C fizzbuzz:

#include <stdio.h>
void main() {
  for (int n = 1; n <= 100; ++n) {
    if (n % 3 == 0) printf("%s", "fizz");
    if (n % 5 == 0) printf("%s", "buzz");
    if ((n % 3 != 0) && (n % 5 != 0)) printf("%d", n);
    printf("\n");
  }
}

I was messing around, trying to separate the transformation logic from the looping logic. As a whole, I don't think that my second Buffer-based solution is necessarily "better" as a FizzBuzz solution, but I did like the way that I dealt with a limited-purpose character buffer.

Thoughts on the second solution?

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

#define BUFF_SZ 9

typedef unsigned int u32;
typedef int i32;

/// A string buffer with a notion of its own size.
typedef struct Buffer {
  char *s;
  u32 sz;
  char *ins_ptr;
} Buffer;

/// Returns the content of the buffer.
const char *Buffer_content(Buffer *);

/// Clears contents of the buffer.
void Buffer_clear(Buffer *);

/// Appends a string S to the end of the buffer.
void Buffer_append_str(Buffer *, char *);

/// Appends the string representation of an integer to the end of the buffer.
void Buffer_append_i32(Buffer *, i32);

/// Returns 1 if the buffer has space for a certain number of chars, else 0.
u32 Buffer_has_space(Buffer *, u32);


/* HIGH LEVEL CODE */


/// Writes a number's fizzbuzz transformation into a Buffer.
void write_fizzbuzz_transformation(i32 n, Buffer *buff) {
  Buffer_clear(buff);
  if (n % 3 == 0) Buffer_append_str(buff, "fizz");
  if (n % 5 == 0) Buffer_append_str(buff, "buzz");
  if (*Buffer_content(buff) == '\0') Buffer_append_i32(buff, n);
}

void main() {
  // setup the string buffer (stack allocated)
  char internal_buff[BUFF_SZ];
  Buffer buff;
  buff.s = internal_buff;
  buff.sz = BUFF_SZ;


  for (i32 i = 1; i <= 100; ++i) {
    write_fizzbuzz_transformation(i, &buff);
    printf("%d -> %s\n", i, Buffer_content(&buff));
  }
}


/* DETAILS -- IMPLEMENTATION CODE */


u32 Buffer_has_space(Buffer *buff, u32 requested_sz) {
  u32 available_sz = buff->sz - (buff->ins_ptr - buff->s) - 1;
  return (available_sz >= requested_sz);
}

void Buffer_append_str(Buffer *buff, char* s) {
  u32 needed_sz = strlen(s);
  assert(Buffer_has_space(buff, needed_sz));
  strcpy(buff->ins_ptr, s);
  buff->ins_ptr += needed_sz;
}

void Buffer_append_i32(Buffer *buff, i32 n) {
  i32 original = n;
  u32 needed_sz = 0;
  if (n < 1) ++needed_sz;
  while (n) {
    n /= 10;
    ++needed_sz;
  }
  assert(Buffer_has_space(buff, needed_sz));
  sprintf(buff->ins_ptr, "%d", original);
  buff->ins_ptr += needed_sz;
}

void Buffer_clear(Buffer *buff) {
  buff->ins_ptr = buff->s;
  *(buff->ins_ptr) = '\0';
}

const char *Buffer_content(Buffer *buff) {
  return buff->s;
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Short comment: (as a fizzbuzz solution) it's insane. \$\endgroup\$ – Jerry Coffin May 14 '16 at 18:33
3
\$\begingroup\$

Misleading

Why would You something like this:

typedef unsigned int u32;
typedef int i32;

That's quite misleading. On 64bit machines it's probably not going to be 32 bit wide. Standard uint32_t, int32_t types would be the way to go, if You really need fixed sized integers. Also, typedefing primitive types like that just sucks and kills readability.

Also, You're using unsigned int for storing size. Why not just size_t?

Readability

Typedefing structs as well kills a bit of readability.

What's char *s;? Start? String? Digging through Your code doesn't make it clearer, especially when You have something like this:

void Buffer_clear(Buffer *buff) {
  buff->ins_ptr = buff->s;
  *(buff->ins_ptr) = '\0';
}

What does this mean: if (*Buffer_content(buff) == '\0')? Something like if (Buffer_isEmpty(buff)) would be waaaaay clearer.

Why would You mix casing in function names? void write_fizzbuzz_transformation(), but const char *Buffer_content(), etc.

Also, decide how do You form Your function names. noun_verb, verb_noun, or whatever. Now it's... quite inconsistent. Again, write_fizzbuzz_transformation(), but Buffer_*().

Generic rant

assert() is not intended to be used for production code. It's used only for debugging purposes and only when NDEBUG is defined. Why wouldn't You have proper logger for errors like... not enough space and... wrong input? In fact, You have no control of wrong inputs because none of those are sanitized properly. For example imagine user of Your buffer library does something stupid like Buffer_clear(NULL), Buffer_content(NULL), etc.

In production code You have potential buffer overrun here:

void Buffer_append_str(Buffer *buff, char* s) {
  u32 needed_sz = strlen(s);
  assert(Buffer_has_space(buff, needed_sz));
  strcpy(buff->ins_ptr, s);
  buff->ins_ptr += needed_sz;
}

As I said before, assert() is used only for debugging purposes. You should use strncpy() or check buffer size explicitly. Same applies to void Buffer_append_i32(Buffer *buff, i32 n).

Also, it would be nice if You'd provide proper error codes or 0 on success on Your buffer operations.

Functionality

You could shorten the following and make it safer using snprintf():

u32 needed_sz = 0;
if (n < 1) ++needed_sz;
while (n) {
  n /= 10;
  ++needed_sz;
}
assert(Buffer_has_space(buff, needed_sz));
sprintf(buff->ins_ptr, "%d", original);

You have #define BUFF_SZ 9, but accepting unsigned integers, which are... by Your typedefs --- 32 bit long. So, anything over 99 999 999 goes to abyss...

Warnings

./fizz.c:45:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
void main() {
     ^
\$\endgroup\$
  • \$\begingroup\$ Lotta great feedback, thanks! Whose responsibility is safety, though, caller or callee? As callee, I could catch the special case of Buffer_clear(NULL), but I can't catch the general case of casting any other invalid value as a Buffer pointer. In other words, other than a null check, library callee can't guarantee that the passed pointer points to a valid Buffer (or can it)? \$\endgroup\$ – brian_o May 16 '16 at 16:53
  • \$\begingroup\$ Nope, You can't. Usually, there is proper constructor and destructor provided, like Buffer_create(), Buffer_destroy() with idea that the library is going to deal only with Buffer handles created using those. Or, in other other words, only valid Buffer is created using Buffer_create(), if You're messing things in other ways, You're on Your own. \$\endgroup\$ – Kamiccolo May 16 '16 at 17:10
  • \$\begingroup\$ Thanks. Did some more reading about assert. I think you meant "only when NDEBUG is [not] defined." \$\endgroup\$ – brian_o May 16 '16 at 18:42
  • \$\begingroup\$ Concerning "You should use strncpy() or check buffer size explicitly.": Incorrect usage of strncpy() is rampant, so unless one provides an example or references proper usage, this advice is not constructive. \$\endgroup\$ – chux May 16 '16 at 20:22
  • \$\begingroup\$ sprintf(buff->ins_ptr, "%d", original); without checking its result trades one form of error for another without overall code improvement. Again not truly constructive. \$\endgroup\$ – chux May 16 '16 at 20:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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