16
\$\begingroup\$

The common FizzBuzz implementation I saw is using a check for % 15 for printing "FizzBuzz"

Would you let me know if there is anything wrong / better with this approach?

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            boolean fizzOrBuzz = false;
            if (i % 3 == 0) {
                System.out.print("Fizz");
                fizzOrBuzz = true;
            }
            if (i % 5 == 0) {
                System.out.print("Buzz");
                fizzOrBuzz = true;
            }

            if (fizzOrBuzz) {
                System.out.println();
            } else {
                System.out.println(String.valueOf(i));
            }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Your version is optimized for the computer. Winston's version is optimized for the human. Got to trust a compiler that it can do a good job at re-shuffling and optimizing the code. Also, it is possible that a single io statement will work faster than two separate ones. You should race your version against the other one. \$\endgroup\$ – Leonid May 4 '12 at 23:01
  • \$\begingroup\$ I'll tell you why I like my version better (only for the interview context) 1) maintainability - you can externalize Fizz and Buzz as strings, you don't need to externalize FizzBuzz 2) DRY, a user writing the above code is thinking in terms of not repeating oneself, which I like \$\endgroup\$ – Eran Medan Aug 28 '12 at 17:21
  • 1
    \$\begingroup\$ +1 but, use a StringBuilder and not 100 * System.out.println(..), and, Integer.valueOf(i) \$\endgroup\$ – cl-r Sep 4 '12 at 15:35
  • \$\begingroup\$ @cl-r thanks, I will keep it at the original so the comments will still make sense, thanks for the feedback \$\endgroup\$ – Eran Medan Sep 4 '12 at 21:05
  • \$\begingroup\$ FizzBuzzEnterpriseEdition is the best implementation there is: github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition ;D \$\endgroup\$ – ZeroOne Aug 11 '15 at 20:51
28
\$\begingroup\$

Let's compare your version to the % 15 version:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            if (i % 15 == 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(String.valueOf(i));
            }
        }
    }
}

The % 15 version is simpler and easier to read. This version neatly delineates the problem into the 4 different cases, and handles each case. In contrast, your version introduces a boolean logic flag (which I consider to be a significant anti-pattern) and a not entirely intuitive dependence on the order of the if statements.

\$\endgroup\$
  • 5
    \$\begingroup\$ 15 is a "magic number" here. I'd suggest using (3 * 5) instead. The compiler should convert it to 15... \$\endgroup\$ – Ron Klein Sep 4 '12 at 19:29
  • 1
    \$\begingroup\$ @Ron - So is, "FizzBuzz", by this criteria. Not that you're wrong necessarily, but for these types of 'trivia' questions, the necessity of declaring everything is usually relaxed slightly. For instance, strictly speaking, for production-level code, all the numbers and strings referenced/printed here should be captured as named constants. \$\endgroup\$ – Clockwork-Muse Sep 4 '12 at 20:46
  • \$\begingroup\$ @X-Zero, would you capture the 3 as a constant, what would you call it? \$\endgroup\$ – Winston Ewert Sep 5 '12 at 17:06
  • \$\begingroup\$ @Winston - The general problem with this is it's basically a 'contrived' example, so it can be hard to get good names for this. Especially as what the test has is essentially 'multiple' checks - continuing to decompose this would likely result in some sort of array/table structure (yeah... I know... heading to outer-space architecture). So, MATCH maybe? CONDITION? Urgh. Which is why these kinds of conventions are relaxed for these kinds of tests; the test is for basic coding ability, not (necessarily) full abstraction skills. \$\endgroup\$ – Clockwork-Muse Sep 5 '12 at 17:33
  • 1
    \$\begingroup\$ I would move if-for-15 inside if-for-5, if you can't divide a number by 3, certainly can't divide by 15, save 4/5 of checks for all numbers \$\endgroup\$ – Kalpesh Soni Mar 27 '18 at 16:16
6
\$\begingroup\$

It looks fine. String.valueOf() is unnecessary, System.out.println(i) would print the same but it is still OK. This test is used only to make sure that the interviewee can write code as the linked site says:

This sort of question won’t identify great programmers, but it will identify the weak ones. And that’s definitely a step in the right direction.

\$\endgroup\$
3
\$\begingroup\$

They are certainly not perfects, but I've tried some ways to optimize the test, here the result (I have had numbers to keep trace of good responses) , and I use a StringBuilder to avoid initialization of output IO:

