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\$
4
  • 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\$ Commented May 4, 2012 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\$ Commented Aug 28, 2012 at 17:21
  • \$\begingroup\$ @cl-r thanks, I will keep it at the original so the comments will still make sense, thanks for the feedback \$\endgroup\$ Commented Sep 4, 2012 at 21:05
  • \$\begingroup\$ FizzBuzzEnterpriseEdition is the best implementation there is: github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition ;D \$\endgroup\$ Commented Aug 11, 2015 at 20:51

4 Answers 4

Reset to default
29
\$\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
  • 6
    \$\begingroup\$ 15 is a "magic number" here. I'd suggest using (3 * 5) instead. The compiler should convert it to 15... \$\endgroup\$ Commented Sep 4, 2012 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\$ Commented Sep 4, 2012 at 20:46
  • \$\begingroup\$ @X-Zero, would you capture the 3 as a constant, what would you call it? \$\endgroup\$ Commented Sep 5, 2012 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\$ Commented Sep 5, 2012 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\$ Commented Mar 27, 2018 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\$
7
  • 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\$ Commented Sep 5, 2012 at 8:22
  • 1
    \$\begingroup\$ Tell me, have you ever heard of the concept over-engineering...? \$\endgroup\$ Commented Dec 19, 2013 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\$ Commented Aug 12, 2015 at 16:43
  • \$\begingroup\$ See Rules for optimization. IMHO Winston Ewert provided the best answer. \$\endgroup\$ Commented Nov 16, 2015 at 13:50
  • \$\begingroup\$ explain math in withTable code please? \$\endgroup\$ Commented Mar 27, 2018 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\$
6
  • 2
    \$\begingroup\$ StringBuilder seems a faster approach than += in this case \$\endgroup\$ Commented Sep 4, 2012 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\$ Commented Sep 4, 2012 at 19:32
  • \$\begingroup\$ @asveikau - you are probably right, I guess this will be over optimization to find out exactly which is faster :) \$\endgroup\$ Commented Sep 4, 2012 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\$ Commented Sep 5, 2012 at 7:56
  • \$\begingroup\$ see above for another version using StringBuilder - but only one call to System.out.println(...). \$\endgroup\$ Commented Sep 6, 2012 at 15:10

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.