Geeks With Blogs

News

A few years ago I blogged about FizzBuzz, at the time the post was prompted by Scott Hanselman who had podcasted about how surprized he was that some programmers could not even solve the FizzBuzz problem within a reasonable period of time during a job interview.

Originally I thought I would give the problem a go in F# and sure enough the solution was fairly simple – I then also did a basic solution in C# but never posted it. Since then I have learned that being able to solve a problem and how you solve the problem are two totally different things. Today I decided to give the problem a retry and see if I had learnt anything new in the last year or so. Here is how my solution looked after refactoring…

### Solution 1 – Cheap and Nasty

```public class FizzBuzzCalculator
{

public string NumberFormat(int number)
{
var numDivisibleBy3 = (number % 3) == 0;
var numDivisibleBy5 = (number % 5) == 0;

if (numDivisibleBy3 && numDivisibleBy5) return String.Format("{0} FizzBuz", number);
else if (numDivisibleBy3) return String.Format("{0} Fizz", number);
else if (numDivisibleBy5) return String.Format("{0} Buz", number);

return number.ToString();
}

}

class Program
{
static void Main(string[] args)
{
var fizzBuzz = new FizzBuzzCalculator();

for (int i = 0; i < 100; i++)
{
Console.WriteLine(fizzBuzz.NumberFormat(i));
}
}
}```

My first attempt I just looked at solving the problem – it works, and could be an acceptable solution but tonight I thought I would see how far  I could refactor it… The section I decided to focus on was the mass of if..else code in the NumberFormat method.

### Solution 2 – Replacing If…Else with a Dictionary

```public class FizzBuzzCalculator
{
private readonly Dictionary<Tuple<bool, bool>, string> _mappings;

public FizzBuzzCalculator(Dictionary<Tuple<bool, bool>, string> mappings)
{
_mappings = mappings;
}

public string NumberFormat(int number)
{
var numDivisibleBy3 = (number % 3) == 0;
var numDivisibleBy5 = (number % 5) == 0;

var mappedKey = new Tuple<bool, bool>(numDivisibleBy3, numDivisibleBy5);

return String.Format("{0} {1}", number, _mappings[mappedKey]);
}
}

class Program
{
static void Main(string[] args)
{
var mappings = new Dictionary<Tuple<bool, bool>, string>
{
{ new Tuple<bool, bool>(true, true), "- FizzBuzz"},
{ new Tuple<bool, bool>(true, false), "- Fizz"},
{ new Tuple<bool, bool>(false, true), "- Buzz"},
{ new Tuple<bool, bool>(false, false), ""}
};

var fizzBuzz = new FizzBuzzCalculator(mappings);

for (int i = 0; i < 100; i++)
{
Console.WriteLine(fizzBuzz.NumberFormat(i));
}

}
}```

In my second attempt I looked at removing the if else in the NumberFormat method. A dictionary proved to be useful for this – I added a constructor to the class and injected the dictionary mapping. One could argue that this is totally overkill, but if I was going to use this code in a large system an approach like this makes it easy to put this data in a configuration file, which would up its OC (Open for extensibility, closed for modification principle).

I could of course take the OC principle even further – the check for divisibility by 3 and 5 is tightly coupled to this class. If I wanted to make it 4 instead of 3, I would need to adjust this class. This introduces my third refactoring.

### Solution 3 – Introducing Delegates and Injecting them into the class

```public delegate bool FizzBuzzComparison(int number);

public class FizzBuzzCalculator
{
private readonly Dictionary<Tuple<bool, bool>, string> _mappings;

public FizzBuzzCalculator(Dictionary<Tuple<bool, bool>, string> mappings, FizzBuzzComparison comparison1, FizzBuzzComparison comparison2)
{
_mappings = mappings;
_comparison1 = comparison1;
_comparison2 = comparison2;
}

public string NumberFormat(int number)
{
var mappedKey = new Tuple<bool, bool>(_comparison1(number), _comparison2(number));

return String.Format("{0} {1}", number, _mappings[mappedKey]);
}

}

class Program
{
private static bool DivisibleByNum(int number, int divisor)
{
return number % divisor == 0;
}

public static bool Divisibleby3(int number)
{
return number % 3 == 0;
}

public static bool Divisibleby5(int number)
{
return number % 5 == 0;
}

static void Main(string[] args)
{
var mappings = new Dictionary<Tuple<bool, bool>, string>
{
{ new Tuple<bool, bool>(true, true), "- FizzBuzz"},
{ new Tuple<bool, bool>(true, false), "- Fizz"},
{ new Tuple<bool, bool>(false, true), "- Buzz"},
{ new Tuple<bool, bool>(false, false), ""}
};

var fizzBuzz = new FizzBuzzCalculator(mappings, Divisibleby3, Divisibleby5);

for (int i = 0; i < 100; i++)
{
Console.WriteLine(fizzBuzz.NumberFormat(i));
}

}```
`}`

