Solving Fizz Buzz using LINQ in C#

Question

The Fizz Buzz challenge is well-known and should not need any explanations, right?
I recently got a bit bored and came up with a solution for Fizz Buzz using LINQ and even asynchronous code and a lot of extension methods. Actually, it's only Extension methods!
So, I thought, let's share it and have a review of my code!

using System;
using System.Collections.Generic;
using System.Linq;

namespace FizzBuzzApp
{
    using IntEnum = IEnumerable<int>;
    using Pair = KeyValuePair<int, string>;
    using PairEnum = IEnumerable<KeyValuePair<int, string>>;
    using StringEnum = IEnumerable<string>;

    public static class FizzBuzzClass
    {
        public static T DoThis<T>(this T data, Action<T> action) { action(data); return data; }
        public static T DoThis<T>(this T data, Action action) { action(); return data; }
        public static IEnumerable<T> DoThis<T>(this IEnumerable<T> data, Action<T> action) { foreach (var item in data) { action(item); yield return item; } }

        public static T IfThis<T>(this T data, Func<T, bool> condition, Func<T, T> action) => condition(data) ? action(data) : data;
        public static T IfThis<T>(this T data, Func<T, bool> condition, Func<T, T> onTrue, Func<T, T> onFalse) => condition(data) ? onTrue(data) : onFalse(data);

        public static IntEnum MakeIntArray(this int count, int start = 0) => Enumerable.Range(start, count);

