8
\$\begingroup\$

I wanted to try FizzBuzz, but to do it in the most efficient way. I think this method employs most of the concepts I learned here. Is there anything unaccounted for? A sprinkle of Swing since that's what I've been focusing on lately.

import javax.swing.JOptionPane;

    public class FizzBuzzCounter {
        public static void main(String[] args) {
            int goalNumber = Integer.parseInt(JOptionPane.showInputDialog("FizzBuzz goal number?"));
            fizzBuzzify(goalNumber);
        }
        public static void fizzBuzzify(int n) {
            for (int i = 1; i <= n; i++) {
                StringBuilder sb = new StringBuilder();
                if (i % 3 == 0) sb.append("Fizz");
                if (i % 5 == 0) sb.append("Buzz");
                if (sb.toString().isEmpty()) System.out.println(i);
                else System.out.println(sb.toString());
            }
        }
    }
\$\endgroup\$
12
\$\begingroup\$

There are a bunch of style issues you should be aware of, a validation problem, an optimization or two, and also an alternate approach:

Style:

Use braces for 1-liners... these are 'no-no' statements:

            if (i % 3 == 0) sb.append("Fizz");
            if (i % 5 == 0) sb.append("Buzz");
            if (sb.toString().isEmpty()) System.out.println(i);
            else System.out.println(sb.toString());

and should be:

          if (i % 3 == 0) {
              sb.append("Fizz");
          }
          if (i % 5 == 0) {
              sb.append("Buzz");
          }
          if (sb.toString().isEmpty()) {
              System.out.println(i);
          } else {
              System.out.println(sb.toString());
          }

Validation

Your OptionPanel blindly assumes the input will be a valid integer. This is not true, and you should build a validation sequence for it.

You should also catch 'NumberFormatException' from the Integer.parseInt(...)

Performance & alternate

The StringBuilder may make sense, but, if you use it, there are two things:

  1. use sb.length() == 0 instead of sb.toString().isEmpty() which creates an unnecessary String value.
  2. create the sb instance outside the loop, and reuse it, and sb.setLength(0) inside the loop to clear it.

But, in reality, there is no need for the StringBuilder if you use plain String Constants, and also use else-statements, and also use %15 (which is %3 and %5).

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(i);
}
\$\endgroup\$
  • \$\begingroup\$ 15 is a magic number. It's the lowest common multiple of 3 and 5 and should be calculated instead of hardcoded. \$\endgroup\$ – RubberDuck Sep 5 '14 at 12:38
  • 3
    \$\begingroup\$ @RubberDuck If 3 and 5 are hardcoded, it's not really a concern (although 3 * 5 would be more self-documenting). Otherwise fizz (3) and buzz (5) parameters should be created, with fizzbuzz (15) calculated outside the loop. \$\endgroup\$ – nmclean Sep 5 '14 at 13:33
  • \$\begingroup\$ @nmclean, for maintainability I find it still be a concern, because it is not immediately obvious that 15 is a calculation, and it's also not obvious that 3 * 5 is a special case that works only for primes. Now, all that said, I'm standing on very hypothetical and academic ground. For all practical purposes however (as practical as FizzBuzz can be at least), you are probably correct. \$\endgroup\$ – RubberDuck Sep 5 '14 at 13:40
  • 1
    \$\begingroup\$ @RubberDuck What if the key numbers just happened to be 6 and 8? The LCM is not 6 * 8 (48), it's 24. Calculating the LCM tends to end up awkward unless you have a library function you can use. On the other hand, making it implicit with something like the StringBuilder version isn't too bad, or a comment like LCM(3, 5) on the 15 to explain where that number came from \$\endgroup\$ – Izkata Sep 5 '14 at 17:05
  • \$\begingroup\$ That's exactly my point @Izkata."it's also not obvious that 3 * 5 is a special case that works only for primes." \$\endgroup\$ – RubberDuck Sep 5 '14 at 17:18
