6
\$\begingroup\$

First of all, I came up with that question on SO already. But I am offered different solution which are "better" in some ways. The goal is to improve this specific version, not to create another one.

Here is what it is about:

Reading Coding Horror, I just came across the FizzBuzz another time. The original post is here.

I just started to code it down. It was a job of a minute, but there are several things that I do not like.

public void DoFizzBuzz()
{
    var combinations = new Tuple<int, string>[] 
    { 
        new Tuple<int, string> (3, "Fizz"), 
        new Tuple<int, string> (5, "Buzz"), 
    };

    for (int i = 1; i <= 100; i++)
    {
        bool found = false;

        foreach (var comb in combinations)
        {
            if (i % comb.Item1 == 0)
            {
                found = true;
                Console.Write(comb.Item2);
            }
        }

        if (!found)
        {
            Console.Write(i);
        }

        Console.Write(Environment.NewLine);
    }
}

So my questions are:

  1. How to get rid of the bool found?
  2. Is there a better way of testing than the foreach?
\$\endgroup\$
11
\$\begingroup\$

I think if you're going to set up a set of data to iterate over you should start thinking more functionally. Why not use an array of functions and aggregate over them instead? That puts the logic in the functions, not in the function that utilises the data.

For example:

static void Main(string[] args)
{
    var generators = new Func<string, int, string>[]
    {
        (s, i) => i % 3 == 0 ? s + "Fizz" : s,
        (s, i) => i % 5 == 0 ? s + "Buzz" : s,
        (s, i) => s ?? i.ToString()
    };
    var results = Enumerable.Range(0, 100).Select(i => generators.Aggregate((string)null, (s, f) => f(s, i))).ToList();
    results.ForEach(Console.WriteLine);
}
\$\endgroup\$
  • \$\begingroup\$ This is pretty cool, but if you had a lot of suffixes, then spelling var generators in code would take much more space than just typing out the data. Lisp has macros for that; perhaps one can utilize T4 transforms to generate the code for C#, though just about anything other than the most trivial code is an overkill for C#. \$\endgroup\$ – Leonid Aug 3 '12 at 16:33
  • 1
    \$\begingroup\$ The idea of using T4 for anything like this makes me nauseous. I can't see how typing those generates out is any worse than typing the data. The data alone doesn't define intent and is incapable of capturing the need to conditionally output the number if noting else was outputted. Instead, that logic has to live in the code that parses the data. Ultimately it doesn't matter too much. This is nothing but a mind exercise with a goal to learn. This is why I posted an alternative method in the first place. \$\endgroup\$ – OJ. Aug 4 '12 at 23:09
  • \$\begingroup\$ You're welcome. Glad to see you enjoyed it. I think that approaching problems like this with a more functional mindset (rather than imperative) leads you to more elegant solutions. They're often more concise too. Cheers! \$\endgroup\$ – OJ. Aug 31 '12 at 1:22
7
\$\begingroup\$

While I think making the code table driven is a reasonable idea, I think you're doing that (mostly) the wrong way.

If you're going to go beyond the obvious solution (e.g., @DanienMeSer's), you should accomplish something by doing so. The obvious problems (IMO) with the original code are that it's not very extensible (e.g., if you had 50 cases instead of three, you'd probably need to rewrite entirely), and it doesn't separate concerns very well (e.g., the logic and the I/O are quite tightly intermingled).

At least IMO, the key here is ensuring that each function (including main) has a clearly defined purpose and layer of abstraction at which it operates. If it delegates everything to lower layers, then it's not doing anything to justify its own existence. At the opposite extreme, doing everything in one layer/function means you're not actually using functions, classes, etc., to keep the code simple and manageable.

With that idea in mind, I'd start by thinking about what the ideal top-level code would look like, then write the lower layers to support that. In this case, it seems to me that it makes sense for the top layer to generate inputs, and write out the result for each:

for (int i=start; i<end; i++)
    console.WriteLine(fizzbuzz(i));

That's pretty simple, but it still accomplishes something useful: it generates inputs, writes outputs, and isolates logic from I/O. This is a decided contrast to a Main that just contains a single function call to doEverything(); (usually under some other name) that just forces the reader to navigate through more code before they find anything meaningful.

