17
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ "It's definitely not something a beginner would write." It's not something anyone should write except for fun. I'm not sure being a beginner or expert has anything to do with it. \$\endgroup\$ – Mast Nov 14 at 18:52
25
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Great catch on that asynchronous/parallel part. But including the .AsParallel() call in the method chain would require me to make two chains. One for parallel and the other for sequential. \$\endgroup\$ – Wim ten Brink Nov 11 at 15:26
  • 2
    \$\begingroup\$ @WimtenBrink You can store the chain in a variable. IEnumerable<int> sequence = count.MakeIntArray(); if(asynchronous){sequence = sequence.AsParallel(); } sequence.MakePairs().Fizz()... etc \$\endgroup\$ – JAD Nov 11 at 15:28
  • \$\begingroup\$ As for going overboard, that was a bit of a purpose here. :-) A generic Substitute could be used but I wanted to have each specific condition as a separate method. After all, there is only one condition for when to Fizz and one for when to Buzz. But the Substitute method is in it! I called it the IfThis() method. But I want the condition and replacement functions to be reusable. It's an exercise in going overboard with reusability. \$\endgroup\$ – Wim ten Brink Nov 11 at 15:32
  • \$\begingroup\$ How about public static IEnumerable<T> DoParallel<T>(this IEnumerable<T> data, bool parallel) => parallel ? data.AsParallel() : data;? Then I can use .DoParallel(true) instead. \$\endgroup\$ – Wim ten Brink Nov 11 at 15:51
  • \$\begingroup\$ Wait... For whatever reason, including AsParallel() into the method chain doesn't make it go parallel! All actions are still executed inside a single thread. So storing the chain variable as either parallel or not isn't working... \$\endgroup\$ – Wim ten Brink Nov 11 at 16:03
15
\$\begingroup\$

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!

\$\endgroup\$
  • 4
    \$\begingroup\$ @WimtenBrink Yep, it's certainly a good exercise. Now, I'm a little curious: you say that you're making these functions reusable, but when, how and why are you imagining these functions might be reused? \$\endgroup\$ – Tanner Swett Nov 12 at 15:31
  • 8
    \$\begingroup\$ @Wim The first thing to learn when thinking about making things super duper reusable without a current need is understanding YAGNI. \$\endgroup\$ – Voo Nov 12 at 20:03
  • 5
    \$\begingroup\$ A good rule of thumb is to only make something reusable once you've used it three times. Before that, it's hard to predict what, if any, context you'll want to reuse it in. \$\endgroup\$ – James_pic Nov 13 at 13:58
  • 2
    \$\begingroup\$ This is about abstraction, not reusability. Do you gain any added insight by replacing IsFizz(data) ? DoFizz(data) : data with data.IfThis(IsFizz, DoFizz)? No, you're not. You're just re-stating what the first piece of code is doing. However, replacing Enumerable.Range(0, count) with count.MakeIntArray() is a valuable abstraction. It clarifies why Enumerable.Range(0, count) is being called. \$\endgroup\$ – Krzysztof Czelusniak Nov 13 at 18:18
  • 1
    \$\begingroup\$ @TannerSwett for someone just learning, or someone writing for people who are just learning Linq concepts, it isn't necessarily a given that Enumerable.Range is understood for what it is at a glance. In this context giving it a name (even if the name is a lie) can certainly be helpful. \$\endgroup\$ – Mr.Mindor Nov 13 at 18:59
11
\$\begingroup\$

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

}
\$\endgroup\$
  • \$\begingroup\$ It's not about performance but about reusability. I turned a small problem into even smaller parts. But testing for other terms is possible by adding an IfThis() in the method chain with a condition and a new key/value that would be used. \$\endgroup\$ – Wim ten Brink Nov 11 at 21:37
  • \$\begingroup\$ As for the DoThis(), one of the three versions is meant to use an unrelated condition to the time, like a timer or maybe a key event. It's meant to be independent. The other has the data type as a parameter for the action. (I'm using DoThis() in another project with a timer that counts 100 milliseconds.) \$\endgroup\$ – Wim ten Brink Nov 11 at 21:38
9
\$\begingroup\$

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?
\$\endgroup\$
9
\$\begingroup\$

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.

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

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));
}
\$\endgroup\$

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.