patterncsharpMinor
Is my implementation of FizzBuzz overkill?
Viewed 0 times
implementationoverkillfizzbuzz
Problem
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);
}
///
/// 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.
///
/// The maximum number [1, infinity] to print to
private void PrintFizzBuzz(int n)
{
if (n = 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
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);
}
///
/// 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.
///
/// The maximum number [1, infinity] to print to
private void PrintFizzBuzz(int n)
{
if (n = 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
Solution
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:
As you can see, the dependency (where to output) is being injected into the class. I used a
The caller is now responsible for where the class puts its output:
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
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 _output = s => { };
public SomeSampleClassThatOutputs(Action 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 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]Code Snippets
// using System;
public class SomeSampleClassThatOutputs
{
Action<string> _output = s => { };
public SomeSampleClassThatOutputs(Action<string> output)
{
_output = output;
}
public void PrintSomething()
{
_output("Something");
}
}// Print to Console
var someClass = new SomeSampleClassThatOutputs(Console.WriteLine);
someClass.PrintSomething();Context
StackExchange Code Review Q#92836, answer score: 8
Revisions (0)
No revisions yet.