From there, we obviously need to define the fizzbuzz function to do the dirty work:

string fizzbuzz(int i) {
    // logic here
}

We have two choices for implementing that. One is mostly monolithic, but still make use of your table to produce the values:

string fizzbuzz(int i) {
    string ret;
    foreach (var comb in combinations) {
        if (i % comb.Item1 == 0)
            ret += comb.item2;
    }
    if (ret.Length() == 0)
        ret = i.toString();
    return ret;
}

Personally, I think I'd break that up into two pieces though: one that's a simple map from multiple of 3/5 to "fizz"/"buzz", and the other to provide a default value in case the first "fails":

string check_mult(int i) {
    string ret = new String();
    foreach (var comb in combinations) {
        if (i % comb.Item1 == 0)
            ret += comb.item2;
    }
    return ret;
}

...then the second, with the full fizzbuzz logic, which is now pretty trivial:

string fizzbuzz(int i) {
    string ret = check_mult(i);
    if (ret.length() == 0)
        ret = i.toString();
    return ret;
}

I'd note that these also give us some building blocks that at least at first blush look like they stand at least a little chance of being useful. One maps multiples of arbitrary numbers to arbitrary strings, and another attempts to map numbers to strings, with a default of mapping the input directly to a string if the first fails.

If you wanted to make this a bit more generic, you'd put just a bit more of the logic into the table, so the first element in the tuple was an object instead of just a value. Then instead of dealing only with multiples of specified values, you could deal with an arbitrary function of an input value. I'll leave that modification for somebody else to deal with though.

\$\endgroup\$
  • \$\begingroup\$ breaking this into multiple pieces is a very good starting point. Having a simple function to achieve what is wanted definitly makes the difference. \$\endgroup\$ – Mare Infinitus Aug 3 '12 at 5:41
  • \$\begingroup\$ In check_mult, ret needs to be initalized to string.Empty, otherwise you return a null...and then the ret.Length throws a null reference exxception. \$\endgroup\$ – jmoreno Aug 16 '14 at 5:25
  • \$\begingroup\$ @jmoreno: Oops, quite right. I should have remembered that Java makes you do everything manually, but I've gotten spoiled by years of C++, where such things happen automatically. \$\endgroup\$ – Jerry Coffin Aug 16 '14 at 6:50
3
\$\begingroup\$

*this post has been heavily modified since it was originally written since I was in a foul mode when I wrote the original post, if any comments seem weird or out of place it's most likely due to this editing*

You are clearly moving away from what I would call the "clean and simple" approach with your solution. From your description I understand it as if this is how you would solve the problem, given that someone asked you to do it for an interview.

I strongly believe that your solution is overcomplicated for this problem, and too many assumptions are made on your side on how the problem will evolve in the future. Doing stuff like this is in contradiction to the software principle called KISS (keep it simple stupid), and due to these "assumptions" you are making it harder to read, understand and change the code.

I will post the original/clean solution that I think is much better suited for this problem, and then I'll follow up with a short description of why I think the "clean" solution is better than yours.

Clean Solution:

for(int i=0; i <= 100; ++i) {
    string s = ""

    if(i%(5*3) == 0) {
        s = "FizzBuzz";
    } else if (i % 3 == 0) {
        s = "Fizz";
    } else if (i % 5 == 0) {
        s = "Buzz";
    } else {
        s = i.ToString();
    }

    Console.WriteLine(s)
}

In the clean solution, it is clear what is happening and what values we check for [even the 5*3-value], in your solution you hide away the only thing interesting in this code (the replacement of strings for numeric values).

You make it harder to grasp what is happening by placing the values and the strings into an unnecessary Tuple-object and hide these important values behind something called "comb.item1" and "comb.item2".

The only time your solution would make sense is if it's known that the next iteration would require 25 replacements in similar fashion (7="Foo", 11="Bar", 77="FooBar"). However, then you would still run into problems and have to handle the "multiple match"-issue in another way [f.e. 3*5*7].

