7
\$\begingroup\$

This is the first time I have recoded in C for several years and I would like to know how clean my code is.

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

#define EXIT_SUCCESS 0

void fizzbuzz(int n) {
    char result[9] = "";  // strlen("fizzbuzz") + 1 == 9
    if (n % 3 == 0) strcat(result, "fizz");
    if (n % 5 == 0) strcat(result, "buzz");
    if (!*result) sprintf(result, "%d", n);
    printf("%s\n", result);
}

int main() {
    for (int i = 1; i <= 100; i++) fizzbuzz(i);
    return EXIT_SUCCESS;
}

I know that I could include stdlib so as not to have to define EXIT_SUCCESS myself, but as I only needed this constant, it's a little clearer and more efficient to do this I think.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ C/C++ are particularly unforgiving languages. It's your responsibility to abide by the rules or face the consequences. This is why, even in trivial cases, I would rather not deliberately violate a rule, even as simple as redefining a reserved constant. You'll get away with it more often that not, but you might be very sorry the day you won't. Besides, C compilation is very fast, including a couple more headers should make no noticeable difference. \$\endgroup\$
    – kuroi neko
    Jul 27 at 9:21
  • \$\begingroup\$ @kuroineko Re: compare return values in pre C99 return 0 or 1: see Any C compiler where “==” evaluates to larger than one?. \$\endgroup\$ Jul 27 at 23:54
  • \$\begingroup\$ Oh, good to know. I didn't realize even the K&R defined the return value of these operators. Anyway, I shied from the printf hair-splitting tedium. Pissing contests are not my thing, and who cares about the 1.000.000th variant of fizzbuzz anyway? \$\endgroup\$
    – kuroi neko
    Jul 27 at 23:58
  • \$\begingroup\$ @kuroineko It's worth pointing out that K&R-style C is hardly modern though. The C standard changed a bit since '89 as well. \$\endgroup\$
    – Mast
    Jul 29 at 5:39
  • \$\begingroup\$ Seriously, give me a break. K&R style was already a thing of the past in the 90's. Knowing when the return value of == was first defined is just an amusing bit of lore, with zero practical value. \$\endgroup\$
    – kuroi neko
    Jul 29 at 6:00
11
\$\begingroup\$
 #define EXIT_SUCCESS 0

