6
\$\begingroup\$

I made a fizzbuzz program in Java 7 I'd like reviewed.

Given an input file with three numbers in each line, fizz, buzz, and count:

3 5 10
2 7 15

The output, for each line, should be the numbers from 1 to count, except that multiples of fizz are replaced with "F", multiples of buzz are replaced by "B", and multiples of both with "FB":

1 2 F 4 B F 7 8 F B
1 F 3 F 5 F B F 9 F 11 F 13 FB 15

The code, in Java 7:

package com.codeeval.fizzbuzz;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

public class Main {
    public static void main(String[] args) throws IOException {
        String path = args[0];
        List<String> lines = getFileLines(path);

        StringBuilder sb = new StringBuilder();
        for (String line : lines){
            int[] arguments = getFizzBuzzArguments(line);
            int x = arguments[0];
            int y = arguments[1];
            int n = arguments[2];

            String result = fizzBuzz(x, y, n);
            sb.append(result).append('\n');
        }

        System.out.println(sb);
    }

    public static String fizzBuzz(int x, int y, int n) {
        StringBuilder sb = new StringBuilder();
        for (int i = 1; i <= n; i++) {
            boolean isX = (i % x == 0);
            boolean isY = (i % y == 0);
            String append;

            if (isX || isY) {
                if (isX && isY) { append = "FB"; }
                else if (isX) { append = "F"; }
                else { append = "B"; }
            } else { append = Integer.toString(i); }

            sb.append(append);
            if (i != n){ sb.append(' '); }
        }
        return sb.toString();
    }

    private static List<String> getFileLines(String path) throws IOException {
        File file = new File(path);
        BufferedReader buffer = new BufferedReader(new FileReader(file));
        List<String> lines = new ArrayList<>();
        String line;
        while ((line = buffer.readLine()) != null) {
            line = line.trim();
            lines.add(line);
        }

        return lines;
    }

    private static int[] getFizzBuzzArguments(String line){
        String[] arguments = line.split(" ");
        return getArrayOfInts(arguments);
    }

    private static int[] getArrayOfInts(String[] strings){
        int[] ints = new int[strings.length];

        for (int i = 0; i < strings.length; i++){
            ints[i] = Integer.parseInt(strings[i]);
        }

        return ints;
    }
}
\$\endgroup\$
2
\$\begingroup\$

Your code is well structured and quite easy to read and understand: this is a very good point about it.

However, there are some things that may be improved, but very few of them seem to be really critical.

main(arg)

Variables int x, int y and int n are treated separately, but indeed they are semantically related, because they describe a "FizzBuzz" to produce. A dedicated class may be created, for example:

public class FizzBuzz {

  private final int x;
  private final int y;
  private final int n;

  public FizzBuzz(int x, int y, int n) {
    this.x = x;
    this.y = y;
    this.n = n;
  }
  // getters, if needed
}

Moreover, your fizzBuzz method may then go directly into this class, named something like processInput(). The loop in main() would look like this:

for (String line : lines) {
  int[] arguments = getFizzBuzzArguments(line);
  FizzBuzz fb = new FizzBuzz(arguments[0], arguments[1], arguments[2]);
  sb.append(fb.processInput()).append('\n');
}

FizzBuzz may also have a constructor taking directly the array of input data as argument.

fizzBuzz(args)

Three points concerning the modulo divisions and the booleans isX and isY.

First, the divisors (x or y) must be validated to be different from 0, if you want to avoid ArithmeticException .

Second, they should be extracted into a dedicated method, which is easier to read:

private static boolean isMultipleOf(int dividend, int divisor) {
  return dividend % divisor == 0;
}

Third, the booleans are named poorly. They'd better sound as isFizz and isBuzz: these are their meanings.

Finally, the conditionals can be simplified:

if (isFizz || isBuzz) {
  if (isFizz) {
    sb.append("F");
  }
  if (isBuzz) {
    sb.append("B");
  }
} else {
  sb.append(i);
}

getFileLines()

Since you underline that the code in Java 7, then its facilities should be used.

  • Replace File with java.nio.file.Path.

  • Use a try-with-resources block in order to close automatically the streams opened with BufferedReader and FileReader.