The entire idea with programming is to keep the code as easy/clean as possible [good book: Martin Fowler - Clean Code] and avoid making "smart solutions" when it is not known what to expect next. Writing overly complicated solutions is something that would make warning bells go off for any recruiter.

Your solution makes it easy to add more replacements, but next iteration might as well be:

replace anything dividable with 9 with 'bazzinga' and then don't bother with fizz and buzz. Unless program is running on a Sunday, Sundays are as everyone knows 'Happy Sunday Sharing'-day, and string should then read 'FizzySundayBuzz' or 'FizzySunday'.

\$\endgroup\$
  • 4
    \$\begingroup\$ -1: aggressive language only makes you sound silly; if you have trouble understanding OP's simple-enough code then I think you are in no position to offer alternatives; I don't believe that someone capable of writing the question's code is incapable of writing what you wrote. \$\endgroup\$ – ANeves Aug 2 '12 at 9:16
  • 2
    \$\begingroup\$ KISS certainly has its place, but that's not the question here. Presumably, the OP wrote the code the way he did because he expected that more cases will be added in the future. \$\endgroup\$ – svick Aug 2 '12 at 9:27
  • 1
    \$\begingroup\$ ANeves: you are right, sorry for language. But I see these overly complicated solutions all the time. and nowhere in his description does it claim why he decides to do it in a complicated way. svick: and that's my point exactly, why make it harder at this point and assume that it will evolve in that particular manner? That assumption is what makes code hard to maintain. Next iteration is just as likely to state "when a multiple of 9 is found, you should only write 'bazzinga', unless it's also a Sunday, then you write 'godspeed' and the original rules regarding fizz and buzz applies. \$\endgroup\$ – Daniel MesSer Aug 2 '12 at 10:23
  • 2
    \$\begingroup\$ Agreed that I'd adopt the simpler technique such as applied here. At the same time, I think it a useful exercise to write a version that uses a data list like that. \$\endgroup\$ – Winston Ewert Aug 2 '12 at 15:11
  • 2
    \$\begingroup\$ This is obviously a learning exercise for the OP (and the rest of us, including you). Rather than coming across as offensive, why not add to the discussion in a positive way? Your solution could also use some work. \$\endgroup\$ – OJ. Aug 3 '12 at 10:44
3
\$\begingroup\$

First of all I think Daniel MesSer have made some strong points although if you are going for something simpler and more readable I think this approach is even more easy to read:

for(int i = 1; i <= 100; i++) 
{
    string output = null;

    if (i % 3 == 0)
    {
        output += "Fizz";
    }

    if (i % 5 == 0)
    {
        output += "Buzz";
    }

    if (output == null)
    {
        output = i;
    }

    Console.WriteLine(output);
}

Although it is possible to replace the last if with the ?? operator, this is the most readable version, in my opinion.

If there are compelling reasons to introduce a table we could use more or less the same solution. The advantage with this solution is that I've gotten rid of the bool but instead I use a variable that holds the printout for each number. Another benefit of this solution is that I only need to have one call to Console.WriteLine instead of multiple calls to Console.Write / WriteLine embedded in the logic.

var combinations = new Tuple<int, string>[] 
{ 
    new Tuple<int, string> (3, "Fizz"), 
    new Tuple<int, string> (5, "Buzz"), 
};

for (int i = 1; i <= 100; i++)
{
    string output = null;

    foreach (var comb in combinations)
    {
        if (i % comb.Item1 == 0)
        {
            output += comb.Item2;
        }
    }

    if (output == null)
    {
        output = i.ToString();
    }

    Console.WriteLine(output);
}

You could get rid of the foreach and replace it with a LINQ expression but i doubt that the majority would find it easier to read.

var combinations = new Tuple<int, string>[] 
{ 
    new Tuple<int, string> (3, "Fizz"), 
    new Tuple<int, string> (5, "Buzz"), 
};

for (int i = 1; i <= 100; i++)
{
    string output = combinations.Where(combination => i % combination.Item1 == 0)
                                .Aggregate((string)null, (sum, value) => sum += value.Item2);

    if (output == null)
    {
        output = i.ToString();
    }

    Console.WriteLine(output);
}

