4
\$\begingroup\$

I'm a self-taught (on the job) programmer, always looking for ways to expand my skills. Seeing all the FizzBuzz examples with hard-coded logic checks made my head hurt, so I thought I'd try my hand at it. Leaning heavily on KISS, I'm ignoring exception handling and such, and all that's required to add new fizzbuzzers is a call to Catalog.Add(Divisor,Output).

I'd appreciate your feedback, whether on style or substance. This is part of my journey to adopting SOLID principles for better OO development, but I felt that not much could be applied here without over-complicating it.

using System;
using System.Collections.Generic;

public class Program
{
    public static void Main()
    {
        var catalog = new Catalog();
        catalog.Add(Divisor: 3, Output: "Fizz");
        catalog.Add(Divisor: 5, Output: "Buzz");
        catalog.Add(Divisor: 10, Output: "Pozz");

        var counter = new Counter(Min: 1, Max: 100, Catalog: catalog);
        counter.Output();
        Console.ReadLine();
    }
}

internal class Counter
{
    private int min;
    private int max;
    private Catalog catalog;

    internal Counter(int Min, int Max, Catalog Catalog)
    {
        this.min = Min;
        this.max = Max;
        this.catalog = Catalog;
    }
    internal void Output()
    {
        for(int i = min; i <= max; i++)
        {
            Console.WriteLine(catalog.ToString(i));
        }
    }
}

internal class Catalog
{
    private List<Spec> specs;
    internal class Spec
    {
        internal int divisor;
        internal string output;
    }
    internal Catalog()
    {
        this.specs = new List<Spec>();
    }
    internal void Add(int Divisor, string Output)
    {
        this.specs.Add(new Spec() { divisor = Divisor, output = Output });
    }
    internal string ToString(int Number)
    {
        string outputstring = "";
        foreach (var x in specs)
        {
            outputstring += Number % x.divisor == 0 ? x.output : "";
        }
        return String.IsNullOrWhiteSpace(outputstring) ? Number.ToString() : outputstring;
    }
}

.NetFiddle

\$\endgroup\$
  • 3
    \$\begingroup\$ Personally I think all the FizzBuzz examples with hard-coded logic checks are much more KISS than your example, which is fine as a practice for SOLID but over-engineering otherwise. See : github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition for a truly over-engineered FizzBuzz. \$\endgroup\$ – Gilles Mar 2 '16 at 20:26
  • 1
    \$\begingroup\$ Maybe, but seeing them code for 3 and 5, and then for 15... What happens when you modify the program for more than 2 numbers? That would get unmanageable quickly. \$\endgroup\$ – Matt Mar 2 '16 at 20:28
  • 2
    \$\begingroup\$ What happens if you don't change the program? \$\endgroup\$ – Gilles Mar 2 '16 at 20:28
  • \$\begingroup\$ Or what happens when the customer wants to change something other than what you planned for? \$\endgroup\$ – Johan Larsson Mar 2 '16 at 20:35
  • 2
    \$\begingroup\$ There should be only 2 factors to the FizzBuzz spec: The counting range and the substitution logic. If they want to change the counting range (count by 2, exponential, random) then refactor the Counter class. If they want to change the substitution logic (include number on all lines, text formatting) then refactor the Catalog class. \$\endgroup\$ – Matt Mar 2 '16 at 20:38
3
\$\begingroup\$

The code looks well structured and seems to do its job (I have just copy-pasted it in VS2015 and worked directly), so many of the above are rather superficial:

1) Function (including constructor) parameter naming. In C#, usually all function parameters are named using CamelCase.

internal Counter(int min, int max, Catalog catalog)
{
    this.min = min;
    this.max = max;
    this.catalog = catalog;
}

2) Non-private members naming. Here, I would also favor PascalCase over CamelCase, although this targeted question and answers do not seem to clarify this.

internal class Spec
{
    internal int Divisor;
    internal string Output;
}

3) ToString usage - ToString(int Number) function has exactly the same name as Object.ToString() function (no parameters). Although it is perfectly legal (becomes an overload), I would choose a different name, just not to create any confusion (your are not getting a string representation of the object using its state only, but also providing some input).

Also, since strings are immutable, concatenation is faster and allocates less memory if StringBuilder is used.

internal string GetString(int Number)
{
    var builder = new StringBuilder();
    foreach (var x in specs)
    {
        builder.Append(Number % x.Divisor == 0 ? x.Output : "");
    }
    string outputString = builder.ToString();

    return String.IsNullOrWhiteSpace(outputString) ? Number.ToString() : outputString;
}

}

The whole code:

using System;
using System.Collections.Generic;
using System.Text;

public class Program
{
    public static void Main()
    {
        var catalog = new Catalog();
        catalog.Add(divisor: 3, Output: "Fizz");
        catalog.Add(divisor: 5, Output: "Buzz");
        catalog.Add(divisor: 10, Output: "Pozz");

        var counter = new Counter(Min: 1, Max: 100, Catalog: catalog);
        counter.Output();
        Console.ReadLine();
    }
}

internal class Counter
{
    private int min;
    private int max;
    private Catalog catalog;

    internal Counter(int min, int max, Catalog catalog)
    {
        this.min = min;
        this.max = max;
        this.catalog = catalog;
    }

    internal void Output()
    {
        for (int i = min; i <= max; i++)
        {
            Console.WriteLine(catalog.GetString(i));
        }
    }
}

internal class Catalog
{
    private List<Spec> specs;

    internal class Spec
    {
        internal int Divisor;
        internal string Output;
    }

    internal Catalog()
    {
        specs = new List<Spec>();
    }

    internal void Add(int divisor, string Output)
    {
        specs.Add(new Spec() { Divisor = divisor, Output = Output });
    }

    internal string GetString(int Number)
    {
        var builder = new StringBuilder();
        foreach (var x in specs)
        {
            builder.Append(Number % x.Divisor == 0 ? x.Output : "");
        }
        string outputString = builder.ToString();

        return String.IsNullOrWhiteSpace(outputString) ? Number.ToString() : outputString;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Overloads to ToString with parameters aren't that unusual: msdn.microsoft.com/en-us/library/zdtaw1bw%28v=vs.110%29.aspx \$\endgroup\$ – Ben Aaronson Mar 2 '16 at 22:09
  • 2
    \$\begingroup\$ @BenAaronson - yes, that is correct. However, a subtle difference exists: ToString(string format) uses only internal state data, format being used to specify how the output is obtained; in our case, ToString(...) takes a parameter that is part of the logic, not presentation. Anyway, it is matter of taste and it can fall into pedantry :). Thanks. \$\endgroup\$ – Alexei Mar 3 '16 at 6:23
  • \$\begingroup\$ Yeah, I agree, I don't think calling it ToString makes sense here \$\endgroup\$ – Ben Aaronson Mar 3 '16 at 9:39
  • \$\begingroup\$ 1) Naming conventions can probably devolve into Pedantry as well: ask a dozen people, get a dozen answers. 2) I always forget about the existence of StringBuilder. 3) Any way to refactor GetString() to do the ToString() conversion in the return? e.g.: return builder.[IsEmpty] ? Number.ToString() : builder.ToString(); ? \$\endgroup\$ – Matt Mar 3 '16 at 13:15
  • 1
    \$\begingroup\$ @Matt if you use a StringBuilder you can get its length, before you eventually return it as a String \$\endgroup\$ – holroy Mar 3 '16 at 13:41

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.