        public static Pair NewPair(this int key) => new Pair(key, string.Empty);
        public static Pair NewPair(this Pair pair, string data) => new Pair(pair.Key, data.Trim());
        public static PairEnum MakePairs(this IntEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(NewPair) : data.Select(NewPair);
        public static IOrderedEnumerable<Pair> SortPairs(this IEnumerable<Pair> data) => data.OrderBy(p => p.Key);

        public static bool IsFizz(this Pair data) => data.Key % 3 == 0;
        public static Pair DoFizz(this Pair data) => data.NewPair("Fizz " + data.Value);
        public static Pair Fizz(this Pair data) => data.IfThis(IsFizz, DoFizz);
        public static PairEnum Fizz(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Fizz) : data.Select(Fizz);

        public static bool IsBuzz(this Pair data) => data.Key % 5 == 0;
        public static Pair DoBuzz(this Pair data) => data.NewPair(data.Value + " Buzz");
        public static Pair Buzz(this Pair data) => data.IfThis(IsBuzz, DoBuzz);
        public static PairEnum Buzz(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Buzz) : data.Select(Buzz);

        public static bool IsNumber(this Pair data) => string.IsNullOrEmpty(data.Value);
        public static Pair DoNumber(this Pair data) => data.NewPair(data.Key.ToString());
        public static Pair Number(this Pair data) => data.IfThis(IsNumber, DoNumber);
        public static PairEnum Number(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Number) : data.Select(Number);

        public static StringEnum Values(this PairEnum data) => data.Select(p => p.Value);
        public static string Combine(this StringEnum data) => string.Join(", ", data);

        public static string FizzBuzz(this int count, bool asynchronous = false) =>
            count.MakeIntArray()
                 .MakePairs(asynchronous)
                 .Fizz(asynchronous)
                 .Buzz(asynchronous)
                 .Number(asynchronous)
                 .SortPairs()
                 .Values()
                 .Combine();
}

To use it, just use 100.FizzBuzz(true); for the asynchronous version.
Now, the thing is that it's full one-liner extension methods. It's definitely not something a beginner would write. But is it well-written for experts?

Answer 1

Parallel computing and asynchonous computing are two separate things. While they might be similar in some cases, the two terms have two very distinct meanings in C#-land:

1. Asynchronous computing is used for the async/await and Task<T> idioms.
2. Parallel computing is what you are doing here, using plinq: processing streams of data in different threads in parallel.

With that said, you're not using the features of Plinq to the fullest. Instead of passing the (incorrectly named) asynchronous to each of the methods, count.MakeIntArray().AsParallel() would have sufficed to make the entire thing parallel.

---

I think you went a bit overboard with the amount of extension methods you created, while gaining little for it in return. What if a new request would come to say something new on every number modulo 10? Now you have to make changes in 4 methods (or more accurately, create 4 more methods) to account for these changes!

Instead you could have made the Fizz and Buzz methods two versions of a more generic Substitute method:

public string Substitute(int number, Func<int, bool> when, string with);

Answer 2

Understanding your code was a bit of a challenge (albeit an interesting one).

Overall, I see a lot of function definitions that don't seem to accomplish anything. Why did you write

public static IOrderedEnumerable<Pair> SortPairs(this IEnumerable<Pair> data) => data.OrderBy(p => p.Key);

and then write

.SortPairs()

when you could have just written

.OrderBy(p => p.Key)

to begin with?

The only possible advantage of what you've done is that it's easier to read .SortPairs() than it is to read .OrderBy(p => p.Key)... but the thing is, it isn't easier to read .SortPairs() than it is to read .OrderBy(p => p.Key). So you haven't gained anything. You've lost something, too: when I'm looking at the definition of FizzBuzz, now I have to go find the definition of SortPairs in order to see what it does.

Likewise, instead of defining count.MakeIntArray() as Enumerable.Range(0, count), you could have just written Enumerable.Range(0, count) to begin with. Writing count.MakeIntArray() serves no purpose other than making me spend an extra 15 seconds in order to understand what this code does.

And likewise, instead of writing data.IfThis(IsFizz, DoFizz), you could have just written IsFizz(data) ? DoFizz(data) : data to begin with. And so on and so forth and so on and so forth.

So, what's my advice to you?

Separating different pieces of logic into separate functions is a great idea... but only to a certain extent. The possible advantages of putting logic into a function include:

• The calling code may be easier to understand if it uses a function name than if it uses the logic directly. For example, it's easier to understand what .OrderBy(p => p.Key) does than to look at an entire sorting algorithm and understand what it does. (But it is not easier to understand what .SortPairs() does than to understand what .OrderBy(p => p.Key) does.)
• The logic may be used in multiple places. If so, putting it in a function makes it so that you can change it everywhere by changing it just once. (But you're only using .SortPairs() in one place.)
• You may want to pass the logic into a higher-order function. (But you're not doing that with .SortPairs().)
• Putting logic in a function may allow you to make your code easier to understand by putting that function in a more appropriate place. For example, if you have a form that needs to display sales tax, then putting the sales tax logic in a function would allow you to put that logic somewhere else besides the form class. (But you're not putting the definition of .SortPairs() in a more appropriate place.)
• Putting logic in a function may make that logic reusable, as you mention yourself a few times. (But the functions you've written are never going to be useful for anything besides FizzBuzz, are they? Where's the reusability?)

The cost of defining an additional function is small, but not zero. Make sure that you're getting a benefit from that cost—especially if you're paying that cost 24 times in a 33-line class. Each function definition took me about 15 seconds to follow, meaning that your code took me an extra 6 minutes to understand, for little if any benefit.

The bottom line: if you're not getting any benefits from defining a function, don't define a function!

Answer 3

While your goals may be admirable, I think that, when performance takes a large hit, the whole approach needs to be re-examined. This is atleast 5X's slower than a comparable one liner, even after removing the Combine() method and just returning an IEnumerable<string>.

If someone wanted to test with different terms(ie 7 and 9) and/or different words(ie "Fazz" and "Bizz") your code would need to be modified and re-compiled.

The DoThis method doesn't check that the data is an appropriate parameter for the action.

The methods particular to Fizz and Buzz basically are just duplicated code. It would seem to me that methods that also take the appropriate Fizz or Buzz value would still be useable but would eliminate much of the duplication.

Here's the one liner I used as comparison:

using kvp = KeyValuePair<int, string>;

/// <summary>
/// Creates IEnumerable<string> with the appropriate FizzBuzz values.
/// </summary>
/// <param name="terms"> This must be 2 elements long</param>
public static IEnumerable<string> FizzBuzz(this int limit, bool parallel = false,
                                           params KeyValuePair<int, string>[] terms)
{
    if(terms.Length != 2)
    {
        throw new ArgumentException("'terms' length must be 2");
    }
    if (parallel)
    {
        return Enumerable.Range(1, limit)
            .AsParallel()
            .Select(x =>
            {
                var fizz = x % terms[0].Key == 0;
                var buzz = x % terms[1].Key == 0;
                return (fizz && buzz) ? $"{terms[0].Value}{terms[1].Value}" : (fizz || buzz) 
                       ? fizz ? terms[0].Value : terms[1].Value : x.ToString();
            });

    }
    else
    {
        return Enumerable.Range(1, limit)
            .Select(x =>
            {
                var fizz = x % terms[0].Key == 0;
                var buzz = x % terms[1].Key == 0;
                return (fizz && buzz) ? $"{terms[0].Value}{terms[1].Value}" : (fizz || buzz) 
                       ? fizz ? terms[0].Value : terms[1].Value : x.ToString();
            });
    }

}

Answer 4

The comments mention being reusable as the main goal of the code. It is however far from it.

The main issues I see with most of the extension methods above:

• Looking at the name of the methods (and parameters) I can't always tell what the outcome will be.
• When the outcome is pretty clear, the implementation reveals surprinzing, totally unexpected side-effects.

Here are some of the questions I would ask if I saw these methods in a library I would like to reuse:

• Pair NewPair(this int key) - How does an int become a pair? What will the key/value look like?
• IntEnum MakeIntArray - Why does it say Array but return IEnumerable?
• Pair NewPair(this Pair pair, string data) - Why does it trim data?
• IOrderedEnumerable<Pair> SortPairs(this IEnumerable<Pair> data) - Sort them by what? Key, value, both? How are items compared?

Answer 5

Now, I fully congratulate you on the exercise - it's an interesting problem and I think a great learning opportunity. However, to learn from this we must acknowledge it's shortcomings.

I think the biggest, most glaring issue for me is that this decomposes business logic beyond the simplest possible implementation! Take a second and look at these three lines:

    public static Pair DoFizz(this Pair data) => data.NewPair("Fizz " + data.Value);
    public static Pair DoBuzz(this Pair data) => data.NewPair(data.Value + " Buzz");
    public static Pair DoNumber(this Pair data) => data.NewPair(data.Key.ToString());

Now, why does 'Fizz' get prepended and yet 'Buzz' get suffixed? Why does DoNumber simply take a brand new value?

Simple: there's an implicit dependency here between the business logic that has been obfuscated under the layers of fluent methods.

To put it another way, what happens if we change the order of these methods? For example: ...Number(asynchronous).Fizz(asynchronous).Buzz(asynchronous)...? I think the results would not be what one would expect.

Other issues:

• There's to much focus on what you are doing, rather than how you are doing it. You've decomposed the business logic... into separate, unreusable steps...rather than finding the common algorithm.

One possible alternative solution would be to try and implement a case statement in Linq. Something like:

public IEnumerable<T2> Case<T1, T2>(Func<T1, bool> match, T2 result, Func<T1, bool> match2, T2 result2...)

• The code is incredibly hard to read (due to the above)... it took me a good 15 minutes, with access to the entire source, to understand what was happening. KISS.
  • Take public static IntEnum MakeIntArray(this int count, int start = 0) => Enumerable.Range(start, count); - It doesn't return an Array, IntEnum is a confusing alias (especially as C# has 'enums' already), and it hides a method that C# developers are already familiar with.
• The asynchronous... isn't asynchronous as others have mentioned.
• It's not really LINQ - sure, it's Fluent and uses IEnumerable<T>'s... but it's not really extending the Query language to help me query things... it's just hardcoded business logic. Think the difference between .First(p => p.name) and .FirstName().

To leave on a good note, I do think this is the beginning of a good idea:

         public static string Combine(this StringEnum data) => string.Join(", ", data);

However, I'd call it Join and make it an alias of String.Join. It's an operation that's generic, and I often find it useful.

Answer 6

I would argue your garden-variety C# developer is pretty sharp with LINQ as it is, rather than abstracted away, and also hopefully keeping up with modern C# innovations such as first-class Tuple support. Your take becomes a two-liner which I would argue is just as readable and maintainable. If you are wanting real maintainability, usability, and extensibility, first look to separating the concerns (into different classes), segregating them via interfaces or delegates as needed, then injecting those dependencies where they're required. I'm not going to go into all that -- feel free to look at those "Enterprise FizzBuzz" links on the question. However, here's the short version that I described at the beginning (yes, I am aware the output is slightly different but matches most original FizzBuzz challenges):

public static string FizzBuzz(this int count, bool parallel = false)
{
    IEnumerable<int> range = Enumerable.Range(0, count);

    return string.Join(
        ", ",
        (parallel ? range.AsParallel() : range)
            .Select(key => (Key: key, Value: string.Empty))
            .Select(data => data.Key % 3 == 0 ? (data.Key, Value: "Fizz") : data)
            .Select(data => data.Key % 5 == 0 ? (data.Key, Value: data.Value + "Buzz") : data)
            .Select(data => string.IsNullOrEmpty(data.Value) ? (data.Key, Value: data.Key.ToString()) : data)
            .OrderBy(p => p.Key)
            .Select(p => p.Value));
}