Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Now FizzBuzz itself isn't a big challenge but I agree that it can be a good tool to see if someone can code or not. I wanted to practice my LINQ a little bit so here's my single line FizzBuzz solution.

Enumerable.Range(1,100).Select(
                        n => 
                        (n % 15 == 0) ? "FizzBuzz" : 
                        (n % 3 == 0) ? "Fizz" : 
                        (n % 5 == 0) ? "Buzz" : 
                        n.ToString())
                        .ToList()
                        .ForEach(Console.WriteLine);
share|improve this question
11  
Would you care to ask a question? :) – Kieren Johnstone May 6 '14 at 12:14
3  
Apart from the alignment, this is exactly how I'd do it. – Magus May 6 '14 at 14:21
1  
This question (and answers) seem more suitable for CodeGolf SE's as popularity contest question – abuzittin gillifirca May 7 '14 at 8:47
up vote 22 down vote accepted

Overall, I like this. It is concise, and neat, and all. My only beef is the nested ternaries.

In general, ternary operator precedence is complicated by the lack of blocks.... The need for the parenthesis on the modulo precedence makes the conditions clear ( (n % 15 == 0) ) and the actual indenting you have, makes the logic a little clearer, but precedence is not that simple, and I have seen bugs where subtle precedence problems have caused the wrong execution-path through the nested ternaries....

.... bottom line, is for simple statements, ternaries are great, but the readability is compromised when there are multi-levels of ternaries. Nice for small things, ugly for big.

I would have preferred if you made the coditionals more obvious ... by extracting to a function maybe... but that would ruin your 1-liner.....

How about ... (Shown in this ideone here):

Enumerable.Range(1,100).Select(
                    n => 
                    (n % 15 == 0) ? "FizzBuzz" : 
                    (
                        (n % 3 == 0) ? "Fizz" :
                        (
                            (n % 5 == 0) ? "Buzz" :  n.ToString())
                        )
                    )
                    .ToList()
                    .ForEach(Console.WriteLine);

EDIT: The above answer is not as good as I wanted it to be.....

The exact problem with ternary precedence is that it is right-associative. This makes the logic potentially buggy, and hard to understand.....

Consider the ternaries:

expa = boola ? expc : expd;
expb = boolb ? expe : expf;

result = boolc ? expa : expb;

You can modify this to be:

expa = boola ? expc : expd;

result = boolc ? expa : boolb ? expe : expf;

Or, rewriting it in 'your' style, as:

expa = boola ? expc : expd;

result = boolc ? expa :
         boolb ? expe :
         expf;

But, you cannot 'inline' expa in the above as easily:

expb = boolb ? expe : expf;

result = boolc ? (boola ? expc : expd) : expb;

You need the parenthesis.

Inlining ternaries is just plain ugly.


Better solution

I regret suggesting to use the parenthesis to block the code in a more readable format. There is no more readable format, other than to extract the function, which is the 'right' thing to do:

    static private string FizzBuzz(int value)
    {
        if (value % 15 == 0)
        {
            return "FizzBuzz";
        }
        if (value % 3 == 0)
        {
            return "Fizz";
        }
        if (value % 5 == 0)
        {
            return "Buzz";
        }
        return value.ToString();
    }



    public static void Main()
    {
        foreach(var val in Enumerable.Range(1, 100).Select(n => FizzBuzz(n)))
            Console.WriteLine(val);
    }

Note, I have also undone the slow, and unnecessary conversion of the Enumerable to the ToList().ForEach() operation. Converting it to the list before re-converting it to an Enumeration is a waste of effort.

Bottom line, is that the 1-liner is not the best, or most readable format.

I have put this in an ideone, to show it working.

share|improve this answer
    
I'm not sure that helps much. At first glance it looks like the block runs if n is divisible by 15. (And shouldn't those be parens, not braces?) – cHao May 6 '14 at 13:33
    
If you're going to do this, at least get the "Fizz" and "Buzz" right, and ditch the 15 in favor of concatenation. You only need an explicit 15 condition in a ternary where you can't concatenate. – Magus May 6 '14 at 21:54
    
@Magus - the concatenation would require a more complicated function... if you have an alternate solution, you should put it together in an answer (with reasoning as to why it improves the original code). – rolfl May 6 '14 at 22:01
1  
@Magus: Three hard-coded strings ends up simpler, and in that respect better, than two. It's easier to see what's going on than if you concatenate. – cHao May 6 '14 at 22:23
3  
@Voo the ternary is intuitive, and I love it, but not for nesting if blocks . This usage is too nested. A single ternary in each statements is all I am saying. – rolfl May 7 '14 at 0:32

Your code is nice, relatively simple and a good attemp at LINQ - I've written very similar code before. However I have the following critiques:

  • A list is created unnecessarily, eagerly loading the data. LINQ loves being deferred and streamed when possible - what happens if we never execute the code? What happens if we decide that we only need the first 50 results?
  • The foreach method, while I like it, is considered poor code - LINQ shouldn't rely upon side effects, and the foreach keyword is roughly the same length.
  • You're not using the LINQ paradigm - merely using the Enumerable and Select as a loop construct. This is why you have to use the Ternary if. Where is the Query? What are your data collections? And most importantly is it declarative - are you asking what you want rather than telling the computer how to get there?

Keeping within the '1LOC' idea:

foreach (var result in from number in Enumerable.Range(1, 100)
                       let match = (from condition in new[] 
                                    {   
                                        new {Key =15, Format= "FizzBuzz" },
                                        new {Key =5, Format= "Buzz" },
                                        new {Key =3, Format= "Fizz" },
                                        new {Key =1, Format= "{0}" }
                                    }
                                    where number % condition.Key == 0
                                    select condition.Format).First()
                       select string.Format(match, number))
    Console.WriteLine(result);
