Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
8  
Would you care to ask a question? :) –  Kieren Johnstone May 6 at 12:14
2  
Apart from the alignment, this is exactly how I'd do it. –  Magus May 6 at 14:21
1  
This question (and answers) seem more suitable for CodeGolf SE's as popularity contest question –  abuzittin gillifirca May 7 at 8:47

4 Answers 4

up vote 13 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 at 13:33
    
@cHao .... well, yes ... :( Let me fix that... –  rolfl May 6 at 13:36
    
@cHao - better answer now, agree? –  rolfl May 6 at 14:41
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 at 22:23
2  
@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 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
3  
+1 ... I simultaneously was editing my answer, when you posted yours. I agree with the foreach.... –  rolfl May 6 at 14:28
    
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 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 at 15:06
3  
This solution seems, to me, to be harder to read than the original. –  emodendroket May 6 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 at 16:05

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 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 at 9:58
    
Thanks, you're both correct. I've added an update that hopefully addresses your points. –  Oran May 7 at 18:46

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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