package exercices;

import java.util.Hashtable;

import org.memneuroo.outils.communs.utilitaires.EnvPrm;

public class FizzBuzz {
    // time for cum with nbIter=30 > 300; 30 ~= 3000
    static final int nbIter = 30;
    static final String sep = "_";

    static long ifNested() {
        final StringBuilder sb = new StringBuilder();
        final long t = System.nanoTime();
        for (int i = 0; i < nbIter; i++) {
            sb.append(//
            i % 15 == 0 //
                ? "FizzBuzz" //
                : (i % 3 == 0 //
                    ? "Fizz"//
                    : (i % 5 == 0//
                        ? "Buzz" //
                        : i)));// sb.append(sep);
        }
        final long totT = System.nanoTime() - t;
        System.out.format("ifNested\t%20d\n", totT);
        // sb.append(EnvPrm.NEWLINE); System.out.println(sb.toString());
        return totT;
    }

    static long stringPlus() {
        final StringBuilder sb = new StringBuilder();
        final long t = System.nanoTime();
        for (int i = 0; i < nbIter; i++) {
            String x = "";
            x += (i % 3 == 0) ? "Fizz" : "";
            x += (i % 5 == 0) ? "Buzz" : "";
            if (x.isEmpty()) { // MODIF
                x += Integer.toString(i);
            }
            sb.append(x);// sb.append(sep);
        }
        final long totT = System.nanoTime() - t;
        System.out.format("stringPlus\t%20d\n", totT);
        // sb.append(EnvPrm.NEWLINE); System.out.println(sb.toString());
        return totT;
    }

    static long withIf() {
        final StringBuilder sb = new StringBuilder();
        final long t = System.nanoTime();
        for (int i = 0; i < nbIter; i++) {
            if (i % 3 == 0) {
                sb.append("Fizz");
                if (i % 5 == 0) {
                    sb.append("Buzz");
                }
            } else if (i % 5 == 0) {
                sb.append("Buzz");
            } else {
                sb.append(i);
            }// sb.append(sep);
        }
        final long totT = System.nanoTime() - t;
        System.out.format("withIf\t\t%20d\n", totT);
        // sb.append(EnvPrm.NEWLINE);System.out.println(sb.toString());
        return totT;
    }

    static long withArray() {
        final String[] lis = {"FizzBuzz", "", "", "Fizz", "", "Buzz", "Fizz",
            "", "", "Fizz", "Buzz", "", "Fizz", "", "",};
        final StringBuilder sb = new StringBuilder();
        final long t = System.nanoTime();
        for (int i = 0; i < nbIter; i++) {
            final String pos = lis[i % 15];
            sb.append(((0 == pos.length()) ? i : pos));// sb.append(sep);
        }
        final long totT = System.nanoTime() - t;
        System.out.format("withArray\t%20d\n", totT);
        // sb.append(EnvPrm.NEWLINE); System.out.println(sb.toString());
        return totT;
    }

    static long withTable() {
        final Hashtable<Integer, String> ht = new Hashtable<>(8);
        ht.put(0, "FizzBuzz");
        ht.put(3, "Fizz");
        ht.put(5, "Buzz");
        ht.put(6, "Fizz");
        ht.put(9, "Fizz");
        ht.put(10, "Buzz");
        ht.put(12, "Buzz");
        final StringBuilder sb = new StringBuilder();
        final long t = System.nanoTime();
        for (int i = 0; i < nbIter; i++) {
            final String s = ht.get(i % 15);
            // MODIF
            // http://www.developpez.net/forums/d1196563-2/java/general-java/if-null-object-if-objet-null/#post6561766
            // sb.append((null == s ? i : s));// sb.append(sep);
            if (null == s) {
                sb.append(i);
            } else {
                sb.append(s);
            }
        }
        final long totT = System.nanoTime() - t;
        System.out.format("withTable\t%20d\n", totT);
        // sb.append(EnvPrm.NEWLINE); System.out.println(sb.toString());
        return totT;

    }

    static int recursive(final StringBuilder sb, final int n) {
        if (0 == n) {
            return 1;
        }
        if (n % 3 == 0) {
            sb.insert(0, "Fizz");
            if (n % 5 == 0) {
                sb.insert(0, "Buzz");
            }
        } else if (n % 5 == 0) {
            sb.insert(0, "Buzz");
        } else {
            sb.insert(0, n);
        }
        return n + recursive(sb, n - 1);
    }

