10
\$\begingroup\$

I wrote FizzBuzz. My aim is to use C# Code Standards correctly, and write flexible code. How can I improve my solution according to code standards? How can it be better in general?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace WindowsFormsApplication2
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private IEnumerable<int> Sequence(int startPoint, int endPoint)
        {
            return
                Enumerable
                    .Range(Math.Min(startPoint, endPoint), Math.Abs(startPoint - endPoint) + 1);
        }

        private bool IsDivisible(int number, int divisor)
        {
            return (number % divisor).Equals(0);
        }

        private static SortedDictionary<int, string> GetRules()
        {
            var rules = new SortedDictionary<int, string>
            {
                [3] = "Fizz",
                [5] = "Buzz"
            };

            return rules;
        }

        private bool IsInRules(int number)
        {
            return
                GetRules()
                    .Any(rule => IsDivisible(number, rule.Key));
        }

        private string FizzBuzz(int number)
        {
            if (IsInRules(number))
            {
                var result =
                    GetRules()
                        .Where(rule => IsDivisible(number, rule.Key))
                        .Aggregate(new StringBuilder(), (stringBuilder, rule) => stringBuilder.Append(rule.Value));

                return Convert.ToString(result);
            }

            return Convert.ToString(number);
        }

        private void button1_Click(object sender, EventArgs e)
        {
            var result =
                Sequence(1, 20)
                    .Select(FizzBuzz);

            listBox1.DataSource = result.ToArray();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This has to be the most complicated way to do a fizzbuzz I've ever seen. Though, it's pretty cool \$\endgroup\$ – IEatBagels Sep 9 '15 at 13:05
  • 1
    \$\begingroup\$ @TopinFrassi more complicated than this? \$\endgroup\$ – nhgrif Sep 10 '15 at 1:30
  • \$\begingroup\$ @nhgrif If I understood Swift, I could answer this question :p \$\endgroup\$ – IEatBagels Sep 10 '15 at 14:39
10
\$\begingroup\$

This is how you end up with projects with forms with 25,000 lines of code. It's also how you end up with duplicated code.

Your (poorly named) Form1 class should only be responsible for stuff directly related to the form it's displaying. Otherwise, complicated forms end up with even more complicated code.

And what happens if you have another form that wants to do some of the same FizzBuzz things? Well, now you have to copy & paste all of your code. As soon as you start to copy & paste anything while programming, that should be a massive red flag that you're absolutely doing something wrong. You need to take a step back and figure out how to do it without copy & pasting.

In this case, we need a separate class to hold all of our FizzBuzzing logic. Now, any form or anything else, that wants to use our FizzBuzz code instantiates our FizzBuzz class and uses its method (although perhaps, we've implemented as a static class and we just call its methods without instantiation).

\$\endgroup\$
7
\$\begingroup\$

As far as lines of code goes, this is pretty succinct, in fact as far as FizzBuzz in WinForms goes this is about as minimal as you can go.

Code Comments

Definitely need comments, yes in many cases your logic is self documenting, but it is hard to review code for bugs if we assume that the current logic and behaviour is by-design. It is likewise hard to reuse code if we have to keep inspecting the code to analyse if a function really is going to do what we need it to do. Make sure the summary comments viewed in IntelliSense are direct, use remarks to add additional information that we don't need to know every time the tooltips are displayed.

Separate Concerns - Keep Business Logic out of UI

To be flexible, separate your FizzBuzz logic out of the code-behind for your form. I know that was the easy way to display the app in this forum, and we forgive you for taking that shortcut for our benefit ^-^ but in code review we can't be lazy about these things, we don't want readers to learn these bad habits.

With FizzBuzz as it's own class we could easily re-use it in all sorts of different scenarios without having to copy and paste the code around. The overall benefit to code reuse is that there is less logic to test, review and importantly, maintain.

Collate Useful Functions in Static Classes

If you have complete methods that perform very specific functions and do not reference any member level resources from outside that method, then you should mark them as static and collate them in utility classes so that they can be easily located and re-used.

    /// <summary>
    /// Collection of utility functions that might be useful for re-use
    /// </summary>
    public static class Utilities
    {
        /// <summary>
        /// Generates a sequence of values including the start and end points
        /// <para>Note: supports both increasing and decreasing ranges</para>
        /// </summary>
        /// <param name="startPoint">First value in the sequence</param>
        /// <param name="endPoint">Last value in the sequence</param>
        public static IEnumerable<int> Sequence(int startPoint, int endPoint)
        {
            return Enumerable.Range(Math.Min(startPoint, endPoint), Math.Abs(startPoint - endPoint) + 1);
        }

        /// <summary>
        /// Tests that an integer value can be divided evenly by the divisor
        /// </summary>
        /// <param name="number">Value to test</param>
        /// <param name="divisor">Number of 'groups' to divide the Value into</param>
        /// <returns>True if the number can be divided evenly by the divisor</returns>
        public static bool IsDivisible(int number, int divisor)
        {
            return (number % divisor).Equals(0);
        }
    }

Re-use Objects instead of re-creating them

If you have a method like GetRules() that always returns the same collection, then it should be provided in a singleton pattern, or at the very least declared once as a member level collection. The next evolution of FizzBuzz from your current start is to do it for large sequences, which will could lead to parallel processing. It's an obvious evolution, to prepare for this some effort should be made to ensure that the rules collection really is immutable and if possible thread-safe.

Creating the array object for every test is simpler than thread-safety, but it is an expensive and lazy habit to get into especially if you try this with larger arrays.

Try something like:

    /// <summary>
    /// Thread-safe collection of FizzBuzz rules 
    /// </summary>
    /// <remarks>
    /// <list type="#">
    /// <listheader>Fizz Buzz Rules</listheader>
    /// <item>If a number is divisible by 3, then we should output 'Fizz'</item>
    /// <item>If a number is divisible by 5, then we should output 'Buzz'</item>
    /// <item>If a number is divisible by Both 3 and 5, then we should output 'FizzBuzz'</item>
    /// </list>
    /// <para>You could extend this concept by adding additional rules to just Fizz and Buzz </para>
    /// <para>This collection is very specific to FizzBuzz but is public to allow for additional use cases byt the caller</para>
    /// </remarks>
    public static readonly ConcurrentDictionary<int, string> Rules = new ConcurrentDictionary<int, string>
    {
        [3] = "Fizz",
        [5] = "Buzz",
    };

Consider Simple over Fancy or Uber efficient

Implementing a Linq Aggregate using StringBuilder is a pretty ambiguous, and in this manner it could be ever so slightly more inefficient than simply using String.Join

  • Unless you have serious performance issues go with succinct LINQ expressions keep the logic managable.
  • A serious code junky will admire your mastery of LINQ and Lambdas but if you are charging dollars per hour spent coding and debugging try not to be too abstract if a simpler option is available

    So your aggregate:

    StringBuilder result = Rules.Where(rule => IsDivisible(number, rule.Key))
                .Aggregate(new StringBuilder(), (stringBuilder, rule) => stringBuilder.Append(rule.Value));
        return Convert.ToString(result);
    

    easier to read and arguably more efficient:

        return string.Join("", Rules
            .Where(rule => IsDivisible(number, rule.Key))
            .Select(rule => rule.Value));
    

Here's the argument: String.Join vs. StringBuilder: which is faster?

Redundant Logic Loops

I would normally question the pattern of checking if the number is in the rules and then if it is re-iterating the rules to build the string. For this implementation I've decided I like it, but in general we should not re-iterate lists multiple times if we don't need to.

I like the implementation here because it does make the code more readable, your intentions are very clear. "I am going to get my number returned if it is not in the rules, otherwise ...magic will happen" ^-^

  • The other concession that I give is that the second iteration will only happen a very small fraction of the times that the first iteration is completed and the size of our rules list is very small so there is very little benefit.

    This is the best alternative that I could come up with:

        var output = string.Join("", Rules
            .Where(rule => Utilities.IsDivisible(number, rule.Key))
            .Select(rule => rule.Value));
        return output.Length > 0 ? output : number.ToString();
    
\$\endgroup\$
3
\$\begingroup\$

I'm not entirely sure that I like this. I just don't see a benefit.

        private IEnumerable<int> Sequence(int startPoint, int endPoint)

        {
            return
                Enumerable
                    .Range(Math.Min(startPoint, endPoint), Math.Abs(startPoint - endPoint) + 1);

        }

I get it. This allows you to get a viable range even if called "backwards" like this.

var seq = Sequence(10,2);

This returns the range of numbers [2..10]. Which is fine, I guess, but it is surprising and as such deserves a doc comment describing what it does. It took me a minute to parse it, which was time that could have been saved with a good comment. Like I said, I don't like it. It should return an empty set, but at that point, just use Enumerable.Range. If you decide to keep it, I'd move it to a static class of it's own. It doesn't belong in the code behind, nor does it logically group into a FizzBuzz class.

\$\endgroup\$
  • 2
    \$\begingroup\$ There's a good reason why libraries take a location & length rather than a start & end index. \$\endgroup\$ – nhgrif Sep 10 '15 at 1:31

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.