Return to Answer

...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:

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.

...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:

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.

I'm not impressed with the fact that the fizzbuzz logic is written in three different places, and if you were asked to add another "special case" and keep the same structure you would probably need to write it all over again in a fourth location - and then I go and swap all 3's with 5's and all 5's with 7's and you have way too many places to go make these changes, and the result would reek of copy-pasta code.

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 regex is doing there.

The code does read nicely and is well formatted, but the tests are clearly sub-par and fail to document the specifications - they document the output, not the specs, they aren't named in a standard way (i.e. "givenConditionThenSomething"), and they would be a royal pain to maintain if {3, 5} would suddenly need to be changed to anything else.

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.

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.