    static long recursive() {
        final StringBuilder sb = new StringBuilder("");
        final long t = System.nanoTime();
        recursive(sb, nbIter);
        final long totT = System.nanoTime() - t;
        System.out.format("recursive\t%20d\n", totT);
        sb.append(EnvPrm.NEWLINE);
        System.out.println(sb.toString());
        return totT;
    }

    /*** @param args */
    public static void main(final String[] args) {
        long cum = 0L, cum2 = 0L;
        for (int i = 0; i < 5; i++) {
            System.out.println("------ " + i + " -----");
            final long totSb = stringPlus();
            final long totIn = ifNested();
            final long totWi = withIf();
            final long totWa = withArray();
            final long totWt = withTable();
            final long totRe = recursive();
            System.out.format("... stringPlus/withIf :%5d\n", (totSb * 100)
                / totWi);
            System.out.format("...   ifNested/withIf :%5d\n", (totIn * 100)
                / totWi);
            System.out.format("...  withArray/withIf :%5d\n", (totWa * 100)
                / totWi);
            System.out.format("...  withTable/withIf :%5d\n", (totWt * 100)
                / totWi);
            System.out.format("...  recursive/withIf :%5d\n", (totRe * 100)
                / totWi);
            cum += totIn + totSb + totWi + totWa + totWt + totRe;
            System.out.println("CUMUL (SECOND) == " + cum / 100000000 + "."
                + cum % 100000000 + "\t , diff: " + (cum - cum2));
            cum2 = cum;
        }
    }
}

And the output :

------ 0 -----
stringPlus                529397
ifNested                  643657
withIf                     27657
withArray                  43581
withTable                  40788
recursive                  87441
12Fizz4BuzzFizz78FizzBuzz11Fizz1314BuzzFizz1617Fizz19BuzzFizz2223FizzBuzz26Fizz2829BuzzFizz

... stringPlus/withIf : 1914
...   ifNested/withIf : 2327
...  withArray/withIf :  157
...  withTable/withIf :  147
...  recursive/withIf :  316
CUMUL (SECOND) == 0.1372521  , diff: 1372521
------ 1 -----
stringPlus                345295
ifNested                   88280
withIf                     88279
withArray                  88838
withTable                 101689
recursive                  93308
12Fizz4BuzzFizz78FizzBuzz11Fizz1314BuzzFizz1617Fizz19BuzzFizz2223FizzBuzz26Fizz2829BuzzFizz

... stringPlus/withIf :  391
...   ifNested/withIf :  100
...  withArray/withIf :  100
...  withTable/withIf :  115
...  recursive/withIf :  105
CUMUL (SECOND) == 0.2178210  , diff: 805689
------ 2 -----
stringPlus                380216
ifNested                   36597
withIf                     20953
withArray                  60063
withTable                  91352
recursive                 111467
12Fizz4BuzzFizz78FizzBuzz11Fizz1314BuzzFizz1617Fizz19BuzzFizz2223FizzBuzz26Fizz2829BuzzFizz

... stringPlus/withIf : 1814
...   ifNested/withIf :  174
...  withArray/withIf :  286
...  withTable/withIf :  435
...  recursive/withIf :  531
CUMUL (SECOND) == 0.2878858  , diff: 700648
------ 3 -----
stringPlus                489168
ifNested                   29613
withIf                     22070
withArray                  27099
withTable                  27378
recursive                  91911
12Fizz4BuzzFizz78FizzBuzz11Fizz1314BuzzFizz1617Fizz19BuzzFizz2223FizzBuzz26Fizz2829BuzzFizz

... stringPlus/withIf : 2216
...   ifNested/withIf :  134
...  withArray/withIf :  122
...  withTable/withIf :  124
...  recursive/withIf :  416
CUMUL (SECOND) == 0.3566097  , diff: 687239
------ 4 -----
stringPlus                143035
ifNested                   24025
withIf                     15924
withArray                  23187
withTable                  26819
recursive                  87162
12Fizz4BuzzFizz78FizzBuzz11Fizz1314BuzzFizz1617Fizz19BuzzFizz2223FizzBuzz26Fizz2829BuzzFizz

