Interview coding test: Fizz Buzz

I was recently given a coding test to complete for a potential client. It was a FizzBuzz-type of thing with a two-hour time limit. I had a request to write basic FizzBuzz, then add a special case, then add a report. I didn't get an interview, and they didn't return any feedback so I don't know why. Can you take a look at this and let me know where I went wrong? It all looks good to me.

Edit
A lot of reviewers have questioned the duplication in my code. The coding-test called for me to write fizzbuzz, show the results, make a change, show the results, then make another change and show the results. This is why there are three methods -- to show the results of each part of the test. I usually wouldn't have duplication like this in my code. It's best to review each method in isolation, as if it's the only method in the code.

```
package com.engineerdollery;

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static java.util.stream.Collectors.*;

public class FizzBuzz {
private static final String NUMBER = "\\d+";

public String basic() {
return IntStream.rangeClosed(1, 20)
.parallel()
.mapToObj(i -> i % 15 == 0 ? "fizzbuzz"
: i % 3 == 0 ? "fizz"
: i % 5 == 0 ? "buzz"
: Integer.toString(i))
.collect(joining(" "));
}

public String lucky() {
return IntStream.rangeClosed(1, 20)
.parallel()
.mapToObj(i -> Integer.toString(i).contains("3") ? "lucky" // this is the only change from basic()
: i % 15 == 0 ? "fizzbuzz"
: i % 3 == 0 ? "fizz"
: i % 5 == 0 ? "buzz"
: Integer.toString(i))
.collect(joining(" "));
}

public String counter() {
List fizzBuzzList = IntStream.rangeClosed(1, 20)

Then add a special case

In all likelihood, they were looking to see how flexible and maintainable you could make your code be, and you gave them a showcase of some raw technical knowledge instead.

I'm not impressed with the fact that the fizzbuzz logic is written in three different places.

If you were tasked with adding another "special case" and keep the same structure because, time constraints, you would probably need to Copy+Paste it all over again in a fourth location... and then when the requirements change and you're tasked with swaping all 3's with 7's, you have way too many places to go make these changes, and the result would still reek of copy-pasta code.

At a minimum, you could have extracted constants for 3 and 5 magic values.

The mapping and parallelism look like an excuse to use Java 8 features, and are frankly overkill and uncalled for - you're iterating 20 values (isn't fizzbuzz supposed to be 1-100?), not 20 millions. Was there a requirement to use regular expressions somewhere? If not, I fail to understand what that a regex is doing in a fizzbuzz submission.

The code does read nicely and is well formatted in general; I would have written the ternaries this way though, to better reveal the logical structure:

public String lucky() {
    return IntStream.rangeClosed(1, 20)
            .parallel().mapToObj(i -> 
                Integer.toString(i).contains("3") 
                    ? "lucky" // this is the only change from basic()
                    : i % 15 == 0 
                        ? "fizzbuzz"
                        : i % 3 == 0 
                            ? "fizz"
                            : i % 5 == 0 
                                ? "buzz"
                                : Integer.toString(i))
            .collect(joining(" "));
}

...and then I'd look at this and think of how I could reduce the nesting. Perhaps it's just me, but I find this way of formatting nested ternaries...

return condition.boolExpression()
       ? true
       : condition.boolExpression()
           ? true
           : false;

...makes it easier to fully grasp what's going on at a glance, and easier to spot the places worthy of being extracted into their own function. For example:

public String lucky() {
    return IntStream.rangeClosed(1, 20)
            .parallel().mapToObj(i -> fizzbuzz(i))
            .collect(joining(" "));
}

I don't know if java does this, but in c# you can have a method group syntax; in Java it might look like this:

public String lucky() {
    return IntStream.rangeClosed(1, 20)
            .parallel().mapToObj(fizzbuzz)
            .collect(joining(" "));
}

Anyway the key concept here is abstraction. It's not about "superficial stylistic concerns" and subjective notions of readability, it's about extracting abstractions out of a problem. You could nest a bunch of IF functions in excel and achieve the same level of abstraction!


"But it's a one-liner!"

It's also a 4-level nested conditional structure that needs a new level for every new "special case" they could come up with. By moving the whole body of the loop into its own function, your code reads more expressively and feels much cleaner already.

Other than that, the tests are clearly sub-par and fail to document the specifications - they test the output, not the specs - and all they document is the name of the method they're testing. They aren't named in a standard way (i.e. "givenConditionThenSomething"), and they would be a royal pain to maintain if {3, 5} (or simply the number of iterations) would suddenly need to be changed to anything else.

StackExchange Code Review Q#126845, answer score: 60