I have taken this one step further and introduced delegates that are injected into the FizzBuzz Calculator class, from an OC principle perspective it has probably made it more compliant than the previous Solution 2, but there seems to be a lot of noise. Anonymous Delegates increase the readability level, which is what I have done in Solution 4.

### Solution 4 – Anon Delegates

```    public delegate bool FizzBuzzComparison(int number);

public class FizzBuzzCalculator
{
private readonly Dictionary<Tuple<bool, bool>, string> _mappings;

public FizzBuzzCalculator(Dictionary<Tuple<bool, bool>, string> mappings, FizzBuzzComparison comparison1, FizzBuzzComparison comparison2)
{
_mappings = mappings;
_comparison1 = comparison1;
_comparison2 = comparison2;
}

public string NumberFormat(int number)
{
var mappedKey = new Tuple<bool, bool>(_comparison1(number), _comparison2(number));
return String.Format("{0} {1}", number, _mappings[mappedKey]);
}

}

class Program
{
static void Main(string[] args)
{
var mappings = new Dictionary<Tuple<bool, bool>, string>
{
{ new Tuple<bool, bool>(true, true), "- FizzBuzz"},
{ new Tuple<bool, bool>(true, false), "- Fizz"},
{ new Tuple<bool, bool>(false, true), "- Buzz"},
{ new Tuple<bool, bool>(false, false), ""}
};

var fizzBuzz = new FizzBuzzCalculator(mappings, (n) => n % 3 == 0, (n) => n % 5 == 0);

for (int i = 0; i < 100; i++)
{
Console.WriteLine(fizzBuzz.NumberFormat(i));
}

}
}```

Using the anonymous delegates I think the noise level has now been reduced. This is where I am going to end this post, I have gone through 4 iterations of the code from the initial solution using If..Else to delegates and dictionaries. I think each approach would have it’s pro’s and con’s and depending on the intention of where the code would be used would be a large determining factor. If you can think of an alternative way to do FizzBuzz, add a comment!

Related Posts on Geeks With Blogs Matching Categories

Comments on this post: Refactoring FizzBuzz

# re: Refactoring FizzBuzz
Have you seen Enterprise Fizzbuzz?
http://codermike.com/if-something-is-worth-doing
Left by Neyah on Oct 17, 2012 8:41 PM

# re: Refactoring FizzBuzz
While it is fun exploring academically superior options, if I was the one doing the hiring, your very first solution would score you the most points.
Left by Brady Kelly on Oct 18, 2012 7:35 AM

# re: Refactoring FizzBuzz
Hi Brady, the appropriate solution really depends on the problem domain, if I was interviewing someone for a senior developer position for a large enterprise application then I would probably tend towards someone that answered one of the later solutions. If I was looking for someone to work on smaller product apps, the first option could be more than appropiate.
Left by Mark Pearl on Oct 18, 2012 8:33 AM

# re: Refactoring FizzBuzz
Number of lines to code is an important parameter -

It's easy to start doing things that "you might require" much before you actually do and bloat your program to hell.

Also remember you would want to write unit tests to be thorough - the number of tests you have to write just increased multi-fold. Not to mention that the name "FizzBuzzCalculator" is no longer appropriate (because it's not necessary it's going to type Fizz and Buzz any more and will depend on the mappings!)
Left by Roopesh Shenoy on Oct 18, 2012 12:16 PM

# re: Refactoring FizzBuzz
I too would go with Option 1 when hiring.

1. Easier to read (Both junior & senior developers will quickly understand what the code does)
2. The least amount of coding invlolved
3. Less code = less likelihood of bugs
4. Less code = easier to troubleshoot if necessary
Left by JB on Oct 18, 2012 12:37 PM

# re: Refactoring FizzBuzz

Soln 4. FizzBuzzCalculator is NOT a FizzBussCalculator. If you pass in a different mapping, it could output Ping/Pong instead of Fizz/Buzz.
The fundamental class name is misleading. It knows NOTHING about Fizz or Buzz

