8
\$\begingroup\$

For my first CodeReview, I would like to present my FizzBuzz solution in C#. My design has all of the rule logic about when to display what in separate classes through an interface. This design should be able to handle the common follow-up changes (ie. mod 7 instead of mod 3, bazz, etc.) for this problem by simply adding another rule to the game. Below is the problem statement.

And as I am not a professional developer, I am especially interested in additional feedback on style, formatting, and adherence to SOLID principles, as well as limitations of this design.

Problem: Write a short program that prints each number from 1 to 100 on a new line. For each multiple of 3, print "Fizz" instead of the number. For each multiple of 5, print "Buzz" instead of the number. For numbers which are multiples of both 3 and 5, print "FizzBuzz" instead of the number.

Solution:

class Program
{
    static void Main(string[] args)
    {
        var rules = new List<IRule>
        {
            new FizzMod3Rule(),
            new BuzzMod5Rule(),
            new BazzMod7Rule()
        };

        new FizzBuzzGame(rules).Play();

        Console.ReadKey();
    }
}

public class FizzBuzzGame
{
    private List<IRule> _rules;

    public FizzBuzzGame(List<IRule> rules)
    {
        _rules = rules ?? new List<IRule>();
    }

    public void Play(int numberOfRounds = 100)
    {
        for (var i = 1; i <= numberOfRounds; i++)
        {
            var output = string.Empty;
            foreach (var rule in _rules) { output += rule.Run(i); }
            if (output == string.Empty) { output += new EmptyRule().Run(i); }
            Console.WriteLine(output);
        }
    }
}

public interface IRule
{
    string Run(double number);
}

public class FizzMod3Rule : IRule
{
    public string Run(double number)
    {
        return (number % 3) == 0 ? "Fizz" : string.Empty;
    }
}

public class BuzzMod5Rule : IRule
{
    public string Run(double number)
    {
        return (number % 5) == 0 ? "Buzz" : string.Empty;
    }
}

public class BazzMod7Rule : IRule
{
    public string Run(double number)
    {
        return (number % 7) == 0 ? "Bazz" : string.Empty;
    }
}

public class EmptyRule : IRule
{
    public string Run(double number)
    {
        return number.ToString();
    }
}

Output:

1
2
Fizz
4
Buzz
Fizz
Bazz
8
Fizz
Buzz
11
Fizz
13
Bazz
FizzBuzz
16
17
Fizz
19
Buzz
FizzBazz
...
\$\endgroup\$
  • 2
    \$\begingroup\$ That's a well written first question! \$\endgroup\$ – Denis Nov 2 at 1:58
  • \$\begingroup\$ Using the EmptyRule class just to append a number to an empty string is an overkill. You should simply do output = i.toString() \$\endgroup\$ – jakubiszon Nov 2 at 7:56
7
\$\begingroup\$

The intention of classes in object oriented programming is not to replace if sentences, but to model code that differs in behavior. Your 4 classes can be merged into 2, since the first 3 of them use structurally identical code. The code could then be:

var rules = new List<IRule>
{
    new ModWordRule(3, "Fizz"),
    new ModWordRule(5, "Buzz"),
    new ModWordRule(7, "Bazz")
};

On second sight, your code completely misses the point of the task. The task says:

Write a short program.

Your program has 74 lines, and I'm not even counting the using declarations that you omitted at the top of the file. You should not omit these when you post your code for code review.

I just wrote the same program in 20 lines, without all the "object oriented" boilerplate. It is easier readable, and equally easy to extend to more rules.

I don't see any point in adding so many interfaces and classes if the problem can be solved in a few lines of code, especially if the simple code is easily testable.

The argument of "don't modify the existing classes when adding features" is moot here since adding a "7 Bazz" rule obviously has to modify the output, and that output has to be assembled by some class. In the FizzBuzz example the code is still simple enough to go into a single function instead of spreading it over 4 classes.

The task does not mention the ReadKey anywhere, therefore you should omit it.

The task says to output "7" for the number 7, your program outputs "Bazz". This latter word is mentioned nowhere in the task.

When dealing with collections of things, it is a common assumption that these collections are never null. Therefore there's no need to check for that condition in the FizzBuzzGame constructor. Just let the program crash in such a case. It's a serious programming mistake, and it must be fixed. Your current code only hides the bug.

As I said above, I prefer code that is to the point, while still testable. Such as the following:

using System;

namespace FizzBuzz231713
{
    class Program
    {
        static object FizzBuzz(int n)
        {
            var fizz = n % 3 == 0 ? "Fizz" : "";
            var buzz = n % 5 == 0 ? "Buzz" : "";
            var word = fizz + buzz;
            return word != "" ? word : (object)n;
        }

        static void Main()
        {
            for (int i = 0; i < 100; i++)
                Console.WriteLine(FizzBuzz(i + 1));
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Not wishing to take anything any from the point of this answer, I'd take issue with " It is [..] equally easy to extend to more rules." since it requires recompiling FizzBuzz, which would seem to be the point of the OP's efforts, regardless of the problem section. \$\endgroup\$ – VisualMelon Nov 2 at 15:15
  • 1
    \$\begingroup\$ @VisualMelon In the OP's code adding another rule would require adding a class and recompiling both the added class and Main. How is that an improvement? \$\endgroup\$ – Roland Illig Nov 2 at 17:19
  • \$\begingroup\$ The code could be used from another assembly, and new rules provided without recompiling the assembly containing FizzBuzzGame... but yeah, the spec is asking for a program and not a library, so I'm looking at this the wrong way. \$\endgroup\$ – VisualMelon Nov 2 at 17:33
  • \$\begingroup\$ @RolandIllig Thanks for the good feedback! I don't wish to spark another "overkill OO" discussion. This may not be the best candidate for OOP. But from what I have read on other "interview questions" here, interviewers not only look for the ability to write an algorithm, but that one has considered future changes and understands OO principles. And I interpret the Open/Closed principle to not apply to the Main method, just the FizzBuzzGame class and the IRule classes. Otherwise, you couldn't modify anything. \$\endgroup\$ – A. Neely Nov 4 at 19:35
  • \$\begingroup\$ interpret Open/Closed principle to not apply to the Main O/C is not a "don't touch this" on/off switch. And it is absolutely not an absolute dictate. Rather, it is a question: "Why do I have to modify this class?" or "Why so often?" There are good reasons, but not here. The top level of FizzBuzz is not in the FizzBuzz class. FizzBuzz is not reusable because the top level of it must be rewritten every time - let's say this is the practical perspective of the Single Responsibility Principle. Main should only ever start the program. \$\endgroup\$ – radarbob Nov 5 at 20:19

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.