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

• (that comment was meant to be under my answer, right?) -- IMO it's not about implementing patterns (although those are a valuable tool for this), it's about identifying the points of change, the implicit assumptions, the things that the business can decide to change on a whim... and then writing code that only needs to change minimally when your assumptions are shattered and the specs do change: that's what my point about flexibility/maintainability was. Keep in mind that FizzBuzz is an exercise, a metaphor for real-world code. – Mathieu Guindon Apr 27 '16 at 15:35
• A small note that the answers haven't covered. In regards to formatting, some lines are just awfully long. While not as important as the issues in the answers, it is still a factor to writing readable code. – Dzhao Apr 28 '16 at 21:21
• Good programmers should also write their code to be understandable and with comments. Using Java 8 features makes it less readable than simply using a standard For loop, which has been around longer. The FizzBuzz task is pretty simple, of course, but comments couldn't hurt. – mbomb007 Apr 28 '16 at 21:59
• @EngineerDollery Comments don't always mean code needs to be refactored. Stating that as an absolute is ridiculous. – AaronLS Apr 29 '16 at 1:59
• blog.codinghorror.com/code-tells-you-how-comments-tell-you-why – mbomb007 May 1 '16 at 20:12

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 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 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 does this, but in 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 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.

• The three implementation methods were only done to show code evolution. I'd made a note to this effect in the submission and that I'd normally only have one method for this that evolved along with the spec. I wanted them to see the separate steps. – Software Engineer Apr 27 '16 at 15:04
• The regex is for the counter() function. – Software Engineer Apr 27 '16 at 15:08
• Point taken about the tests though -- that's spot on. I wrote them specifically based on the technical spec -- literally copying the examples. I shouldn't have been that lazy, but there was a time constraint. – Software Engineer Apr 27 '16 at 15:11
• "I'd made a note to this effect in the submission"... as an interviewer I'd not be enthusiastic about this at all. I want to see that you can apply to FizzBuzz (which is pretty trivial) the level of discipline I'd expect and require for a bulletproof solution to a really complicated problem. The message I get is "this (copypasta, overcomplex) is the way I code and I've done it knowing it's not really what you want". – Julia Hayward Apr 28 '16 at 8:41
• Thanks Julia -- I was weary about this approach in the first place. You've confirmed that it was a bad idea and I should have stuck to the script. I appreciate the insight. – Software Engineer Apr 29 '16 at 0:44

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;

• Ty, this is great. Parallel: I wasn't interested in performance with this, as there were no directions to make it particularly efficient, I was more interested in documenting the fact that this is parallelisable than improving performance. The import.* was a terrible mistake -- I had just installed a new copy of IntelliJ and it does this by default sometimes and I hadn't noticed -- I hate that construct and would not have used it if I'd been paying attention. The system line separator is a great catch -- this would break anywhere but on my own system. – Software Engineer Apr 27 '16 at 15:13
• What's the point of extracting the numbers and words as constants? Sometimes we go too far in applying best practices like avoiding magic numbers. It's often fine to hardcode constants that are part of the business logic. Extracting them just moves them away from the business logic code. It's a form of premature generalization. – John Kugelman Apr 27 '16 at 21:56
• @JohnKugelman it's tricky to say in the fizzbuzz case since it's so contrived, but I'd argue that 3 and 5 really DON'T mean just 3 and 5 here. they are the current numeric representation of "fizz" and "buzz", whatever that means. I agree it'd be silly to declare final int FIVE = 5; Doing final int FIZZ = 3; is pretty reasonable and desirable though imo. Maybe I wouldn't put it as a field class in the class, maybe a hashmap would be better, or some external resource, or just a local variable, I dunno, but there is value in labeling a number that carries meaning beyond it's numerical value. – sara Apr 28 '16 at 7:26
• But what does it buy you to extract FIZZ = 3? How does it help? It adds a layer of indirection, which in a vacuum is a negative, so there should be some benefit that outweighs the negative. I'm not seeing it. There's no need to have the code "explain" what 3 is. The code itself explains it: i % 3 == 0 ? "fizz" tells you exactly what 3 means. We don't always need constants. Code has meaning, too. I suppose I'm harping on this quite a bit, but it's a pet peeve of mine: becoming obsessive about applying best practices without thinking through the actual reasons for doing so. – John Kugelman Apr 28 '16 at 12:24
• @JohnKugelman I didn't say in my answer to factor out the 5. The hard-coded part was about the max range of 20 in IntStream.rangeClosed(1, 20) – Tunaki Apr 28 '16 at 15:34

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.

• Don't apologise. I've come here asking for a review -- I expect to be criticised :) – Software Engineer Apr 28 '16 at 0:51

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.

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

• I don't think that parallelism breaks the order of the output. parallel() simply allows the mapping function to be applied on each i in individual threads, so they needn't wait for each other when executed sequentially. Once mapping is done, all results are collected in input order by the Collector. More details here. Had there been a sysout in the mapping function itself, then the order would be broken. – Vineet Apr 28 '16 at 4:57
• I have to agree with @Vineet. This answer doesn't seem right. His use of parallelism in the theoretical sense is completely fine. The parallel map can work in parallel. The collect then preserves the order. The major issue with parallel is that it is overkill, and although I have not benchmarked it, I would suspect is slower than the serial version given the machinery needed to use them in the first place. – Dair Apr 28 '16 at 7:19
• @EngineerDollery No, this answer is completely wrong. The parallelism will work and the correct result will always be produced. It is guaranteed by the Stream API that the encounter order will be kept when collecting. So your code will work, whether it is parallel or not. Refer here stackoverflow.com/questions/22350288/… – Tunaki Apr 28 '16 at 8:21
• 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: stackoverflow.com/questions/29710999/… 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 Apr 28 '16 at 11:58
• @Necreaux I edited your answer to include this comment. I'm not sure if the author of the question will un-accept it or not, but this way at least it's made evident to future readers. – Phrancis Apr 28 '16 at 16:40