You have FAR Too much detail in Main().

Create a new class eg _3_5_FizzBuzzCalculator which contains the ClassFormerlyKnownAsFizzBuzz, and contains the mappings, and the delegates.

You now have a generic translation class, and a specific instance of the class which deals with 3/5/fizz/buzz.

If you expect 3/5/fizz/buzz to change parameters often, create a factory class which can "source" these parameters from config, database or where ever.

Make sure that you can use an alternate factory class which does not need database/files so that you can run unit tests.

Left by Dave on Oct 18, 2012 12:43 PM

# re: Refactoring FizzBuzz
I prefer option 1, except NumberFormat() could (should?) be a static method so that no irrelevant instance of FizzBuzzCalculator need be constructed. Come to that, why does a separate class exist at all to merely contain NumberFormat()?
Left by Bob Beechey on Oct 18, 2012 1:53 PM

# re: Refactoring FizzBuzz
Ever seen this implementation?

Enumerable.Range(1, 100)
.Select(a => string.Format("{0}{1}", a%3 == 0 ? "Fizz" : "", a%5 == 0 ? "Buzz" : string.Empty))
.Select((b, i) => string.IsNullOrWhiteSpace(b) ? (i + 1).ToString() : b)
.Aggregate((a, b) => { Console.WriteLine(a); return b; });
Left by Deon on Oct 18, 2012 1:58 PM

# re: Refactoring FizzBuzz
I also like option 1
Left by Peter Munnings on Oct 18, 2012 2:14 PM

# re: Refactoring FizzBuzz
Hey Mark,

Great blog post really interesting to see your thinking. I must admit I would also choose option number 1. Its easier to read and if it was a 3rd party lib I had to use, easy to setup and use.

The other options are interesting from a coding perspective of how much can you "refactor" or change to make it extensible or adaptable. I've found the code that tries to cater for more than originally specified or allow for a lot of change are normally the worst. Very rarely have I found my "extensible" code able to be extensible in the way it needs to be.

New challenge write it in 2 other languages, with one of them being a dynamic language :-)
Left by Garren on Oct 18, 2012 2:04 PM

# re: Refactoring FizzBuzz
Couldn't resist

for(var i = 0; i<=100; i++) Console.WriteLine(i%15==0?"FizzBuzz":(i%3==0?"Fizz":(i%5==0?"Buzz":i.ToString())));
Left by Peter Munnings on Oct 18, 2012 2:27 PM

# re: Refactoring FizzBuzz
Time to get back to work:

var output = "";
for (var i = 0; i <= 100; i++) {output += (i % 15 == 0 ? "FizzBuzz" : (i % 3 == 0 ? "Fizz" : (i % 5 == 0 ? "Buzz" : i))) + "<br />";}
\$("#div_out").html(output);
});
Left by Peter Munnings on Oct 18, 2012 2:36 PM

# re: Refactoring FizzBuzz
Unfortunately, I chose not to read you blog post because it forces an extremely long line width. I know from experience that it is hard to read very long lines because my eyes don't know when to start thye next line once I reach an end, and also I have to scroll my browser to get to the second half of the line.

Thanks for contributing.
Left by Dominic Amann on Oct 18, 2012 2:52 PM

# re: Refactoring FizzBuzz
Why does everybody check the "FizzBuzz" condition on these? "FizzBuzz" is simply the conjunction of the other two conditions. If you check Fizz first, leave the cursor where it is, then check Buzz and print if it's also true. Print a new line between numbers and you're done, and you avoid an entire conditional branch that way.
Left by Jasmine on Oct 18, 2012 5:14 PM

# re: Refactoring FizzBuzz
I hate all of these. They are all needlessly complicated. Is it really so hard to use a flag?
class FizzBuzzPrinter
{
bool PrintNumber=true;
void PrintWord(string Word)
{
Console.Write(Word);
PrintNumber = false;
}
void TryPrintNumber(int Number)
{
if(PrintNumber)
{
Console.Write(Number);
}
Console.WriteLine();
PrintNumber = true;
}
public void Execute(int min, int max)
{
for(int i=min; i< max; ++i)
{
if(i%3 == 0)
{
PrintWord("Fizz");
}
if(i%5 == 0)
{
PrintWord("Buzz");
}
TryPrintNumber(i);
}
}
}

Clean, compartmentalized, extensible. No delegates, dictionaries, or other unnecessary complexities.
Left by Erzengel on Oct 18, 2012 5:15 PM

