FizzBuzz without hard-coded logic checks

Question

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

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. — Gilles
– Gilles, Commented Mar 2, 2016 at 20:26
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. — Matt
– Matt, Commented Mar 2, 2016 at 20:28
Or what happens when the customer wants to change something other than what you planned for? — Johan Larsson
– Johan Larsson, Commented Mar 2, 2016 at 20:35
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. — Matt
– Matt, Commented Mar 2, 2016 at 20:38

Community · Accepted Answer · 2017-05-23 12:40:57Z

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; } }

Overloads to ToString with parameters aren't that unusual: msdn.microsoft.com/en-us/library/zdtaw1bw%28v=vs.110%29.aspx — Ben Aaronson
– Ben Aaronson, Commented Mar 2, 2016 at 22:09
@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. — Alexei
– Alexei, Commented Mar 3, 2016 at 6:23
Yeah, I agree, I don't think calling it ToString makes sense here — Ben Aaronson
– Ben Aaronson, Commented Mar 3, 2016 at 9:39
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(); ? — Matt
– Matt, Commented Mar 3, 2016 at 13:15
@Matt if you use a StringBuilder you can get its length, before you eventually return it as a String — holroy
– holroy, Commented Mar 3, 2016 at 13:41

Stack Exchange Network

FizzBuzz without hard-coded logic checks

1 Answer 1

You must log in to answer this question.

Hot Network Questions

FizzBuzz without hard-coded logic checks

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions