18
\$\begingroup\$

I've just finished my first program written in Java. It's a simple, boring, yet well-known FizzBuzz challenge where you have to:

Write a program that prints the numbers from 1 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”."

I know there are some other FizzBuzz questions here, but I'd really like to start learning Java in the right way, which means that I'm looking for:

  • Naming conventions
  • Coding standards and best practices

Coming from Python world, I was tempted to create a separate method named fizzBuzz() which I then called in my main() method. Is this a bit overkill for such a simple task?

package fizzbuzz;

class FizzBuzz {
/*
    Write a program that prints the numbers from 1 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”."
 */

    public static void fizzBuzz() {
        for(int i = 1; i <= 100; i++) {
            if (i % 3 == 0 && i % 5 == 0)
                System.out.println("FizzBuzz");
            else if (i % 3 == 0)
                System.out.println("Fizz");
            else if (i % 5 == 0)
                System.out.println("Buzz");
            else
                System.out.println(i);
        }
    }

    public static void main(String[] args) {
        fizzBuzz();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I always try to get out of the main method and file as fast as possible. To me, that place should only be used to launch the app and nothing else. So yes, creating a separate method or class is the right approach. \$\endgroup\$ – Ben C. R. Leggiero Jan 4 '17 at 13:46
  • 7
    \$\begingroup\$ An enterprise version of FizzBuzz in Java can be found here: github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition \$\endgroup\$ – fafl Jan 4 '17 at 15:51
  • 11
    \$\begingroup\$ If your FizzBuzz isn't boring, you did it wrong. \$\endgroup\$ – jpmc26 Jan 4 '17 at 22:53
  • 1
    \$\begingroup\$ You could shorten the if..else if you use a String variable and concatenate using ternary: String strOut = ""; strOut = ((i % 3) ? "Fizz" : "") + ((i % 5) ? "Buzz" : ""); System.out.println((strOut.length() > 0) ? strOut : i). I'm not sure how this stands up to everyone's scrutiny, but it reads cleaner to me than an if..else. \$\endgroup\$ – Daevin Jan 5 '17 at 16:31

10 Answers 10

28
\$\begingroup\$

Coming from Python world, I was tempted to create a separate method named fizzBuzz() which I then called in my main() method. Is this a bit of overkill for such a simple task ?

Well, it is a little bit too much for such a simple task, but it is a good habit to do so and if I were you I would stick to it.


Overall your fizzBuzz looks good.

  • Naming -> Check

but

  • Best Practices -> Fail

This is because you omitted braces {} although they are optional for single instruction if, else if and else branches.

Omitting the braces can lead to hidden, and therefore hard to find bugs. I would like to encourage you to always use them.

Another thing is that you have some magic numbers, like 1, 100, 3, and 5 in your code which should either be extracted to some meaningful named constants, or passed to the method as parameters.

You could add a isDivisibleBy(int value, int divider) method as well to make the responsibilities clear.

\$\endgroup\$
  • 23
    \$\begingroup\$ While 1 and 100 can be passed as parameters, the use of 3 and 5 are specific to this algorithm, and I wouldn't expect them to be extracted, since it's obvious what they're doing. \$\endgroup\$ – Arturo Torres Sánchez Jan 4 '17 at 15:03
  • 4
    \$\begingroup\$ I have to echo @ArturoTorresSánchez's point. Extracting a FizzDivisor and a BuzzDivisor is like creating a function isEven(x, evenDivisor = 2). \$\endgroup\$ – Agop Jan 4 '17 at 16:47
  • 7
    \$\begingroup\$ The “best practice” bit is by no means generally agreed on. For what it’s worth I disagree that always using braces is best practice, and there’s no evidence that it actually prevents bugs. On the flip side, it takes up more vertical space, thus pushing functions over the height of the monitor, which may make code harder to read and, in turn, may lead to bugs. In other words, it’s far from clear which leads to more robust code: always using braces, or omitting optional braces. \$\endgroup\$ – Konrad Rudolph Jan 4 '17 at 18:02
  • 1
    \$\begingroup\$ @KonradRudolph It only adds one line per if/else chain when this bracing style is used. \$\endgroup\$ – Andrew says Reinstate Monica Jan 4 '17 at 18:13
  • 14
    \$\begingroup\$ I have to disagree with the isDivisibleBy function idea, it's unnecessarily verbose, and adds extra complexity. The modulo operation is much clearer. Checking for divisibility is a really basic/common task in programming, and any decent programmer would understand what you're doing there. "Keep It Simple, Stupid" \$\endgroup\$ – Jezzamon Jan 4 '17 at 20:52
26
\$\begingroup\$

Congratulations.

First, because you made it quite easily into the next round of your job interview. That's better than 80% to 90% of all candidates applying for the average software development job apparently. Seriously, a lot of people applying for jobs wouldn't have been able to write that code.

Second, because you resisted the temptation to write an "enterprisy" solution. No builder. No factory. No factory builder, no factory builder factory, just the simple, straightforward solution to a simple, straightforward problem. Giving a solution that requires >50 lines of code, even if bug free, would mean you turned an easy victory into a crushing defeat.

It's a design pattern called "YAGNI" = You Ain't Gonna Need It. You will never, ever in your life need to run this code in any other way, with other numbers than 3, 5 and 100, or with any other words than "Fizz", "Buzz" and "FizzBuzz". Therefore you should not write code that does more than solve the problem.

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm confused... your answer is saying that the OP's code is great and there is nothing to change? Shouldn't this be a comment then? \$\endgroup\$ – Chris Cirefice Jan 5 '17 at 18:47
  • \$\begingroup\$ @ChrisCirefice I think from a You Ain't Gonna Need It perspective he means this is a great answer + a nice poking at his interview :D \$\endgroup\$ – Honey Jan 5 '17 at 19:47
  • \$\begingroup\$ @Honey I get that, but I think "you're code is fine and not overly complex" explained in 150 words still doesn't make an answer. It's really a comment. \$\endgroup\$ – Chris Cirefice Jan 5 '17 at 20:13
  • 4
    \$\begingroup\$ @ChrisCirefice "Your code is fine as it is", stated with justification, is a perfectly acceptable answer on Code Review. It would not be appropriate as a comment, though. \$\endgroup\$ – 200_success Jan 5 '17 at 21:47
  • \$\begingroup\$ @200_success Thanks for the link, I didn't know that :) \$\endgroup\$ – Chris Cirefice Jan 5 '17 at 21:57
6
\$\begingroup\$

Use proper parentheses

if (i % 3 == 0 && i % 5 == 0)

Contains 6 operations in a short line. While one can somewhat easy see what you intend (since we know the code's intention) you will have an awesome bug at hand if you make a mistake when assuming operator precedence.

Improvement 1: Place parentheses

if ( ((i % 3) == 0) && ((i % 5) == 0) )

Now precedence is none of your worries any more, but of course if your condition becomes more complex, it will look increasingly terrible. Also note that you use the expressions

(i % 3) == 0 and (i % 5) == 0

multiple times. Why calculate them twice in the worst case?

Improvement 2: Extract expressions

boolean numberFizzes = ((i % 3) == 0);
boolean numberBuzzes = ((i % 5) == 0);

if(numberFizzes && numberBuzzes){
…
} else if(numberFizzes){
…
} else if(numberBuzzes){
…
} else {
…
}

Improvement 3: Reorder cases to reduce complexity

Why cover the FizzBuzz case first, if it is

  • the most complex
  • composed from two other cases

A simple re-ordering and breaking up the if-else cascade helps you cover the corner case quite nice:

boolean numberFizzes = ((i % 3) == 0);
boolean numberBuzzes = ((i % 5) == 0);

if(numberFizzes){
    // print Fizz
}

if(numberBuzzes){
    // print Buzz
}
// In the FizzBuzz case both will now be printed

if( !(numberFizzes || numberBuzzes) ){
    // print i
}

While in the concrete example the gain is arguably small, the lesson is that problem can be re-constructed to come to other solutions that may have beneficial impact on your code smell.

\$\endgroup\$
  • 16
    \$\begingroup\$ Disagree that if ( ((i % 3) == 0) && ((i % 5) == 0) ) is "proper parenthesis". If I saw that on a interview I'd wonder if the candidate didn't understand precedence or order of operations, and what other trivial things they might not know. \$\endgroup\$ – Nick Jan 4 '17 at 18:37
  • 1
    \$\begingroup\$ Good point about extracting the expressions, but I think numberFizzes is a misleading variable name; I'd expect a variable with that name to hold a number value, not a Boolean. I'd lean instead toward something like isMultipleOfThree. \$\endgroup\$ – Thriggle Jan 4 '17 at 20:02
  • 2
    \$\begingroup\$ @Nick: Interesting, never thought about it this way. I prefer explicit parenthesis (and/or the one action at a time paradigm) to make very clear what I intend. Consequence is of course that you stop thinking about precedence, since it becomes irrelevant. I would consider this more a piece of defensive programming, than actual lack of knowledge. However now you got me interested: which other pieces of knowledge would one probably lack if one can't get the precedence right? \$\endgroup\$ – fer-rum Jan 5 '17 at 10:09
  • 1
    \$\begingroup\$ @Thriggle: Point taken, maybe it may also be called numberIsFizzing or something. If to fizz would be a verb, numberFizzes could be interpreted as numberDoesFizz (what I meant), or numberOfFizzes (what you read). \$\endgroup\$ – fer-rum Jan 5 '17 at 10:15
  • 3
    \$\begingroup\$ Am I taking crazy pills? This is like, five times harder to read, to me. \$\endgroup\$ – Lynn Jan 5 '17 at 15:19
5
\$\begingroup\$

Overkill?

I wouldn't say it is overkill. As Ben C. R. Leggiero says:

I always try to get out of the main method and file as fast as possible. To me, that place should only be used to launch the app and nothing else. So yes, creating a separate method or class is the right approach.

Plus, I can now call FizzBuzz.fizzBuzz() from one of my classes. (Although, on the other hand, I guess one could ask why I would ever need to use your fizzBuzz() function.) I wouldn't do more than though. In this case, it's 2 lines of code more for some re-use and re-enforcement of a good practice... Remember FizzBuzz is usually an interview question... A lot of abstraction is just not going to be needed. There aren't very many use cases for this problem.

Javadocs

You can document your code with Javadocs. The code you have right now doesn't utilize this, and it is pretty trivial to fix your existing comments to do this.

Use braces

This is the most important issue. This:

        if (i % 3 == 0 && i % 5 == 0)
            System.out.println("FizzBuzz");
        else if (i % 3 == 0)
            System.out.println("Fizz");
        else if (i % 5 == 0)
            System.out.println("Buzz");
        else
            System.out.println(i);

Is prone to errors when you don't use braces. Python enforces indentation so it is hard(er?) to make a mistake with the lack of braces, but in Java the indentation doesn't influence anything here and some programs will silently "work" that shouldn't and no error message will be given.

\$\endgroup\$
5
\$\begingroup\$

This escalated quickly. And wow, I've never been called enterprisy before, I'm proud. So, anyway, I'm obviously not recommending anyone to do FizzBuzz like this: I mean, it's obviously an overkill, and a horrible first solution to turn in when asked at an interview. I used the basic problem as a springboard to show OP some concepts (magic constants, encapsulation, builder pattern) and also some language features (constructor delegation, Javadoc) they may not be familiar with. Also, as an interviewer I'd bloody well ask follow-up questions like "but I like my FizzBuzz parametrizable" and "what's wrong with this constructor?", and I'd prefer to get answers like this.


And in addition to the answers posted by @Heslacher and @TimothyTruckle: I would take this even further.

Because, really, what are those magic numbers doing in the code? 3, 5 and 100, all code checkers (checkstyle or PMD) would complain about these. Sure, we could extract them into static constants:

private static final int FIZZ_NUMBER = 3
private static final int BUZZ_NUMBER = 5
private static final int ITERATIONS = 100

But I like my FizzBuzz configurable: the user of our FizzBuzz lib should be able to specify these constants. Likewise, just printing stuff on the console is bad form in any language: why can't I supply my own outputstream?

The obvious solution is to encapsulate the FizzBuzz logic into its own class:

public class FizzBuzz {
    private final int fizzNumber;
    private final int buzzNumber;
    private final int iterations;

    /**
     * Constructor.
     *
     * @param fizzNumber We should print a "fizz" when the current 
     *                   iteration is divisible by this number
     * @param buzzNumber We should print a "buzz" when the current 
     *                   iteration is divisible by this number
     * @param iterations The simulation should run this many cycles.
     */
    public FizzBuzz(int fizzNumber, int buzzNumber, int iterations) {
        this.fizzNumber = fizzNumber;
        this.buzzNumber = buzzNumber;
        this.iterations = iterations;
    }

    /**
     *  Constructor creating a vanilla FizzBuzz, with parameters 3, 5, and 100
     */
    public FizzBuzz() {
        this(3, 5, 100);
    }

    /**
     *  Write fizzes and buzzes on the given output stream.
     *
     *  @param out The outputStream we should write fizzes and buzzes on.
     */
    public void write(OutputStream out) {
         // fizzbuzz logic
    }

    /**
     *  Write fizzes and buzzes on the standard output.
     */
    public void write() {
        this.write(System.out);
    }
}

Call it like this:

FizzBuzz writer = new FizzBuzz(5, 2, 200);
writer.write();

That constructor, though... 5 and 2, which was the Fizz and which was the Buzz? Writing constructors with a relatively large numbers of indistinguishable paramters harms the readability of the code. I'm really fond of the builder pattern of Joshua Bloch:

public class FizzBuzz {
    private FizzBuzz(int fizzNumber, int buzzNumber, int iterations) {
        this.fizzNumber = fizzNumber;
        this.buzzNumber = buzzNumber;
        this.iterations = iterations;
    }

    /* FizzBuzz implementation here. */

    public Builder create() {
        return new Builder();
    }

    public static final class Builder {
        private int fizzNumber = 3;
        private int buzzNumber = 5;
        private int iterations = 100;

        private Builder() {
            // private ctor to hide the default one.
        }

        public Builder withFizzNumberOf(int fizzNumber) {
            this.fizzNumber = fizzNumber;
            return this;
        }

        public Builder withBuzzNumberOf(int buzzNumber) {
            this.buzzNumber = buzzNumber;
            return this;
        }

        public Builder withIterations(int iterations) {
            this.iterations = iterations;
            return this;
        }

        public FizzBuzz done() {
            // perform checks on the parameters here.
            return new FizzBuzz(fizzNumber, buzzNumber, iterations);
        }
    }
}

Call it like this:

FizzBuzz fizzBuzz = FizzBuzz.create()
    .withFizzNumber(7)
    .withBuzzNumber(3)
    .withIterations(200)
    .done();

Oh and, use Javadoc to document your code!

\$\endgroup\$
  • 7
    \$\begingroup\$ This is what I would call 'over-engineering' :) Don't get me wrong, it is a perfectly valid solution if you think your requirements will change. But if you know they won't, don't bother making everything parameterized. \$\endgroup\$ – RobAu Jan 4 '17 at 16:52
  • \$\begingroup\$ @RobAu sure, I got carried away a bit here :). OP asked for idiomatic Java, though, and this is pretty idiomatic Java. Also, everybody seems to put this in the context of an interview: while this is most definitlely a really bad solution to give at first, as an interviewer I'd ask questions like this. \$\endgroup\$ – SáT Jan 4 '17 at 17:57
  • 6
    \$\begingroup\$ I love that this answer puts "fizzbuzz logic" as an implementation detail to be filled in later. \$\endgroup\$ – mattdm Jan 4 '17 at 18:16
  • 8
    \$\begingroup\$ So... This isn't satire...? \$\endgroup\$ – Agop Jan 5 '17 at 0:46
  • 2
    \$\begingroup\$ If you are going to do this the builder should have steps like; addRule(3,"Buzz") \$\endgroup\$ – Taemyr Jan 5 '17 at 13:50
4
\$\begingroup\$

In addition to the answer of @Heslacher which I totally support:

Coming from Python world, I was tempted to create a separate method named fizzBuzz() which I then called in my main() method. Is this a bit of overkill for such a simple task ?

You should have gone one step further.

Java is an object oriented language therefore you are encuraged to (create and) use objects. There are two little changes in your example to do so:

public /* remove static */ void fizzBuzz() {
  // ...
public static void main(String[] args) {
    new FizzBuzz().fizzBuzz();
}

Another point I'd like to stress is visibility scopes.

Your method fizzBuzz() has public visibility which means it can be accessed from anywhere throughout your program. You should always reduce the visibility as much as possible. Since there is no other class (yet ;o)) needing access to this merhod you should replace the public key word by private.


[edit]
The comments to my answer raised another issue:

identifier names should express programmers intention

You named your method fizzBuzz() which is a repetition of the classes name.

But method names should start with a verb.

Therefore the method should better be called execute(), run() or alike.

\$\endgroup\$
  • 48
    \$\begingroup\$ Is it me, or is it missing a FizzBuzzServiceProviderFactoryProviderFactory? \$\endgroup\$ – Willem van Rumpt Jan 4 '17 at 10:46
  • 5
    \$\begingroup\$ @TimothyTruckle: Nothing against objects. Mostly C# / C++ here. I just think it overkill to make a FizzBuzz class (and would frown at the solution if it were presented to me (had it been in my repertoire for interviewing candidates to begin with)). To each their own :) \$\endgroup\$ – Willem van Rumpt Jan 4 '17 at 12:03
  • 7
    \$\begingroup\$ Surely @SáT is being sarcastic. This answer is over-engineered. Just because Java is an OO language doesn't mean that a simple function like fizzBuzz() should be turned into a class. Or should we start doing sum = new AddOperation(5, 3).execute()? \$\endgroup\$ – Agop Jan 4 '17 at 16:44
  • 5
    \$\begingroup\$ @TimothyTruckle I'm not implying you should avoid objects in Java. All I'm saying is that for this particular program, there is absolutely no reason to use anything other than a static method. In fact, I wouldn't even advise using a separate method for this. \$\endgroup\$ – Agop Jan 4 '17 at 16:56
  • 6
    \$\begingroup\$ @TimothyTruckle: The object (no pun intended) of Java is not turning everything into a class. Just because you can, doesn't mean you should. Not creating a class out of every spec of dust does not imply you're using Java wrongly (or any object oriented language for that matter) , it's just an indication that you've given thought to what should be modeled and what not. \$\endgroup\$ – Willem van Rumpt Jan 4 '17 at 18:17
4
\$\begingroup\$

I have some feedbacK:

DRY (don't repeat yourself): you have repeated both i % 3 == 0 and i % 5 == 0 twice.

Single responsibility principle: your code is doing (at least) two things: 1) mapping rules to words, and 2) printing the words. If there was a requirement to print the words in some other way, you'd need to change about 50% of the entire code (I'm also thining Open/closed principle here); ideally you'd be able to implement a new type of 'printer' and inject in as a dependency.

I would expect to see some test code e.g. given 3 input then 'fizz' output, given 7 then '7' output, etc.

\$\endgroup\$
  • \$\begingroup\$ There is DRY code and there is code golf. In this case I think both solutions are fine. \$\endgroup\$ – Antzi Jan 6 '17 at 10:07
4
\$\begingroup\$

The solution is fine, but IMO FizzBuzz is a bad example to have a discussion of conventions and best practices about. The "problem" has only one purpose: to make an interview candidate demonstrate that they can actually write a logically correct piece of functioning code. Will it compile? Will it work? Will it fit on the sheet of paper we gave the candidate? That's all that matters for FizzBuzz. It demonstrates a most basic proficiency that is barely higher than HelloWorld. It's not the best direction to pursue for actually learning a language.

\$\endgroup\$
  • 1
    \$\begingroup\$ Sanity is good. \$\endgroup\$ – Tony Ennis Jan 6 '17 at 0:13
4
\$\begingroup\$

You might want to consider this:

 for(int i = 1; i <= 100; i++) {
   String output = "";
   if (i % 3 == 0) {
     output += "Fizz";
   }
   if (i % 5 == 0) {
     output += "Buzz";
   }
   if (output.length() == 0) {
     output =  Integer.toString(i);
   }
   System.out.println(output);
}