You are not permitted to define standard library names yourself. So just include <stdlib.h>, or omit the return from main() (remember, C will provide a success return if you don't provide one).

void fizzbuzz(int n) {

We could give that internal linkage:

static void fizzbuzz(int n) {
char result[9] = "";  // strlen("fizzbuzz") + 1 == 9

Instead of manually calculating, we could use sizeof "fizzbuzz" to let the compiler do that for us.

    if (!*result) sprintf(result, "%d", n);

We should be using snprintf() here, as the decimal representation of n can be longer than result has capacity for (even on common platforms with 32-bit int).

I would avoid writing to result in this case - we can format directly to stdout:

    if (*result) {
        puts(result);
    } else {
        printf("%d\n", n);
    }

main should be specific about its arguments:

 int main(void)

Modified code

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

void fizzbuzz(int n)
{
    char result[sizeof "fizzbuzz"] = "";
    if (n % 3 == 0) { strcat(result, "fizz"); }
    if (n % 5 == 0) { strcat(result, "buzz"); }
    if (*result) {
        puts(result);
    } else {
        printf("%d\n", n);
    }
}

int main(void)
{
    for (int i = 1;  i <= 100;  ++i) {
        fizzbuzz(i);
    }
}
\$\endgroup\$
9
  • 4
    \$\begingroup\$ Not permitted by the C standard (it's undefined behaviour, IIRC). Compilers are not obliged to detect that you do so, and you probably get away with it in most cases. Default return from main() (and only from main()) is standard C, since C99 I think. So all compliant compilers in 2021 should accept that. \$\endgroup\$ Jul 26 at 10:49
  • 1
    \$\begingroup\$ I'll try and find a reference for that. However, be aware that the standard library headers are allowed to include each other (e.g. <stdio.h> is allowed to transitively include <stdlib.h>) so you cannot say for definite what's not included. \$\endgroup\$ Jul 26 at 11:03
  • 2
    \$\begingroup\$ @Toby In C++, headers may include any additional header they want. C doesn't have (nor really need) that provision. \$\endgroup\$ Jul 26 at 20:48
  • 2
    \$\begingroup\$ "Even with all the compiler options enabled, I don't get an error, not permitted by whom?" - If that's your attitude, you're in for a world of pain with C ;) - There's lots of things in C that won't trigger a compiler error (or even warning), but are still prohibited and lead to hard to debug problems. Most Undefined Behavior pitfalls fall in this category. \$\endgroup\$
    – marcelm
    Jul 27 at 8:50
  • 1
    \$\begingroup\$ One good thing you've done "silently" is to put braces round every conditionally-executed section. To the OP: This is considered good practise under every coding standard I'm aware of, because a common error is to start with a single-line conditionally-executed statement, then need to add a second line and forget to put both lines within braces. The result if you do that is that the first line is conditionally executed (depending on the "if" statement or whatever), but the second line isn't covered by the condition and is always executed. \$\endgroup\$
    – Graham
    Jul 27 at 8:59
1
\$\begingroup\$

Re-use

could include stdlib so as not to have to define EXIT_SUCCESS myself, but as I only needed this constant, it's a little clearer and more efficient to do this

Consider someone uses your good code as part of a larger task and does include <stdlib.h>, then the compiler may warn about the redefinition. Now the next coder needs to spend time sorting this out.

Tip: plan for code re-use and consider the next guy. Avoid re-implementing the standard library.


Buffer too small

char result[9] .... sprintf(result, "%d", n) --> result too small for all int. Be more generous.

Various ways to pre-calculate buffer needs.

 #define INT_STRING_N (sizeof(int)*CHAR_BIT/3 + 3)
 #define FIZZBUZZ_STRING_N 9
 #define BUFF_N (INT_STRING_N > FIZZBUZZ_STRING_N ? INT_STRING_N : FIZZBUZZ_STRING_N)

 char result[BUFF_N];

I favor using 2x buffers (twice the expected max needed size) and snprintf() for less controlled cases.

\$\endgroup\$
1
\$\begingroup\$
  1. If you can include the appropriate header, do that. Redefining yourself is error-prone even where allowed. This is C not C++, headers are simple enough it doesn't significantly influence compile-time.

  2. If you add the origin of a constant in a comment, try to instead let the compiler determine it from the true source for you.

     char result[9] = "";  // strlen("fizzbuzz") + 1 == 9
    

    becomes the simpler and shorter

     char result[sizeof "fizzbuzz"] = "";
    
  3. If you can, avoid copying strings around. Your pattern has period 3*5==15, so use an array. You can even compress it if you want, though that is overkill.

  4. return 0; is implicit for main() since C99. While return EXIT_SUCCESS; might be a different successful execution, afaik there is no implementation which differentiates.

This is what it looks like when you pander to the little tin god, without completely loosing your head:

#include <stdio.h>

void fizzbuzz(unsigned n) {
#define Z "\n\0"
#define F "fizz"
#define X "%u" Z
    const char* text = X F Z F "buzz\n";
    static const char index[] = {
        sizeof(X F Z),   1,         1,         sizeof(X), 1,
        sizeof(X F Z F), sizeof(X), 1,         1,         sizeof(X),
        sizeof(X F Z F), 1,         sizeof(X), 1,         1,
    };
#undef X
#undef F
#undef Z
    printf(text + index[n % 15] - 1, n);
}

int main() {
    for (unsigned i = 1; i < 100; ++i)
        fizzbuzz(i);
}

Or after encoding data and offsets with a custom program (we are now far beyond overkill), it becomes this:

#include <stdio.h>

void fizzbuzz(unsigned n) {
    const char* p = "\31\17\17\23\17\35\23\17\17\23\35\17\23"
        "\17\17%u\n\0fizz\n\0fizzbuzz\n";
    printf(p + p[n % 15], n);
}

int main() {
    for (unsigned i = 1; i < 100; ++i)
        fizzbuzz(i);
}

The difficult part was converting the calculated string into a valid short source-code literal. Trickier than one might think.

\$\endgroup\$
5
  • \$\begingroup\$ I was worried that printf("fizz\n", 3); might be undefined behaviour, but it is not. \$\endgroup\$
    – Ken Y-N
    Jul 27 at 1:43
  • \$\begingroup\$ I am not familiar with the idiom "pander to the little tin god"; what does this mean? \$\endgroup\$
    – Cody Gray
    Jul 27 at 4:20
  • \$\begingroup\$ Think of what can be accomplished with all that performance gain! \$\endgroup\$
    – t_d_milan
    Jul 27 at 7:39
  • \$\begingroup\$ @CodyGray The little tin god means that in the end, it takes far more prominence than deserved. And in this case, it is obviously the little tin god of efficiency. \$\endgroup\$ Jul 27 at 9:19
  • \$\begingroup\$ @t_d_milan That was obviously never the point. \$\endgroup\$ Jul 27 at 9:28

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.