6
\$\begingroup\$

To support a demonstration I'm going to be doing, I need a short C# program that can be unit tested. I decided to make a FizzBuzz split into a few methods:

using System;
namespace FizzBuzz
{
    class FizzBuzzer
    {
        public static void Main()
        {
            FizzBuzzer fizzBuzzer = new FizzBuzzer();
            int n = Convert.ToInt32(Console.ReadLine());
            fizzBuzzer.StartLoop(n);
        }

        public int StartLoop(int n)
        {
            int fizzBuzzes = 0;
            for(int i = 0; i < n; i++)
            {
                string output = getOutputForI(i);
                if(output == "FizzBuzz") {
                    fizzBuzzes++;
                }
                Console.WriteLine(output);
            }
            return fizzBuzzes;
        }

        public string getOutputForI(int i)
        {
            string s = "";
            if(i % 3 == 0)
            {
                s += "Fizz";
            }
            if(i % 5 == 0)
            {
                s += "Buzz";
            }
            if(s.Length == 0)
            {
                s = "" + i;
            }
            return s;
        }
    }
}

I want people to focus on the point of my demonstration, and not on potential mistakes in my code. As someone new to C#, I can't evaluate my code very well. Are there any beginner mistakes that would distract pedantic C# experts?

I am of course open to any type feedback, I mainly just want people to not laugh at or get annoyed by my code.


Edit:

here are example unit tests

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace FizzBuzz.Tests
{
    [TestClass()]
    public class FizzBuzzerTests
    {
        [TestMethod()]
        public void StartLoopTest()
        {
            Assert.AreEqual(6, FizzBuzzer.StartLoop(100));
        }

        [TestMethod()]
        public void GetOutputFor0()
        {
            Assert.AreEqual("FizzBuzz", FizzBuzzer.getOutputForI(0));
        }

        [TestMethod()]
        public void GetOutputFor4()
        {
            Assert.AreEqual("4", FizzBuzzer.getOutputForI(4));
        }
    }
}

and for context, here is the updated code (based on Ron Beyer's answer) that those tests are run against:

using System;
namespace FizzBuzz
{
    public static class FizzBuzzer
    {
        public static void Main() { }

        public static int CountFizzBuzzes(int max)
        {
            int fizzBuzzes = 0;
            for (int i = 1; i <= max; i++)
            {
                string result = FizzBuzz(i);
                if (result == "FizzBuzz")
                {
                    fizzBuzzes++;
                }

                Console.WriteLine(result);
            }
            return fizzBuzzes;
        }

        public static string FizzBuzz(int input)
        {
            string s = "";
            if (input % 3 == 0)
            {
                s += "Fizz";
            }
            if (input % 5 == 0)
            {
                s += "Buzz";
            }
            if (s.Length == 0)
            {
                s = "" + input;
            }
            return s;
        }
    }
}
\$\endgroup\$
7
  • 6
    \$\begingroup\$ Since your focus is on unit-testing then I think you should provide your tests too. \$\endgroup\$
    – t3chb0t
    Commented Jan 30, 2019 at 16:34
  • \$\begingroup\$ And about mistakes that can distract your audience... we don't give classes the same name as the enclosing namespace. \$\endgroup\$
    – t3chb0t
    Commented Jan 30, 2019 at 16:35
  • 1
    \$\begingroup\$ Well, nobody knows whether some code can be tested until you actually try to write tests ;-] This is a very deceiving feeling so I'd be really good if you wrote tests first. \$\endgroup\$
    – t3chb0t
    Commented Jan 30, 2019 at 16:38
  • 2
    \$\begingroup\$ I can already tell that this is not testable because of your console output... so... \$\endgroup\$
    – t3chb0t
    Commented Jan 30, 2019 at 16:38
  • 1
    \$\begingroup\$ @t3chb0t Understood. I'll see if I can prototype some unit tests soon. \$\endgroup\$ Commented Jan 30, 2019 at 16:44

1 Answer 1

11
\$\begingroup\$

Since it's a code-review site, I'll focus on issues you have with your code...

Having Main in your class

I would generally avoid this. I understand that this is probably a single-class demonstration project, but typically C# projects have a Program class that has Main inside of it, and I would stick with that. If you want to be able to unit-test FizzBuzzer, the Main function would not be unit-testable. As a result, your code-coverage of this class will be lower.

Inconsistent method names

You have a method properly cased as StartLoop and then you have an improper cased getOutputForI. Stick with the Microsoft recommendation of Capitalization Rules for Methods which should be Pascal Cased.

