Linq me a FizzBuzz

Question

I got this requirement recently:

Write some code that prints out the following for a contiguous range of numbers:

• the number 'fizz' for numbers that are multiples of 3
• 'buzz' for numbers that are multiples of 5
• 'fizzbuzz' for numbers that are multiples of 15

e.g. if I run the program over a range from 1-20 I should get the following output

1 2 fizz 4 buzz fizz 7 8 fizz buzz 11 fizz 13 14 fizzbuzz 16 17 fizz 19 buzz

This attempt was done this way because I needed to print the sequence on the console and I needed to show the corresponding tests ala TDD.

public static class Evaluate
{
    public static string FizzBuzz(int start, int end)
    {
        return Enumerable.Range(start, (end - start) + 1)
            .Select(FizzOrBuzz)
            .Aggregate(String.Empty, (y, x) => String.Format("{0} {1}", y, x))
            .Trim();
    }

    public static string FizzOrBuzz(int n)
    {
        if (n % 15 == 0) return "fizzbuzz";
        if (n % 3 == 0) return "fizz";
        if (n % 5 == 0) return "buzz";
        return n.ToString();
    }
}

Answer 1

I'd prefer to use the String.Join method instead of the Aggregate:

public static string FizzBuzz(int start, int end)
{
    return String.Join(" ", Enumerable.Range(start, end - start + 1).Select(FizzOrBuzz));
}

This should eliminate multiple string concatenations.

EDIT. Removed the superfluous generic type parameters.

Answer 2

Your names aren't great. For example Evaluate as a class name doesn't tell me anything.

From a TDD point of view, it would be good to get at the sequence generated, not just the resulting string. This lets you test things like expected number of elements.

The actual logic of your methods is good.

Renamed version with extra method:

public static class FizzBuzz
{
    public static string GenerateDisplayString(int start, int end)
    {
        return GenerateSequence(start, end)
            .Aggregate(String.Empty, (y, x) => String.Format("{0} {1}", y, x))
            .Trim();
    }

    public static IEnumerable<string> GenerateSequence(int start, int end)
    {
        return Enumerable.Range(start, (end - start) + 1)
            .Select(GetForDisplay);
    }

    private static string GetForDisplay(int number)
    {
        if (number % 15 == 0) return "fizzbuzz";
        if (number % 3 == 0) return "fizz";
        if (number % 5 == 0) return "buzz";
        return number.ToString();
    }
}

I'd then test the GenerateSequence method.

Answer 3

Are you sure about your requirements? In particular this:

'fizzbuzz' for numbers that are multiples of 15

The problem is generally stated as

Print "FizzBuzz" if the number is a multiple of 3 and 5.

If your interviewer gave you the requirements that you posted, then you're good. However, if the requirements were actually the latter, then you've missed an opportunity to keep your code flexible and DRY.

Based on the classic requirements, imagine that the numbers changed and you now needed to print "Fizz" for multiples of 5 and "Buzz" for multiples of 7. Instead of making two changes to the code, you would have to make three. Not only that, but the maintainer would need to understand that 15 is the lowest common multiple of 3 and 5 to be able to calculate the new constant for "FizzBuzz" numbers. You might be better of removing the 15 constant and making it what it really is: the lowest common multiple of the other two.

Answer 4

I suggest that using modulus or division operators for each item in series is unnecessarily expensive, both are processor intensive compared to simple adds and comparisons.

An enumerator to generate the series can be trivially specified, as below, avoiding repeated use of %. I've included an optional start parameter so the series can be initiated after the traditional beginning, without a wasteful Skip call.

public static IEnumerable<string> FizzBuzzSeries(long start = 1)
{
    const ulong FFizz = 3UL;
    const ulong FBuzz = 5UL;

    if (start < 1)
    {
        throw new ArgumentOutOfRangeException("start");
    }

    var i = (ulong)start;
    var fizz = FFizz;
    if (i > FFizz)
    {
        fizz = i + (FFizz - (i % FFizz));
    }

    var buzz = FBuzz;
    if (i > FBuzz)
    {
        buzz = i + (FBuzz - (i % FBuzz));
    }

    var s = new StringBuilder(8);
    for (; i <= ulong.MaxValue; i++)
    {
        if (i == fizz)
        {
            fizz += FFizz;
            s.Append("fizz");
        }

        if (i == buzz)
        {
            buzz += FBuzz;
            s.Append("buzz");
        }

        if (s.Length > 0)
        {
            yield return s.ToString();
            s.Clear();
        }
        else
        {
            yield return i.ToString(CultureInfo.InvariantCulture);
        }
    }
}

To get the output as you desire,

Console.WriteLine(string.Join(" ", FizzBuzzSeries().Take(20)));

Answer 5

When something is simple, keep it simple. I think a one line problem should be solved in one line:

return String.Join(
        Environment.NewLine,
        Enumerable.Range(start, (end - start) + 1)
          .Select(n => n % 15 == 0 ? "fizzbuzz" 
                     : n % 3 == 0 ? "fizz" 
                     : n % 5 == 0 ? "buzz" 
                     : n.ToString())