7
\$\begingroup\$
import javax.swing.JOptionPane;

    public class FizzBuzzCounter {

Why is this way indented over here? I'm sure its a matter of copy/paste issues but the fact that you left it there wouldn't give me a good starting impression.

        public static void main(String[] args) {
            int goalNumber = Integer.parseInt(JOptionPane.showInputDialog("FizzBuzz goal number?"));

You've got bit of a strange mix here. Here you take input via a dialog, but you print to System.out below. That's a confused mix.

            fizzBuzzify(goalNumber);

What does it mean to fizzBuffiy something? It'd be better to call printFizzBuzz or something.

        }
        public static void fizzBuzzify(int n) {

The lack of a newline between functions sticks out to me. Of course, its not wrong, but pretty much every style has them.

            for (int i = 1; i <= n; i++) {
                StringBuilder sb = new StringBuilder();
                if (i % 3 == 0) sb.append("Fizz");
                if (i % 5 == 0) sb.append("Buzz");
                if (sb.toString().isEmpty()) System.out.println(i);
                else System.out.println(sb.toString());

This to me is the sign of a coder who has the mechanics of coding down, but doesn't have a good sense of readability. Its far harder to see what this code does in each case as compared to a more straightforward rendering such as that by @rolfl.

            }
        }
    }
\$\endgroup\$
6
\$\begingroup\$

A pet peeve of mine: Line length. A good style guide is to limit line length to 80 characters at most. This improves readability and makes code easier to understand.

This is even more important on stackexchange and github, where people also come to find good examples. Making them easier to read helps. (SE actually tends to break of code lines at different lengths, depending on the browser.)

This sample has a very long variable definition:

   public static void main(String[] args) {
        int goalNumber = Integer.parseInt(JOptionPane.showInputDialog("FizzBuzz goal number?"));
        fizzBuzzify(goalNumber);
    }

It can also be written as:

    public static void main(String[] args) {
        int goalNumber = Integer.parseInt(
            JOptionPane.showInputDialog("FizzBuzz goal number?")
        );
        fizzBuzzify(goalNumber);
    }

Doing a web search for 'Java style guide, line length' gives various resources: (Google actually highlighted the first one for me)

(and many more, several of which can also be found in SO Resources for java coding style

Each language tends to have its own tradition, and in my experience, most vary between 72 and 132, with the median probably around 80.

Besides Java, we have Python's PEP8, with a general 80 character recommendation, but also A foolish consistency is the hobgoblin of little minds. Both are good advice.

\$\endgroup\$
  • 4
    \$\begingroup\$ I don't want to start a flamewar, but I think the way you wrote it actually makes the code less readable. The 80-character rule is a convention from times when more characters wouldn't fit into the console and the programming languages weren't so verbose. Back then, having more than 80 characters on one line was making it hard to read because it could mean a lot of logic on one line. In this case, it's just setting a variable from console input. It's not a big deal. \$\endgroup\$ – JohnEye Sep 5 '14 at 14:42
  • \$\begingroup\$ As the next developer coming to read your code, I thank you greatly for making the lines shorter. Making them so long I have to scroll right significantly in my IDE will make me want to find you and lynch you. \$\endgroup\$ – James Sep 5 '14 at 16:41
  • \$\begingroup\$ Any more comments on this? Sort of confused if I should generally do the above or keep it my way. Was also wondering if, in addition to the suggestion, the last parentheses and semi colon on a separate line at bottom would be better? \$\endgroup\$ – Legato Sep 6 '14 at 2:42
  • \$\begingroup\$ @Legato - like this? Well spotted. I actually agree on this. \$\endgroup\$ – Henk Langeveld Sep 6 '14 at 11:18
  • \$\begingroup\$ BTW, @JohnEye - I started out on a Decwriter LA36. That was a paper terminal, with a column width of 132 characters. No fiddly 2-character soft-tabs then. ;-) Later, with the 80x24 character ttys, I would prefer to print my code on A4 paper in portrait - that's only readable with a narrow pitch and a 72 character line length. I still recommend that for code review: print it, step away from the screen, and bring a red pen (or pencil). \$\endgroup\$ – Henk Langeveld Sep 6 '14 at 11:26

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.