Method names and parameters that don't self-document

The name you give a method should identify to the caller what it does. StartLoop both starts and ends a loop, and runs the entire sequence. Ideally this would be named Run instead of StartLoop, but even then, what is it returning? Maybe CountFizzBuzz would be even better. Similarly, getOutputForI certainly gets the output for the method parameter I, but that really doesn't say what it does. Maybe consider renaming it to FizzBuzz(int input) instead.

Similarly the method parameters are not self documenting. What is n exactly in StartLoop? It would be more apparent if the method were something like Run(int max). Which would also require a modification of your for loop because you would stop just shy of max. i is also not apparent of what it is (see a suggested version in the paragraph above).

Mixing UI with code

This makes your code unit-test unfriendly. You can't unit test the output to the console, so in order to unit test this you would need to pass a stream to the output:

public int CountFizzBuzz(int max, TextWriter output)
{
    int fizzBuzzes = 0;
    for (int i = 0; i <=max; i++)
    {
        string fizzBuzz = FizzBuzz(i);
        if (fizzBuzz == "FizzBuzz")
        {
            fizzBuzzes++;
        }
        output.WriteLine(fizzBuzz);
    }
    return fizzBuzzes;
}

Now this is unit-testable because you can create a TextWriter and pass in a value that can be Asserted later.

Inconsistent Brackets

You have two methods of writing brackets in your code, the "Microsoft" way, and the "Java" way. Again, I would stick with Microsoft practices here and use the one that has both brackets on their own lines.

Using an instance object for something that has no state

FizzBuzzer has no state, so why isn't everything static? There isn't a good reason to require a new each time you have to run the "Fizz Buzzer", take up space on the stack, etc. This class should be made static.


So with all my recommendations, here is what it would possibly turn out like:

public static class FizzBuzzer
{
    public static int CountFizzBuzzes(int max)
    {
        return CountFizzBuzzes(max, Console.Out);
    }

    public static int CountFizzBuzzes(int max, TextWriter output)
    {
        int fizzBuzzes = 0;
        for (int i = 0; i <= max; i++)
        {
            string result = FizzBuzz(i);

            if (result == "FizzBuzz")
                fizzBuzzes++;

            output.WriteLine(result);
        }
        return fizzBuzzes;
    }

    public static string FizzBuzz(int input)
    {
        string s = "";

        if (input % 3 == 0)
            s += "Fizz";

        if (input % 5 == 0)
            s += "Buzz";

        if (s.Length == 0)
            s = "" + input;

        return s;
    }
}

I still don't find this completely "ideal" as CountFizzBuzzes really does two things, counts them and outputs them to the stream. This really should be two separate methods for separation of concerns.

Just as a final note, your Main has an issue... Right now when you start the program it just stares at you with a blank window until you mash the right key(s).

public static void Main()
{
    FizzBuzzer fizzBuzzer = new FizzBuzzer();
    int n = Convert.ToInt32(Console.ReadLine());
    fizzBuzzer.StartLoop(n);
}

What is to stop the user here from entering "My Name Is" instead of "42"? The Convert.ToInt32 will throw an exception and the program will terminate. The user isn't even prompted for a number or any thing. At least prompt the user:

There are a multitude of questions on here about how to validate console input, so I'll leave that to you to research, but you should prompt the user "Please Enter a Number:" and if that isn't a number, tell them and ask again (and provide them a way to quit).

\$\endgroup\$
5
  • 3
    \$\begingroup\$ I wouldn't necessarily call not-using {} as cleaned. \$\endgroup\$
    – t3chb0t
    Commented Jan 30, 2019 at 18:44
  • \$\begingroup\$ @t3chb0t Maybe a poor choice of words, revised \$\endgroup\$
    – Ron Beyer
    Commented Jan 30, 2019 at 19:05
  • \$\begingroup\$ Upvoted, very good answer - I would only point out that many pedantic C# experts you say making the conditional block scopes implicit isn't improving the code's maintainability. \$\endgroup\$ Commented Jan 31, 2019 at 13:09
  • \$\begingroup\$ @MathieuGuindon I'd be one of them! All good advice, except the bit about skipping {} for single-line ifs, with which I disagree vehemently. \$\endgroup\$ Commented Feb 1, 2019 at 16:34
  • \$\begingroup\$ @BittermanAndy FWIW I'm one of them as well. Rubberduck pull requests without these braces inevitably get the review comments and change suggestions accordingly ;-) \$\endgroup\$ Commented Feb 1, 2019 at 16:36

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