... stringPlus/withIf :  898
...   ifNested/withIf :  150
...  withArray/withIf :  145
...  withTable/withIf :  168
...  recursive/withIf :  547
CUMUL (SECOND) == 0.3886249  , diff: 320152
\$\endgroup\$
  • 1
    \$\begingroup\$ StringBuilder is based on a char[16]. You should set the initial capacity to a value larger than the String you want to create - or you worsen the performance, as StringBuilder has to grow (allocate new char[], copy old data). You should also be aware of how [System.nanoTime()](stas-blogspot.blogspot.com/2012/02/what-is-behind-systemnanotime.html) works and the issues it has. And I'd advise you to do a lot more iterations. I think you mainly measure the time for System.out.println() right now, the other operations probably take very little time compared to a system call. \$\endgroup\$ – Arne Sep 5 '12 at 8:22
  • 1
    \$\begingroup\$ Tell me, have you ever heard of the concept over-engineering...? \$\endgroup\$ – Max Dec 19 '13 at 18:44
  • 2
    \$\begingroup\$ @Max I do, but is this site is done for some researches or not? Is for comfortable eyes, or to try to understand how some ways are better ? So you can choose and compare the faster, the easy to read, or interrogate the code to find missing optimization. \$\endgroup\$ – cl-r Aug 12 '15 at 16:43
  • \$\begingroup\$ See Rules for optimization. IMHO Winston Ewert provided the best answer. \$\endgroup\$ – hfontanez Nov 16 '15 at 13:50
  • \$\begingroup\$ explain math in withTable code please? \$\endgroup\$ – Kalpesh Soni Mar 27 '18 at 16:21
3
\$\begingroup\$

Here's a version that's (IMHO) a little better for humans than yours and better for computers as well:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            String value;
            switch (i % 15) {
            case  3:
            case  6:
            case  9:
            case 12:  // divisible by 3, print Fizz
                value = "Fizz";
                break;
            case  5:
            case 10:  // divisible by 5, print Buzz
                value = "Buzz";
                break;
            case  0:  // divisible by 3 and by 5, print FizzBuzz
                value = "FizzBuzz";
                break;
            default:  // else print the number
                value = Integer.toString(i);
            }
            System.out.println(value);
        }
    }
}

The comments provide information to humans (but they could still see it on their own) and there's only one System.out.println call per i.

EDIT: This is another way to fizz-buzz (focus: DRY):

public class FizzBuzz {
    public static void main(String[] args) {
        final String EMPTY = "";
        for (int i = 1; i <= 100; i++) {
            String value = EMPTY;
            if (i % 3 == 0) value += "Fizz";
            if (i % 5 == 0) value += "Buzz";
            if (value == EMPTY) value += i;
            System.out.println(value);
        }
    }
}

EDIT 2: yet another, using StringBuilder, DRY as well:

public class FizzBuzz {
    public static void main(String[] args) {
        StringBuilder builder = new StringBuilder(1000);
        for (int i = 1; i <= 100; i++) {
            final int length = builder.length();
            if (i % 3 == 0) builder.append("Fizz");
            if (i % 5 == 0) builder.append("Buzz");
            if (length == builder.length()) builder.append(i);
            builder.append('\n');
        }
        System.out.println(builder);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ StringBuilder seems a faster approach than += in this case \$\endgroup\$ – Eran Medan Sep 4 '12 at 15:40
  • \$\begingroup\$ @EranMedan - Are you sure of this? StringBuilder is good because N appends doesn't lead to N allocations, however here N is at most 2, which is a small enough number that it likely won't make a difference, and it's quite likely to be 1, which means no difference at all (or maybe worse, as StringBuilder probably does an extra allocation for that 1 append). \$\endgroup\$ – asveikau Sep 4 '12 at 19:32
  • \$\begingroup\$ @asveikau - you are probably right, I guess this will be over optimization to find out exactly which is faster :) \$\endgroup\$ – Eran Medan Sep 4 '12 at 21:04
  • 1
    \$\begingroup\$ I'm aware of StringBuilder - which I use all the time in String heavy code - and thought along the lines of asveikau. += is less "noisy" (also the reason for my avoidance of Integer.toString()) and it won't make much of a difference as there's at most two concatenations. Btw.: I'd have used StringBuilder like this: declare before the loop, initialize with capacity set to 8, cleared and reuse per iteration. Creating it inside the loop wouldn't improve much. \$\endgroup\$ – Arne Sep 5 '12 at 7:56
  • \$\begingroup\$ see above for another version using StringBuilder - but only one call to System.out.println(...). \$\endgroup\$ – Arne Sep 6 '12 at 15:10

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