\$\endgroup\$
  • 1
    \$\begingroup\$ I find that a FizzBuzz class would be overkill here. It would only provide one method, and storage for the parameters to that method. That is better of as a static method instead. And a isMultipleOf method is also overkill, a % b == 0 is very common to use and totally readable. \$\endgroup\$ – Simon Forsberg Nov 14 '15 at 23:27
  • \$\begingroup\$ @Simon, I agree with your remarks, these items are an overkill in this context, because the entire task is small enough. I suggested to introduce the class because it clearly separates the core logic of the program (fizzBuzz method) from other pieces, which are mostly input data reading and preparation of arguments. And the extraction of isMultipleOf is done in order to avoid the almost duplicated (i % x == 0) calls. Of course, they are very common to use. \$\endgroup\$ – Antot Nov 15 '15 at 7:39
  • \$\begingroup\$ If you would extract isMultipleOf, then you would have multiple isMultipleOf calls. (i % x == 0) really isn't any more code duplication than calling isMultipleOf multiple times. \$\endgroup\$ – Simon Forsberg Nov 15 '15 at 10:53
2
\$\begingroup\$

Handling errors

You're not handling these possible errors:

  • A filename argument is not given
  • The specified file does not exist.

You can handle them by checking the number of arguments:

if(args.length != 1) {
    System.err.println("Specify a filename as an argument");
    return;
}

and catching FileNotFoundException: (import it from java.io)

    try {
        String path = args[0];
        /* ... */
        System.out.println(sb);
    } catch(FileNotFoundException e) {
        System.err.println("The specified file was not found");
    }

You should also handle the case when a field in the file isn't a number (by catching NumberFormatException) or the number of fields isn't 3 (check it in getFizzBuzzArguments and throw in that case).

Naming

I would rename x, y and n to fizz, buzz and count (and rename isX and isY to isFizz and isBuzz).

Simplify conditionals

You can simplify the conditionals as @Antot suggested, and further simplify to:

if(isFizz) {
    sb.append("F");
}
if(isBuzz) {
    sb.append("B");
}
if(!isFizz && !isBuzz) {
    sb.append(i);
}
\$\endgroup\$
0
\$\begingroup\$

You don't need to concatenate the lines in main with a StringBuilder. Instead, inside the loop you can print each line right after you get the result from fizzBuzz:

System.out.println(result);

The output will be the same.

BTW if you are adding Strings without if/for conditions like a + b then it is optimized to StringBuilder by the compiler. So you don't need to use StringBuilder for simple cases of String concatenation.

It is over-optimization in this case.

Rest looks good.

\$\endgroup\$
  • 3
    \$\begingroup\$ Uh, no, that's wrong. It's recommended to use StringBuilder any time you're performing repeated, non-hard-coded appends. \$\endgroup\$ – Fund Monica's Lawsuit Nov 14 '15 at 17:18
  • \$\begingroup\$ @QPaysTaxes I have decompiled class files on Oracle's JDK 6 and JDK 7 to check that it actually happens. This is not a wild guess. Moreover when I search all new content also suggests that this happens. stackoverflow.com/questions/18453458/… stackoverflow.com/questions/1532461/…. One of these refer to the Java specification in which it is stated that this optimization can be done by compiler. What are your references for the recommendation? \$\endgroup\$ – Aseem Bansal Nov 14 '15 at 20:08
  • \$\begingroup\$ While this answer is not completely wrong, I find it a bit confusing and have chosen to downvote it primarily for that reason. \$\endgroup\$ – Simon Forsberg Nov 14 '15 at 23:23
  • \$\begingroup\$ Hm, I can't find the line in the spec that says that it puts the StringBuilder in the narrowest scope possible, but I'm pretty sure it's there. \$\endgroup\$ – Fund Monica's Lawsuit Nov 15 '15 at 0:46
  • 1
    \$\begingroup\$ @QPaysTaxes Just for my future reference what would putting StringBuilder in the narrowest scope imply? \$\endgroup\$ – Aseem Bansal Nov 15 '15 at 5:21

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.