7
\$\begingroup\$

The rules are unsurprisingly:

Write a program that prints the numbers from 0 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”."

#include <stdio.h>

int main(void) {
    int i, j;
    i = j = 0;

    for (; i < 100 ; ++i, j = 0, putchar('\n')) {
        j += printf("%s", i % 3 ? "" : "Fizz");
        j += printf("%s", i % 5 ? "" : "Buzz");
        if (!j) printf("%d", i);
    }

    return 0;
}

If you were a job interviewer, would you be ok with an answer like this? Or would you ask for another solution that is not (ab)using the semantics of for and the return value of printf in order to do fewer operations?

\$\endgroup\$

We are looking for answers that provide insightful observations about the code in the question. Answers that consist of independent solutions with no justification do not constitute a code review, and may be removed.

  • \$\begingroup\$ Is it just me or will this begin by printing a zero? \$\endgroup\$ – Simon Forsberg Jan 4 '17 at 17:58
  • \$\begingroup\$ Any reason for why you are writing it in this way in the first place? \$\endgroup\$ – Simon Forsberg Jan 4 '17 at 17:59
  • \$\begingroup\$ Yes it will start by printing 0 and it was done on purpose . I carelessly copy pasted the problem wording. I wrote it this way because I think it use only two divisions. I perfectly agree that this is bad code for more than one reason but I wanted to have external opinions about it. \$\endgroup\$ – 永劫回帰 Jan 4 '17 at 18:11
  • \$\begingroup\$ @cHao Comments are for seeking clarification to questions. All critiques and opinions about the code belong in answers — no matter how short. \$\endgroup\$ – 200_success Jan 4 '17 at 18:19
  • \$\begingroup\$ "I perfectly agree that this is bad code for more than one reason but I wanted to have external opinions about it." — This question may run afoul of the "Do I want the code to be good code? (i.e. not obfuscation)" rule stated in the help center. \$\endgroup\$ – 200_success Jan 4 '17 at 18:24
16
\$\begingroup\$

The for loop seems fine to me; it isn't abusing any semantics. The semantics of the for loop are quite clear:

for (initialization; loop condition; increment)

Anyone who doesn't know this off the top of their head probably doesn't know the language as well as they think they do. Although you could have written this code using a while loop, it's just a matter of opinion whether that is better. In general, you can transform any for loop into a while loop. When I write a "complex" for loop, I tend to prefer to break each of these components into separate lines for readability (and also to keep the line from getting too long).

However, using the return value of printf the way you did is just wrong. It returns a negative value if the operation failed. You are adding arbitrary positive and negative return values together and checking to see whether the result is non-zero. That is incorrect, and thus decidedly unclever.

You could might say that the test condition could be modified to j > 0, but that's still not precisely correct because one of the printf calls might succeed and return, say, 5, while the other one might fail and return −5. Should the number be printed, or not?

So, while I have no problem with your choice to use a for loop here, I do not like what you've done in the body of that loop. I disagree with cHao's comment that this would be an instant no-hire. I'd probably assume you were a competent programmer and having fun with the inherently-boring FizzBuzz problem. However, I would expect you, with a bit of prompting, to see and begin to explain some of the problems with your chosen approach.

Aside from that…

  • Keep in mind that code does not become more efficient just because you reduce the number of apparent instructions that you write in C. Optimizing compilers do not perform a literal translation of C instructions into assembly instructions. And even non-optimizing compilers may not be able to do so if they wanted to, because there isn't necessarily a 1-to-1 correspondence of instructions in both languages.

    In this case, you're not even gaining any efficiency with the way you've written the printf statements. In fact, the execution of printf is by far the slowest part of the program, so by any real-world performance metrics, avoiding calls to printf whenever possible would be the way to boost performance. Certainly it is not a win to replace a reasonably well-predicted conditional branch with an expensive and pointless function call!

    In fact, if you weren't trying to be overly clever, you could substitute puts for printf where you were outputting constant "Fizz" and "Buzz" strings, which would be even more efficient. (Some compilers like GCC will do this translation for you when they can, but they wouldn't be able to do it with the way you'd originally written the code, obviously.)

  • Stylistically, I would prefer that you kept the variables to as narrow a scope as possible. Since at least C99, you have been allowed to declare variables within a for loop's scope, and this is what I would recommend that you do.

  • Even more importantly, I prefer to see that variables are initialized at the point of their declaration whenever possible. It bothers me to see lines like:

    int i, j;
    i = j = 0;
    

    I don't like the attempt to combine initialization of multiple variables onto one line, either. Break them up into two lines. No excuses.

\$\endgroup\$
  • \$\begingroup\$ The comment don't exist anymore, maybe just remove the link to the comment and just say you disagree with the idea? \$\endgroup\$ – Marc-Andre Jan 4 '17 at 18:35
  • \$\begingroup\$ I agree with what you said. I indeed was careless about the case where printf may fail (nevertheless I did not use puts because it adds a newline at the end so I would have to make a special case for "FizzBuzz" and I did not want to have a special case). But yes, it would be more efficient and the return value would have predicable but not directly usable the way I did. \$\endgroup\$ – 永劫回帰 Jan 4 '17 at 18:47
  • \$\begingroup\$ @CodyGray Ah, yes. I didn't spot that immediately. \$\endgroup\$ – πάντα ῥεῖ Jan 4 '17 at 18:47
  • \$\begingroup\$ Agree with the review. I still say no hire, though. If someone's gonna show off, it'd better be technically flawless. \$\endgroup\$ – cHao Jan 4 '17 at 21:32
  • 2
    \$\begingroup\$ printf only returns a negative value if the operation fails. If a write to stdout fails, something has gone dreadfully wrong, and it's not clear to me that attempted recovery is worthwhile for a FizzBuzz. Assuming a working environment where stdout can be written to, is there anything wrong with the algorithm here? \$\endgroup\$ – Kyle Strand Jan 4 '17 at 23:06

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.