This

  • reduces the number of print calls which makes it easier to redirect later if required.
  • combines the "Fizz" and "Buzz" for "FizzBuzz" which makes it easier to add a check for future divisors (typical followup question on an interview)
\$\endgroup\$
  • \$\begingroup\$ I don't like output = "Fizz";. That's a bug waiting to happen if we want to add something for numbers divisible by 2. \$\endgroup\$ – jpmc26 Jan 6 '17 at 15:08
  • \$\begingroup\$ @jpmc26 - I agree. Fixed. \$\endgroup\$ – Erno Jan 6 '17 at 20:48
1
\$\begingroup\$

You ask if moving fizzbuzz outside of main is overkill, I would say yes and no. No, because main should not be doing complex logic, it should be short and simple. If there is complexity, that should be handled elsewhere. Yes, because you moved the WRONG THING out of main.

Fizzbuzz has three components, a loop to be executed some numbers of times, a calculation on a given value, and printing the result of said calculation. You move all three of these out of main into another function, but you don't actually gain anything by doing so.

Turn the calculation into a function that returns a result. At that point, you will probably want to have the loop stay in main, but either way, it will be a better program.

    public static String fizzBuzzTransformation(int numToTransform) {
           if (numToTransform % 3 == 0 && numToTransform % 5 == 0)
                return "FizzBuzz";

            if (numToTransform % 3 == 0)
                return "Fizz";

            if (numToTransform % 5 == 0)
                return "Buzz";

            return numToTransform.toString();
    }

Stylistic comments. You have a class, it has a public static main method and a public static method fizzBuzz. They do exactly the same thing. After moving the calculation out into it's own method, I would eliminate the fizzBuzz method, at that point it's just a loop and a println, well within what I would consider acceptable for main to be doing, and just as importantly, acceptable for another class to do if it wants to take advantage of your class. Main is as "pure" as it needs to be, it exercises the capability of the class in a simple straightforward manner.

\$\endgroup\$
  • \$\begingroup\$ The code is not indented properly. \$\endgroup\$ – Roland Illig Jul 3 '17 at 5:16

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.