There are many problems with your code, as others have indicated.

However, my guess is that you used features which the person reviewing your code was not familiar with. Quite often the person who determines if you are hired is not an active developer, but a development manager who used to be an active developer.

They are less likely to keep up to speed with the latest features of the language. Showing you are using them to solve a problem will make your code look worse to this ex-developer; they will probably feel ego-challenged (I don't understand this, I am smart, thus this is bad code) by not understanding what your code does or how it solves a problem.

In this case they are right; the code demonstrated is showing off your knowledge of Java features (which could be quite useful in more complex problems) which are not appropriate for this simple task. The parts that are not showing off are not well done; copy-pasta, hard coded constants, brittle tests, undiciplined library use, etc. (Other answers cover this better than I) Finally, apparently your code fails to compile in some "modern" compilers; that could have eliminated you before they even looked at your code!

So if someone doesn't understand those features, the code looks like noise and is ego-bruising. If someone does, they'll say "ok", then look at the rest.

More likely, they where looking for someone with the minimium competence that would solve their problem: if they are hiring someone to write Java to move controls around in a UI, they don't want someone who likes playing with Java8. Or they found a candiate already. Or they changed their mind about hiring because they got distracted. Or you where judged acceptable at Java, but you didn't list SQL on your resume, and there was another candidate who did and was also marked as acceptable in Java, and the HR noticed that SQL was required and elminated your resume from the pool. Or the application process was a fake, and they where just gathering FizzBuzz implementations to feed to a FizzBuzz plagarism detector. Or they where gathering FizzBuzz implementations that could avoid Fizzbuzz plagarism detectors to game some other application process. Or they are someone who gathers resumes and code in order to have a stable of people, then they find contracts and try to become middlemen; if they don't get the contracts, they discard the applicants and repeat.

Quite often things happen that have nothing to do with your performance, and feedback is not provided that it was nothing to do with you. Or, it is something to do with you, and they lie and say it wasn't.

In the future, the thing to keep in mind with FizzBuzz style tests is that they are looking to check if you can program at all. They aren't asking you to show off. The goal of the test is "can you write a simple solution to a simple problem", and to filter out people who cannot program at all. Make your code simple, non-challenging. Try to follow "best practices" in the most conservative way.

• I down-voted this answer because I think it spends too much time talking about interviews etc. and too little talking about the code itself. – Phrancis Apr 28 '16 at 22:07
• The parts of this answer that address the code don't add anything to what has already been said. The other parts are speculation that don't add anything. Why speculate that the reviewer doesn't know the features used when someone who does know those features would still think this is a bad way to write it? – dan1111 Apr 29 '16 at 10:55

I realize we are supposed to keep our comments about the code posted in the question. I am breaking a little from that because this is less a what's wrong with my code than a why did the interviewer not like my code.

Here is my take on a FizzBuzz - see how simple it is?

public class FizzBuzz {

enum Special {

Fizz {
@Override
boolean special(int n) {
return (n % 3) == 0;
}
},
Buzz {
@Override
boolean special(int n) {
return (n % 5) == 0;
}
};

abstract boolean special(int n);
}

static CharSequence encode(int n) {
StringBuilder s = new StringBuilder();
boolean normal = true;
for (Special fb : Special.values()) {

if (fb.special(n)) {
s.append(fb.name()).append(" ");
normal = false;
}
}
if (normal) {
s.append(n).append(" ");
}
return s;
}

private void fizzBuzz() {
for (int i = 1; i < 20; i++) {
System.out.println("" + i + " -> " + s);
}
}

public static void main(String args[]) {
try {
new FizzBuzz().fizzBuzz();
} catch (Throwable t) {
t.printStackTrace(System.err);
}
}
}


Yes it looks tidier and neater but now look what happens when they change their mind and request an enhancement. All I have to do is add:

        Lucky {
@Override
boolean special(int n) {
return Integer.toString(n).contains("3");
}
}


and everything still works.

## Why

This solution demonstrates that if you write your code in certain ways it is naturally malleable in certain directions. In this case, by using an enum, a change in requirements that add new specials can be accomodated trivially.

I suspect the interviewer could clearly see that your code was not malleable in any way - as demonstrated by the fact that you had to duplicate code in several places.

Adage: If you've duplicated code (or any other code smell) you've probably got something wrong. Fix it before you present it to the interviewer.

• What does encode return when n is 15? I think the usual spec for a FizzBuzz problem is it's supposed to return a compound word like "FizzBuzz" (no spaces between "Fizz" and "Buzz"). – David K Apr 29 '16 at 13:18
• @DavidK - That can be fixed by moving the .append(" ");s around. – OldCurmudgeon Apr 29 '16 at 13:21
• Or maybe better, omit them altogether and let the caller decide what separators to use. And yeah, I know it's a pretty small nit to pick. – David K Apr 29 '16 at 13:33
• I mentioned in another comment, but should put into the post, that the duplication isn't real -- it's an artefact of showing how the code has evolved across the three requirements I had to implement, and that in reality I'd only have the third function. I can see clearly now that this is a mistake and confusing. – Software Engineer Apr 29 '16 at 21:32
• This is still an overkill super complicated FizzBuzz and for what it's worth I would fail someone a coding interview for that. – Benjamin Gruenbaum Apr 30 '16 at 6:57