Does this FizzBuzz code correctly follow SRP and unit-testing?

Question

This is code for FizzBuzz. I have also hosted in on my GitHub.

FizzChecker Tests

[TestClass]
public class FizzChecker_Tests
{
 IFizzChecker _fizzChecker;
 bool isFizz;
 [TestInitialize]
 public void TestInitialize()
 {
 //Arrange
 _fizzChecker = new FizzChecker();
 isFizz = default(bool);
 }
 [TestMethod]
 public void When_Devisible_By_Three_Return_True()
 {
 //Act
 isFizz = _fizzChecker.IsFizz(3);
 //Assert
 Assert.AreEqual(isFizz, true);
 }
 [TestMethod]
 public void When_Not_Devisible_By_Three_Return_False()
 {
 //Act
 isFizz = _fizzChecker.IsFizz(1);
 //Assert
 Assert.AreEqual(isFizz, false);
 }
 [TestMethod]
 public void When_Devide_By_Zero_Return_True()
 {
 //Act
 isFizz = _fizzChecker.IsFizz(0);
 //Assert
 Assert.AreEqual(isFizz, true);
 }
}

Fizz

public interface IFizzChecker
{
 bool IsFizz(int number);
}
public class FizzChecker : IFizzChecker
{
 public bool IsFizz(int number)
 {
 return (number % 3) == 0;
 }
}

Buzz tests

[TestClass]
public class BuzzChecker_Tests
{
 BuzzChecker _buzzChecker;
 bool isFizz;
 [TestInitialize]
 public void TestInitialize()
 {
 //Arrange
 _buzzChecker = new BuzzChecker();
 isFizz = default(bool);
 }
 [TestMethod]
 public void When_Devisible_By_Fize_Return_True()
 {
 //Act
 isFizz = _buzzChecker.IsBuzz(5);
 //Assert
 Assert.AreEqual(isFizz, true);
 }
 [TestMethod]
 public void When_Not_Devisible_By_Three_Return_False()
 {
 //Act
 isFizz = _buzzChecker.IsBuzz(1);
 //Assert
 Assert.AreEqual(isFizz, false);
 }
 [TestMethod]
 public void When_Devide_By_Zero_Return_True()
 {
 //Act
 isFizz = _buzzChecker.IsBuzz(0);
 //Assert
 Assert.AreEqual(isFizz, true);
 }
}

BuzzChecker

public interface IBuzzChecker
{
 bool IsBuzz(int number);
}
public class BuzzChecker : IBuzzChecker
{
 public bool IsBuzz(int number)
 {
 return (number % 5) == 0;
 }
}

I've written a test for FizzBuzzPrinter. Is it correct to mock the return results on IBuzzChecker and IFizzChecker instead of using concrete implementation? Since I think that I am testing FizzBuzzPrinter on this abstraction, not its dependency, so I mock all the dependency.

[TestClass]
public class FizzBuzzPrinter_Tests
{
 IFizzBuzzPrinter _fizzBuzzPrinter;
 Mock<IFizzChecker> _mockFizzChecker;
 Mock<IBuzzChecker> _mockBuzzChecker;
 [TestInitialize]
 public void Initialize()
 {
 _mockFizzChecker = new Mock<IFizzChecker>();
 _mockBuzzChecker = new Mock<IBuzzChecker>();
 _fizzBuzzPrinter = new FizzBuzzPrinter(_mockFizzChecker.Object, _mockBuzzChecker.Object);
 }
 [TestMethod]
 public void When_Number_Is_In_FizzCheckerConditionOnly_Return_Fizz()
 {
 //Arrange
 int number = 3;
 _mockFizzChecker.Setup(checker => checker.IsFizz(number)).Returns(true);
 _mockBuzzChecker.Setup(checker => checker.IsBuzz(number)).Returns(false);
 //Act
 string result = _fizzBuzzPrinter.Print(number);
 Assert.AreEqual("Fizz", result);
 }
 [TestMethod]
 public void When_Number_Is_In_BuzzCheckerConditionOnly_Return_Fizz()
 {
 //Arrange
 int number = 5;
 _mockFizzChecker.Setup(checker => checker.IsFizz(number)).Returns(false);
 _mockBuzzChecker.Setup(checker => checker.IsBuzz(number)).Returns(true);
 //Act
 string result = _fizzBuzzPrinter.Print(number);
 Assert.AreEqual("Buzz", result);
 }
 [TestMethod]
 public void When_Number_Is_In_Both_BuzzChecker_And_FizzChecker_Condition_Return_FizzBuzz()
 {
 //Arrange
 int number = 15;
 _mockFizzChecker.Setup(checker => checker.IsFizz(number)).Returns(true);
 _mockBuzzChecker.Setup(checker => checker.IsBuzz(number)).Returns(true);
 //Act
 string result = _fizzBuzzPrinter.Print(number);
 Assert.AreEqual("FizzBuzz", result);
 }
 [TestMethod]
 public void When_Number_Is_Not_In_Both_BuzzChecker_And_FizzChecker_Condition_Return_Original_Number()
 {
 //Arrange
 int number = 22;
 _mockFizzChecker.Setup(checker => checker.IsFizz(number)).Returns(false);
 _mockBuzzChecker.Setup(checker => checker.IsBuzz(number)).Returns(false);
 //Act
 string result = _fizzBuzzPrinter.Print(number);
 Assert.AreEqual("22", result);
 }
}

FizzBuzzPrinter

public interface IFizzBuzzPrinter
{
 string Print(int number);
}
public class FizzBuzzPrinter : IFizzBuzzPrinter
{
 private IFizzChecker fizzChecker;
 private IBuzzChecker buzzChecker;
 public FizzBuzzPrinter(IFizzChecker fizzChecker, IBuzzChecker buzzChecker)
 {
 this.fizzChecker = fizzChecker;
 this.buzzChecker = buzzChecker;
 }
 public FizzBuzzPrinter() : this (new FizzChecker(), new BuzzChecker())
 {
 }
 public string Print(int number)
 {
 if (fizzChecker.IsFizz(number) && buzzChecker.IsBuzz(number))
 {
 return "FizzBuzz";
 }
 if (fizzChecker.IsFizz(number))
 {
 return "Fizz";
 }
 if(buzzChecker.IsBuzz(number))
 {
 return "Buzz";
 }
 return number.ToString();
 }
}

Then in the main Program, I call it like this:

class Program
{
 static void Main(string[] args)
 {
 IFizzBuzzPrinter fizzBuzzPrinter = new FizzBuzzPrinter();
 for (int i = 1; i <= 100; i++)
 {
 Console.WriteLine(fizzBuzzPrinter.Print(i));
 }
 Console.ReadKey();
 }
}

Does this follow the proper mindset of SRP and unit-testing? Is there anything wrong that needs to be improved?

• Jesse C. Slicer: Related: github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition

Answer 1

I think you're overdoing it. SRP does not mean that every class should have only one method and every method should be a one-liner.

Instead, you should structure your code into parts that are likely to change together (though that's not the only criterion). But here, it's not really possible to guess that, because it's obvious these are not real requirements.

I think that FizzBuzz is a good problem to test that you know the basics of programming (logical way of thinking about problems) or to test that you know the basics of some programming language (ifs, functions). But because of how artificial the problem is, it's not very good for testing things like SRP or unit testing.

• Sarawut Positwinyu: Is there any good practice excercise for SRP ?
• Mathieu Guindon: @SarawutPositwinyu take a look at our weekend-challenge posts; Rock-Paper-Scissors-Lizard-Spock is a fun one.

Answer 2

I know this is an old post, but it got bumped and I noticed this oddness:

 public string Print(int number)
 {
 if (fizzChecker.IsFizz(number) && buzzChecker.IsBuzz(number))
 {
 return "FizzBuzz";
 }
 if (fizzChecker.IsFizz(number))
 {
 return "Fizz";
 }
 if(buzzChecker.IsBuzz(number))
 {
 return "Buzz";
 }
 return number.ToString();
 }
}

I would expect a function named "Print" to print something (Probably to standard output), not return a string. Considering you have an interface for the printer, and you're printing to the Console, I would say that FizzBuzzPrinter should output directly to console. That way you can avoid doing this.

Console.WriteLine(fizzBuzzPrinter.Print(i));

And replace it with:

fizzBuzzPrinter.Print(i);

Different implementations of IFizzBuzzPrinter could Print to different outputs as necessary.

@Jamal's idea in the comments about calling Print() ToString() instead is probably a better idea though. Like @BenAaronson pointed out, different implementations would require a lot of this duplicated logic.

• Jamal: So it should be named toString(), I suppose.
• RubberDuck: That's another good alternative @Jamal.
• Ben Aaronson: If different implementations of IFizzBuzzPrinter.Print varied by how they did their output, there'd be a lot of repeated logic between them
• Jamal: I've just realized something regarding my naming suggestion. Would that not work since C#'s functions are in PascalCase, and may clash with the system's ToString()?
• RubberDuck: I believe you would have to over ride it. ToString is a method of Object and all other objects implicitly inherit from Object. So, yeah. I'm pretty sure it works.