Is my implementation of FizzBuzz overkill?

Question

I am doing FizzBuzz on a take home test (in Unity). I am sure this is just a competency check, but I went ahead and followed some coding conventions such as using constant fields, dependency injection, comments, etc. Is this overkill? If I should "dumb it down", how should I do so? Also, as seen in the directions, it implies that for multiples of 3 and 5, I should print out "Fizz", "Buzz", and "FizzBuzz"...but I am reading too much into it, right?

I am trying to have an optimal program here in terms of production code. I hope I am not being too picky and misusing Code Review.

///******************************************
// * FizzBuzz.cs
// *
// * INSTRUCTIONS:
// * Write a program that prints the numbers 1 to 100.
// * For multiples of 3, output the string "Fizz" instead of the number.
// * For multiples of 5, output the string "Buzz" instead of the number.
// * For multiples of 3 & 5, output "FizzBuzz" instead of the number.
// ******************************************/
using UnityEngine;
using System.Collections;
public class FizzBuzz : MonoBehaviour
{
 private const string FIZZ = "Fizz";
 private const string BUZZ = "Buzz";
 private const string FIZZ_BUZZ = "FizzBuzz";
 void Start()
 {
 PrintFizzBuzz(100);
 }
 /// <summary>
 /// Prints the numbers [1, n], unless it is a multiple of 3 or 5.
 /// If a multiple of 3, prints FIZZ.
 /// if a multiple of 5, prints BUZZ.
 /// if a multiple of 3 and 5, prints FIZZ_BUZZ.
 /// </summary>
 /// <param name="n">The maximum number [1, infinity] to print to</param>
 private void PrintFizzBuzz(int n)
 {
 if (n < 1)
 {
 Debug.Log("int n in fizzBuzz.PrintFizzBuzz(int) must be >= 1");
 return;
 }
 for (int i = 1; i <= n; i++)
 {
 if (i % (3 * 5) == 0)
 {
 Debug.Log(FIZZ_BUZZ);
 }
 else if (i % 3 == 0)
 {
 Debug.Log(FIZZ);
 }
 else if (i % 5 == 0)
 {
 Debug.Log(BUZZ);
 }
 else
 {
 Debug.Log(i);
 }
 }
 }
}

• Jeroen Vannevel: Can you clarify where you see dependency injection used? As for your question: a multiple of 3 and 5 only prints "FizzBuzz". Not "Fizz" and "Buzz" separately as well.
• Evorlor: @JeroenVannevel Dependency Injection (unless I misunderstand the concept, which is possible), is used where I call it from Start passing in the max opposed to hard coding it in the Start method. I realize it doesn't print "Fizz" and "Buzz" as well, but this is technically against the instructions. I wanted to make sure I was reading too much into it, and they don't expect it to print all 3.

Answer 1

This doesn't belong in a MonoBehaviour. It has no need of any of MonoBehaviour's logic, and in fact uses no Unity-specific logic at all, with the exception of Debug.Log, and this can be separated out as RobH mentions. As such, keep it as a separate class in your model. This will allow you to re-use it across multiple behaviours or even outside your Unity project.

public class FizzBuzzBehaviour : MonoBehaviour
{
 [SerializeField]
 private int count = 100;
 void Start()
 {
 var fizzBuzz = new FizzBuzz();
 fizzBuzz.Print(count, x=> Debug.Log(x));
 }
}
public class FizzBuzz
{
 private const string FIZZ = "Fizz";
 private const string BUZZ = "Buzz";
 private const string FIZZ_BUZZ = "FizzBuzz";
 /// <summary>
 /// Prints the numbers [1, n], unless it is a multiple of 3 or 5.
 /// If a multiple of 3, prints FIZZ.
 /// if a multiple of 5, prints BUZZ.
 /// if a multiple of 3 and 5, prints FIZZ_BUZZ.
 /// </summary>
 /// <param name="n">The maximum number [1, infinity] to print to</param>
 public void Print(int n, Action<string> printAction)
 {
 if (n < 1)
 {
 printAction("int n in fizzBuzz.PrintFizzBuzz(int) must be >= 1");
 return;
 }
 for (int i = 1; i <= n; i++)
 {
 if (i % (3 * 5) == 0)
 {
 printAction(FIZZ_BUZZ);
 }
 else if (i % 3 == 0)
 {
 printAction(FIZZ);
 }
 else if (i % 5 == 0)
 {
 printAction(BUZZ);
 }
 else
 {
 printAction(i);
 }
 }
 }
}

My use of SerializeField ensures the count field shows up in the Unity inspector but cannot be edited from other code. This is because I prefer my behaviours to be monolithic and instead allow the models to communicate with each other. Depending on your use case, your mileage may vary.

Answer 2

You have misunderstood what dependency injection is - what you have done is just use a parameter to set the length of the sequence which is great from a flexibility point of view.

Dependency injection at its simplest level is a class having its dependencies injected into it by something higher up. So, you're writing the fizzbuzz out to Debug - that's a dependency but your class uses it directly. It would be better to have that injected in. In a non-unity world:

// using System;
public class SomeSampleClassThatOutputs
{ 
 Action<string> _output = s => { };
 public SomeSampleClassThatOutputs(Action<string> output)
 {
 _output = output;
 }
 public void PrintSomething()
 {
 _output("Something");
 }
}

As you can see, the dependency (where to output) is being injected into the class. I used a Func<string> for simplicity but you could easily use a Stream or something.

The caller is now responsible for where the class puts its output:

// Print to Console
var someClass = new SomeSampleClassThatOutputs(Console.WriteLine);
someClass.PrintSomething();

That's the essence of it. You could also use property injection but I don't know enough about Unity to advise on the best way of doing it in Unity.

There's a lot of examples of FizzBuzz around (especially here) so I won't really go into detail about that. Note however, that you have factored out the strings to constants (Fizz, Buzz, FizzBuzz) but you haven't done the same for the numbers (3, 5, 15). You'll notice that they are actually paired together - might want to think about suitable structure for storing that...

C# constants are generally named UpperPascalCase not ALL_CAPS_SNAKE_CASE but I don't know whether Unity follows a different convention.

Anyway, FizzBuzz is generally just a test of whether you can write even the most basic of programs - if you have a solution that works you'll probably get through.

Edit

Just thought I'd nitpick - your comment [1, infinity] is incorrect - it's actually [1, int.MaxValue]