Multiple FizzBuzz with input from file

Question

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;
    }
}

Answer 1

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.

Answer 2

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);
}

Answer 3

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.