For fun, why not make the whole thing unreadable by making it into a single LINQ expression:

var combinations = new Tuple<int, string>[] 
{ 
    new Tuple<int, string> (3, "Fizz"), 
    new Tuple<int, string> (5, "Buzz"), 
};

Enumerable.Range(1, 100)
          .Select(i => combinations.Where(combination => i % combination.Item1 == 0)
                                   .Aggregate((string)null, (sum, value) => sum += value.Item2) ?? i.ToString())
          .ToList()
          .ForEach(Console.WriteLine);
\$\endgroup\$
  • \$\begingroup\$ +1 for clearly illustrating the plunge into absurdity. I'm reminded of the anecdote where the boss says "Jane is not such a good programmer, she never tackles difficult problems. Her solutions are too simple." \$\endgroup\$ – radarbob Aug 10 '12 at 17:46
2
\$\begingroup\$

Since you are printing at the same time as you are trying to figure out if it was found, then no, you cannot. If you do not mind using higher level approach such as Linq, then here is my version below. It just might be a tad faster for large number of combinations if Console.Write and Console.WriteLine are slow.

namespace FizzBuzzTest
{
    using System;
    using System.Diagnostics.Contracts;
    using System.IO;
    using System.Linq;

    public static class FizzBuzz
    {
        private static readonly Tuple<int, string>[] combinations = new[] 
        { 
            new Tuple<int, string> (3, "Fizz"), 
            new Tuple<int, string> (5, "Buzz"), 
        };

        public static void PrintFizzBuzz(
            TextWriter textWriter = Console.Out,
            int start = 1,
            int end = 100)
        {
            foreach (string value in GenerateFizzBuzz(start: start, end: end))
            {
                textWriter.Writeline(value);
            }
        }

        public static IEnumerable<string> GenerateFizzBuzz(
            int start = 1,
            int end = 100)
        {
            Contract.Requires(start >= 0, "Negative start value is not allowed.");
            Contract.Requires(end >= 0, "Negative end value is not allowed.");
            Contract.Requires(start > end, "Start must be greater than end.");

            for (int i = start; i <= end; i++)
            {
                var humanReadableTokens = from cmb in combinations
                                          where i % cmb.Item1 == 0
                                          select cmb.Item2;
                string humanReadableStr = String.Concat(humanReadableTokens);
                yield return humanReadableStr == String.Empty ? i.ToString() : humanReadableStr;
            }
        }

        public static int Main(string[] args)
        {
            PrintFizzBuzz();

            // PrintFizzBuzz(textWriter: Console.Error);

            // Or you can create your own subclass of TextWriter which gives you the ability to read and clear its buffer if you want to test FizzBuzz.
            // var testWriter = new testTextWriter();
            // PrintFizzBuzz(textWriter: testWriter);
            // Assert somethng about testWriter.Buffer
            // testWriter.ClearBuffer();
            // Reuse the testWriter object for further testing. 

            // Or you could take the values in from command line if there are exactly two integers.
            // Parse(...);
            // DoFizzBuzz(start: start, end: end);

            return 0;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Nice idea. LINQ definitly helps here. \$\endgroup\$ – Mare Infinitus Aug 3 '12 at 5:44
  • \$\begingroup\$ this suggestion may be OTT, but my only suggestion would be to pull out the UI logic (console writeline) and perhaps provide another parameter callback that could handle this? Doing this could even then allow for a bit of Unit testing? \$\endgroup\$ – dreza Aug 3 '12 at 21:27
  • \$\begingroup\$ @dreza, I would not put this code in the production hence I would not test it, but yeah, it is not hard to modify it to allow for testing, so check out the revised version. I like my approach better than the one with callbacks because I had to make fewer modification to the core code, and yet tests are now easy to write. blog.objectmentor.com/articles/2009/06/05/… \$\endgroup\$ – Leonid Aug 3 '12 at 21:47
  • \$\begingroup\$ @Leonid yes, I like it. The GenerateFizzBuzz method is completely isolated from how it's going to be used and even better doesn't have that extra parameter like I originally suggested. \$\endgroup\$ – dreza Aug 3 '12 at 23:18

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.