# re: Refactoring FizzBuzz
Just had to do it right without if's :)
So here's a better version:

using System;
using System.Collections.Generic;

namespace Dela.Mono.Examples
{
public class HelloWorld
{
public static Dictionary<int,string> fb = new Dictionary<int,string>() {
{ 1, "- Fizz" },
{ 2, "- Buzz" },
{ 3, "- FizzBuzz" }
};

public static void Main(string[] args)
{
string os = "";
for (int i=1; i<=100; i++)
{
fb[0] = i.ToString();
Console.WriteLine(
fb[
(Convert.ToInt32((i % 3)==0) +
Convert.ToInt32((i % 5)==0)*2)
]);
}
}
}
}
Left by IUnknown on Oct 18, 2012 9:20 PM

# re: Refactoring FizzBuzz
I'd just like to point out that removing the if/else statements and replacing them with a Dictionary is not gaining you anything in performance. I don't believe its more readable either.

The Dictionary is, at best, doing a hash. After that it must still compare the values to make sure it didn't find a hash collision.

Your best bet would be an array of four strings. The index would be picked by DivisibleBy3 + DivisibleBy5*2. This would result in no branches and get the best pipeline performance.
Left by Zan Lynx on Oct 18, 2012 10:34 PM

# re: Refactoring FizzBuzz
I did something like this a while back. Not being in .Net 4, I didn't try tuples and was sort of surprised they worked based on my experience using arrays in a syntactically similar way. (http://www.skylark-software.com/2012/06/functional-fizzbuzz-in-c.html) Must be enough semantic differences with tuples to work properly.

Thanks for the write-up.
Left by Harley Pebley on Oct 18, 2012 11:17 PM

# re: Refactoring FizzBuzz
As a tech lead, I won’t hire any developer who will suggest a solution more complicated than the first one.
A lot of people tend to overcomplicate tasks and produce unmaintainable code with horrible performance and unreasonable memory consumption.
In most cases, the simplest solution is the best one.

Left by Vadim on Oct 19, 2012 3:01 AM

# re: Refactoring FizzBuzz
here is the code I did for it using C# (note mine had divisible by 7 = bazz)

void Main()
{
string l;
Enumerable
.Range(1, 200)
.Select(x => (
(
l = string.Join("",
(new []{ new {v=3, n="Fizz"}, new {v=5, n="Buzz"}, new {v=7, n="Bazz"}})
.ToList()
.Select(v => x % v.v == 0 ? v.n : null))
) != "" ? l : x.ToString() )).Dump();
}
Left by lindholm on Oct 19, 2012 2:22 PM

# re: Refactoring FizzBuzz
I know I'll likely get trashed for this but I would go with #1 but use goto's instead of ifelse's. I like a single exit point instead of mutiple returns,

var numDivisibleBy3 = (number % 3) == 0;
var numDivisibleBy5 = (number % 5) == 0;
string result;

if (numDivisibleBy3 && numDivisibleBy5) {result = string.Format("{0} FizzBuz", number); goto singleExit;} // 3&5
if (numDivisibleBy3) {result = String.Format("{0} Fizz", number); goto singleExit;} // 3
if (numDivisibleBy5) {result = String.Format("{0} Buz", number); goto singleExit;} // 5
result = number.ToString(); // No condition met

singleExit:

return result;
Left by Randy on Oct 19, 2012 7:17 PM

# re: Refactoring FizzBuzz
If I were your boss, I would lay you off, because this is such a trivial little thing and trying to come up with a zillion different exotic options is complete overkill and misuse of the bosses resources :)
Left by Nils on Oct 22, 2012 2:18 PM

# re: Refactoring FizzBuzz
Without picking one solution over the other (as Mark points out, impossible without knowing a bit more of the context), I'd just like to raise the issue of littering your code with 'ifs'...

The moment you add an if to your code you add a potential place that will need to change should requirements change. Changing existing code is an opportunity for bugs to creep in, especially when this change is needed in multiple places as often happens with if conditionals.

In an OO language a very high percentage of ifs can be replaced simply by using object orientation 'properly'.

Misko does a way better job of explaining this than I can: http://www.youtube.com/watch?v=4F72VULWFvc

PS: Anyone who makes a hiring decision purely on the basis of a single coded problem done within an interview context is probably not from the kind of team that I might like to work for.
Left by Kevin Trethewey on Oct 22, 2012 4:10 PM