\$\begingroup\$

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.