Console.ReadLine();

Why This Way?

I spent a fair bit of time coming up with this particular example - though its possible improvements could be made. Since this question and this answer have some interest, I'd explain why I chose this format.

My decision is simple: I want to use the LINQ Paradigm as much as possible, but want to stick within the artificial 1LOC requirement (otherwise I'm not really answering the question).

Let's break it down into steps:

Get a collection (0 to infinity) of values.
Get from a collection (0 to infinity) of 'conditions' 
   the format of the first condition where 'key' matches the value
Select the value formatted by the format.

Now the cool thing is that this basic structure is very generic. For example the collection of values can be XML, or maybe an infinite IEnumerable. The conditions could be a simple array, or a remotely hosted database. That's not forgetting that 'value' and 'condition' can be defined in many different ways - e.g. value could be an exception log and condition could be a function that formats or raises alerts based said exception.

foreach (var result in from log in Log.LastRun()  //gets log entries
                       let match = (from logHandler in Log Handling.Current  //finds appropriate handlers
                                    where log.Type == logHandler.Key
                                    select logHandler.Handler).First()
                       select match(log).Result)  //handle log - e.g. send SMS, alert user, create statistics, do nothing
    Console.WriteLine(result);  //E.g. 'Successfully SMS'd Error to User'
Console.ReadLine();

Of course, returning to our simple number variant, LINQ can be useful for making our problem easier to modify. For example, I require a version of FizzBuzz that prints "Douglas Adams" on mod 42 == 0, and will give me values 1 through to Infinity - and will ignore all values between 600 and 700.

foreach (var result in (from number in Counter(start: 1)
                       where number < 600 || number > 700
                       let match = from condition in new[] 
                                    {   
                                        new {Key =42, Format= "Douglas Adams" },
                                        new {Key =15, Format= "FizzBuzz" },
                                        new {Key =5, Format= "Buzz" },
                                        new {Key =3, Format= "Fizz" },
                                        new {Key =1, Format= "{0}" }
                                    }
                                    where number % condition.Key == 0
                                    select condition.Format).First()
                       select string.Format(match, number))
    Console.WriteLine(result);
Console.ReadLine();
share|improve this answer
    
Thanks, you're revised solution is quite good - keeping the imperative code separate from the declarative LINQ. +1. I tried to keep it all declarative (at least, to my ability) due to the question for learning purposes, even though it's not great production code. – NPSF3000 May 6 '14 at 14:46
    
Other than the fact that .ForEach() is not LINQ (it's an instance method on List<T>), I agree completely. – Jesse C. Slicer May 6 '14 at 15:06
3  
This solution seems, to me, to be harder to read than the original. – Casey May 6 '14 at 15:24
4  
I like it, although to me, it would be cleaner to use an anonymous type instead of a tuple, so I would for example replace Tuple.Create (15, "FizzBuzz" ) with new { number = 15, text = "FizzBuzz" }. – Overly Excessive May 6 '14 at 16:05
    
Does C# know better than to create a new array of tuples for each item in the range? Cause if not, this seems like it would create quite a bit of unnecessary garbage. – cHao May 6 '14 at 21:29

It's not necessary to extract the function. A multi-statement lambda will suffice:

Enumerable.Range(1,100).Select(n => {
    if (n % 3 == 0) {
      return n % 5 == 0 ? "FizzBuzz" : "Fizz";
    }
    return n % 5 == 0 ? "Buzz" : n.ToString();
}).ForEach(Console.WriteLine);

I prefer to use my own ForEach extension method rather than put the whole collection into a List:

public static void ForEach<T>(this IEnumerable<T> collection, Action<T> fn)
{
    foreach(T item in collection) { fn(item); }
}
share|improve this answer

Here's a variation on NPSF3000's example, but using method syntax and a Dictionary instead of a Tuple[]:

Enumerable.Range(1, 100)
.Select(n => new Dictionary<int, string>
    { {15, "FizzBuzz"}, {3, "Fizz"}, {5, "Buzz"}, {1, n.ToString()} }
    .First(kv => n % kv.Key == 0).Value)
.ToList()
.ForEach(Console.WriteLine);

Update: as noted in the comments, Dictionary doesn't guarantee order, even though this example works correctly on my machine. So if you use this KeyValueList implementation, you can replace Dictionary<int, string> with KeyValueList<int, string> and still use the dictionary initializer syntax. I prefer this to using the SortedDictionary since it doesn't change the ordering shown in code and therefore doesn't require using Last() instead of First().

share|improve this answer
1  
There's no guarantee that enumerating a Dictionary will return the entries in the order in which you added them. You could call OrderBy before First each time but since you're not using the O(1) lookup of the dictionary you may as well use a List or other ordered collection. – Rawling May 7 '14 at 9:15
1  
Well, you could use a SortedDictionary and Last instead of First; but the only advantange over a simple array would be the nice collection initializer syntax instead of using an anonymous type. – sloth May 7 '14 at 9:58
    
Thanks, you're both correct. I've added an update that hopefully addresses your points. – Oran Dennison May 7 '14 at 18:46

I've used some answers here with a little refactoring, so you can easily add more numbers to divide:

var dic = new Dictionary<int, string> { { 3, "Fizz" }, { 5, "Buzz" } };

var t = 
        from number in Enumerable.Range(1, 100)
        let names = dic.Where(kv => number % kv.Key == 0).Select(kv => kv.Value)
        select names.Any() ? string.Join("", names) : number.ToString();

t.ToList().ForEach(Console.WriteLine);

Notice that it concatenates all the names that a number has, if it doesnt have any, it returns the number.

share|improve this answer

protected by Jamal Nov 9 '15 at 17:14

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.