posts - 293 , comments - 426 , trackbacks - 0

My Links

News

View Steve Michelotti's profile on LinkedIn

Twitter












Tag Cloud

Archives

Post Categories

My Online Presence

Refactoring FizzBuzz

 

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

        Console.ReadLine();
    }
}

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;
    private readonly FizzBuzzComparison _comparison1;
    private readonly FizzBuzzComparison _comparison2;

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

        Console.ReadLine();
    }
}

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;
        private readonly FizzBuzzComparison _comparison1;
        private readonly FizzBuzzComparison _comparison2;

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

            Console.ReadLine();
        }
    }

 

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!

Print | posted on Monday, October 15, 2012 7:58 PM | Filed Under [ C# ]

Feedback

Gravatar

# re: Refactoring FizzBuzz

Have you seen Enterprise Fizzbuzz?
http://codermike.com/if-something-is-worth-doing
10/17/2012 8:41 PM | Neyah
Gravatar

# 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.
10/18/2012 7:35 AM | Brady Kelly
Gravatar

# 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.
10/18/2012 8:33 AM | Mark Pearl
Gravatar

# 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!)
10/18/2012 12:16 PM | Roopesh Shenoy
Gravatar

# 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
10/18/2012 12:37 PM | JB
Gravatar

# re: Refactoring FizzBuzz

How about:

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.

10/18/2012 12:43 PM | Dave
Gravatar

# re: Refactoring FizzBuzz

@Dave - great point about the name of the FizzBuzzCalculate - it should definately be renamed.

I could have continued to refactor and extract the logic out of the main method but decided to stop at iteration 4. But all valid points.
10/18/2012 1:00 PM | Mark Pearl
Gravatar

# re: Refactoring FizzBuzz

@Roopesh - I am not advocating one approach over the other - in the end it depends on what you are writing and the bigger solution.

Number of lines of code is important - but in a larger system this functionality could have been reused over and over again, in which case you would have less lines of code than more.

Thanks for the naming point - totally agree!
10/18/2012 1:04 PM | Mark Pearl
Gravatar

# 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()?
10/18/2012 1:53 PM | Bob Beechey
Gravatar

# 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; });
10/18/2012 1:58 PM | Deon
Gravatar

# re: Refactoring FizzBuzz

I also like option 1
10/18/2012 2:14 PM | Peter Munnings
Gravatar

# 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 :-)
10/18/2012 2:04 PM | Garren
Gravatar

# 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())));
10/18/2012 2:27 PM | Peter Munnings
Gravatar

# re: Refactoring FizzBuzz

@Garren, totally agree on making things to extensible, the gauntlet has be thrown for putting it in a dynamic language ;-)
10/18/2012 2:12 PM | MarkPearl
Gravatar

# re: Refactoring FizzBuzz

@Deon... nice!
10/18/2012 2:13 PM | MarkPearl
Gravatar

# re: Refactoring FizzBuzz

Time to get back to work:

$(document).ready(function () {
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);
});
10/18/2012 2:36 PM | Peter Munnings
Gravatar

# 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.
10/18/2012 2:52 PM | Dominic Amann
Gravatar

# re: Refactoring FizzBuzz

@DominicAmann - thank you for your unsarcastic comment, please by a larger monitor and reduce your screen res.
10/18/2012 4:01 PM | Mark Pearl
Gravatar

# 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.
10/18/2012 5:14 PM | Jasmine
Gravatar

# 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.
10/18/2012 5:15 PM | Erzengel
Gravatar

# 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)
]);
}
}
}
}
10/18/2012 9:20 PM | IUnknown
Gravatar

# 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.
10/18/2012 10:34 PM | Zan Lynx
Gravatar

# 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.
10/18/2012 11:17 PM | Harley Pebley
Gravatar

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

10/19/2012 3:01 AM | Vadim
Gravatar

# re: Refactoring FizzBuzz

@Zan - Thanks Zan, interesting comment
10/19/2012 6:06 AM | MarkPearl
Gravatar

# 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();
}
10/19/2012 2:22 PM | lindholm
Gravatar

# 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;
10/19/2012 7:17 PM | Randy
Gravatar

# 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 :)
10/22/2012 2:18 PM | Nils
Gravatar

# 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.
10/22/2012 4:10 PM | Kevin Trethewey
Gravatar

# re: Refactoring FizzBuzz

@Nils - you may find this totally overwhelming, but I do this sort of thing in my own time ;-)
10/22/2012 3:57 PM | MarkPearl
Post A Comment
Title:
Name:
Email:
Comment:
Verification:
 
 

Powered by: