22
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ You can make FizzOrBuzz private, other than that everything seems fine to me \$\endgroup\$ – Vsevolod Goloviznin Jan 13 '15 at 12:29
  • 2
    \$\begingroup\$ @RubberDuck Apparently you haven't seen this: codereview.stackexchange.com/questions/49058/… \$\endgroup\$ – Simon Forsberg Jan 13 '15 at 13:05
  • \$\begingroup\$ @SimonAndréForsberg Good point. That discussion prompted me to post the question. \$\endgroup\$ – chosenbreed37 Jan 13 '15 at 13:15
  • \$\begingroup\$ curiously similar to this answer stackoverflow.com/a/5661471/659190 \$\endgroup\$ – Jodrell Jan 14 '15 at 15:22
  • 2
    \$\begingroup\$ I'll never understand the unhealthy obsession with fizzbuzzing the modulo operator. Practical in professional career: Never. Occurrence in interview screenings: Always. \$\endgroup\$ – Andrew Hoffman Jan 14 '15 at 16:37
18
\$\begingroup\$

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ Note that the generic type parameters are superfluous, they can be inferred from the signature of FizzOrBuzz - so .Select(FizzOrBuzz) works just as well. \$\endgroup\$ – Mathieu Guindon Jan 13 '15 at 17:31
  • \$\begingroup\$ @Mat'sMug I've removed the generic type parameters. Thank you. \$\endgroup\$ – Dmitry Jan 13 '15 at 19:28
8
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ The naming troubled me. It was a timed exercise so I didn't dwell too much on it. I agree with extracting out the display concerns to a separate function. Thank you for your time. \$\endgroup\$ – chosenbreed37 Jan 13 '15 at 13:12
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I literally copy+pasted the requirement (offline test). I agree with your points though. \$\endgroup\$ – chosenbreed37 Jan 13 '15 at 13:01
  • 12
    \$\begingroup\$ If there are any holy wars in FizzBuzz, this would be it. \$\endgroup\$ – Simon Forsberg Jan 13 '15 at 13:03
  • 3
    \$\begingroup\$ The "classic requirements" were a test if the interviewee could program at all. This is overthinking it by quite a bit. \$\endgroup\$ – RemcoGerlich Jan 13 '15 at 14:55
  • \$\begingroup\$ @RemcoGerlich you might find this humorous. \$\endgroup\$ – RubberDuck Jan 13 '15 at 14:57
  • 1
    \$\begingroup\$ This seems to assume that the requirements you're given state the problem in the way that best describes its underlying structure. In real life that's often not the case, and discovering the structure is part of your job as a developer. \$\endgroup\$ – Ben Aaronson Jan 14 '15 at 16:45
4
\$\begingroup\$

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)));
\$\endgroup\$
  • \$\begingroup\$ ok, I guess this works but I think using an else is much better than using a continue. continue functionally is very close the goto statement and IMO has similar issues. else would work just fine. \$\endgroup\$ – Hogan Jan 14 '15 at 16:58
  • \$\begingroup\$ @Hogan, as you wish. \$\endgroup\$ – Jodrell Jan 14 '15 at 16:59
  • \$\begingroup\$ @Jodrell a novel (at least for me) implementation. Nicely done. I like this solution. Perhaps I will use it next time I get it at an interview :-) \$\endgroup\$ – chosenbreed37 Jan 15 '15 at 17:10
  • \$\begingroup\$ @Jodrell...having said that all efficiency appears to be lost if you do this: FizzBuzzSeries.Skip(1000000).Take(20)... \$\endgroup\$ – chosenbreed37 Jan 15 '15 at 17:52
  • \$\begingroup\$ @chosenbreed37 I added an optional start parameter. \$\endgroup\$ – Jodrell Jan 16 '15 at 9:53
1
\$\begingroup\$

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())
\$\endgroup\$
  • 1
    \$\begingroup\$ Why do you think this is simpler than the top voted answer? I don't believe it's significantly simpler. \$\endgroup\$ – recursive Jan 13 '15 at 18:48
  • \$\begingroup\$ Simple in terms of what it compiles down to? Probably not. However it's different from the top answer because the method is inline so you can read it right there, and the method body is one line instead of 4 so you don't need to use brackets and semicolons inside the lambda. \$\endgroup\$ – moarboilerplate Jan 13 '15 at 23:47
  • \$\begingroup\$ I'm not talking about the MSIL compilation. I'm talking about the code. It is certainly fewer statements, but I don't think that makes it any easier to read. Statements aren't always bad. They do make it pretty easy to add breakpoints, for example. \$\endgroup\$ – recursive Jan 14 '15 at 1:29
  • \$\begingroup\$ @recursive - I think the code stands for itself, trying to explain why I think it is simpler is pointless. Each person will find it simpler or not. I'm not saying it is better because it is simpler -- that is different issue. But I do think that half as many lines and 5 less object names is simpler in this case. There are other cases where reducing the lines and number of names makes something more complex... but in my opinion not in this case. \$\endgroup\$ – Hogan Jan 14 '15 at 16:50
  • \$\begingroup\$ That's fair enough. \$\endgroup\$ – recursive Jan 14 '15 at 23:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.