Problem
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<String> fizzBuzzList = IntStream.rangeClosed(1, 20)
.parallel()
.mapToObj(i -> Integer.toString(i).contains("3") ? "lucky"
: i % 15 == 0 ? "fizzbuzz"
: i % 3 == 0 ? "fizz"
: i % 5 == 0 ? "buzz"
: Integer.toString(i))
.collect(Collectors.toList());
Map<String, Long> countMap = fizzBuzzList
.parallelStream()
.collect(groupingBy(s -> s.matches(NUMBER) ? "integer" : s, counting()));
// reports
String fizzbuzz = fizzBuzzList.parallelStream().collect(joining(" "));
String counts = countMap.entrySet().parallelStream()
.map(e -> e.getKey() + ": " + e.getValue())
.collect(joining("n"));
return fizzbuzz + "n" + counts;
}
}
Test
package com.engineerdollery;
import org.junit.Test;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
public class FizzBuzzTest {
public static void main(String[] args) {
System.out.println(new FizzBuzz().counter());
}
@Test
public void basic() {
FizzBuzz fizzBuzz = new FizzBuzz();
String actual = fizzBuzz.basic();
assertEquals("1 2 fizz 4 buzz fizz 7 8 fizz buzz 11 fizz 13 14 fizzbuzz 16 17 fizz 19 buzz", actual);
}
@Test
public void lucky() {
FizzBuzz fizzBuzz = new FizzBuzz();
String actual = fizzBuzz.lucky();
assertEquals("1 2 lucky 4 buzz fizz 7 8 fizz buzz 11 fizz lucky 14 fizzbuzz 16 17 fizz 19 buzz", actual);
}
@Test
public void counter() {
FizzBuzz fizzBuzz = new FizzBuzz();
String actual = fizzBuzz.counter();
assertTrue(actual.contains("1 2 lucky 4 buzz fizz 7 8 fizz buzz 11 fizz lucky 14 fizzbuzz 16 17 fizz 19 buzz"));
assertTrue(actual.contains("fizz: 4"));
assertTrue(actual.contains("buzz: 3"));
assertTrue(actual.contains("fizzbuzz: 1"));
assertTrue(actual.contains("lucky: 2"));
assertTrue(actual.contains("integer: 10"));
}
}
Solution
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.
Java 8 has good support accross all IDEs but there are still a few differences. For example your code, as-is, doesn’t compile with the latest Eclipse version (Mars.2). On the command line with JDK 1.8.0_51, it compiles fine. I would hope they didn’t reach out to you because they tested it with Eclipse.
(Since I’m using Eclipse, I tweaked it a little to make it compile, it comes down to adding explicitely the type <String> in the Stream pipelines when calling mapToObj).
Hard-coded values
You are fizzbuzzing from 1 to 20 only. Those numbers are hard-coded. It would be preferable to make the methods take a maximum value as parameter.
public String basic(int max) {
return IntStream.rangeClosed(1, max) //...
}
It would make them more generic. Also, you are hard-coding the fizzbuzz words; consider making them a constant. You should also try to extract the mapping methods like:
i -> i % 15 == 0 ? "fizzbuzz"
: i % 3 == 0 ? "fizz"
: i % 5 == 0 ? "buzz"
: Integer.toString(i)
in a dedicated method.
Don’t (ab)use parallel pipelines
The Stream API was built with parallel capabilities in mind. However, it doesn’t mean that every pipeline should be made parallel. For example, you currently have:
IntStream.rangeClosed(1, 20).parallel() //...
for all of your Stream pipelines.
This is likely to have no gain at all on such a low amount of values; in fact, it may hinder performance. Before going down the parallel route, you should always measure the difference to determine if it’s worth it or not. Micro-benchmarking is not an easy task; if you want to do that, I would suggest you to use the JMH framework that facilitates the creation of benchmarks in Java.
Use system line separator
In the counter method, you are joining your Strings with "n". It would be better to use the system line separator with System.lineSeparator().
Wildcard import statements
You are using
import static java.util.stream.Collectors.*;
to import all the collectors. This is generally not a good practice. You want to import the specific classes with which you are interacting. In this case, you want
import static java.util.stream.Collectors.counting;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.joining;
I apologize if the tone of this is a bit harsh, but hopefully I can provide some valuable advice.
For me, Fizzbuzz is about three things:
- Is there a loop?
- Is there a function?
- Does the code run?
You’ve got 2.5 of 3. There’s no function/method that takes an integer and returns something printable for that input; that’s the heart of fizzbuzz. I understand you buried your transform in a lambda and mapped over it, but it’s only accessible through a parameterless interface which can’t be independently accessed or tested. Then you copy/pasted it into report instead of factoring it out–a big red flag.
Speaking of tests, you wrote some but they are incredibly brittle. If I wanted you to iterate from 1 to 21, all your tests would break. This should have been a clue that the interface to the core logic should have been refactored, but instead you happily wrote a bunch of brittle conditions which don’t provide a whole lot of confidence that the code is working well.
You showed that you were aware of fancy types like IntStream, but when doing something fundamental like counting the number of fizzs, you chose to test against a string actual.contains("fizz: 4").
countMap is awesome. Do more of that. In fact, countMap is the report. Move NUMBER next to it, and make it your return value. Do the printing elsewhere.
Parallelism was an incredibly distracting flourish; completely unnecessary for the task and a bit worrying. If I were a potential employer, I’d wonder what other unnecessary junk you were going to insert into my codebase.
I don’t care about extracting magic numbers or whatever; that’s easy to do later, and arguably they shouldn’t be moved anyway because they are uniquely tied to that particular transformation logic. It’s also dirt simple to change import statements, so I wouldn’t hold that against you too much either.
I think you should strive for KISS. You wrote 96 lines where I think far fewer would have sufficed, and failed to write a good string fizzbuzz(int). You signaled that you are more interested in writing lots of lines with fancy features than writing clean, minimalist code.
Final thought: You could write the best code in the world and a business might not provide any feedback or contact you again–for any number or reasons. Don’t sweat it too much.
Your solution probably looks too different from how they would have coded it themselves, and so it looks like complete overkill for what they’re asking for.
Basically you’ve just told them that you will take a simple problem and find a complicated, hard-to-read, hard-to-debug, and hard-to-maintain solution to it. If your reaction is “but it’s not that complicated, I’m just mapping stuff and a regex”, then that is the problem: you don’t need to map for this problem.
For this task, they just wanted to see if you can write a for loop and either use the modulus operator or keep track of 2 counters and reset them.
Maybe some companies or some interviewers would have been impressed by your use of more advanced features of the language. Apparently not this one.
Other thoughts:
The tests are a nice addition, and would have scored points if I was doing the interviewing.
For the special lucky case, it’s not clear if they wanted you to still print fizz as well. Maybe they wanted to see if you’d ask for clarification.
Others have mentioned that your code is overkill, but the biggest problem I see is the parallelism. It is frankly broken and will not always give the expected result. To an observer, it looks like you used a feature that you did not really understand in order to look good, and as a result you wrote incorrect code. Your parallelism will not always produce the correct result and it is expected that you would understand that if you use that feature.
There are other minor stylistic things that others have covered, but that is by far the worst problem.
Edit, per comments:
Looks like I am in fact wrong. The problem, however, is that it took quite a bit of time digging through nitty gritty details of the Java 8 implementation to figure this out. The following give better details than the link @Tunaki provided: https://stackoverflow.com/questions/29710999/is-collect-guaranteed-to-be-ordered-on-parallel-streams?lq=1 So what should I do with this answer. It perhaps should be deleted, but makes sense in some respects because this is likely what the reviewer thought. – Necreaux