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.

I'm just playing with the concept of GroupBy inside Rx. So, I wondered how hard it would be to write a console application that continuously reads lines, groups them by similar words and just prints out the word among the current count of how often the word was written before. It really was a breeze to do that but I wonder if my attempt could be rewritten in a more elegant way. Especially if I can get rid of the nested Subscribes.

Here is what I have:

static void Main(string[] args)
{
    var subject = new Subject<string>();

    var my = subject.GroupBy(x => x);

    my.Subscribe(x => x.Scan(new { Chars = string.Empty, Count = 0},
                                 (a, chars) => new { Chars = chars, Count = a.Count + 1})
      .Subscribe(result => Console.WriteLine("You typed {0} {1} times", result.Chars, 
                 result.Count)));

    while (true)
    {
        subject.OnNext(Console.ReadLine());
    }
}

Result

test
you typed test 1 times
test
you typed test 2 times
hallo
you typed test 1 times
test
you typed test 3 times
...
share|improve this question

migrated from stackoverflow.com Nov 12 '11 at 18:52

This question came from our site for professional and enthusiast programmers.

    
Can anyone who donvoted this question please leave a comment what's wrong with it? I'm willing to fix it! –  Christoph Nov 12 '11 at 17:58
    
I think you got downvotes because you're not even asking a question. –  Otiel Nov 12 '11 at 18:02
    
It would help to know what you would consider more elegant. I would suggest more newlines in the lambda you pass to Subscribe would make a bigger difference. –  Richard Nov 12 '11 at 18:03
    
@Otiel, well the question is if the code snippet can be improved. I should point out that I'm striving for improvement in terms of mathematical beauty. For example, in my first answer I improved it with the SelectMany so that the code is more in line with the whole functional programming approach. In the second answer I avoided passing the string information through the Scan() method in favor for the Zip. –  Christoph Nov 12 '11 at 18:16
1  
@Christoph and I added it back. I hope you don't mind, but it really does need to be tagged C#, and since the question is right on topic here at CodeReview, I don't think you'll have any further downvote issues. –  codesparkle Sep 8 '12 at 8:50
show 1 more comment

3 Answers 3

Ok, this looks a lot better! Can anyone even do better than that?

    static void Main(string[] args)
    {
        var subject = new Subject<string>();

        subject
            .GroupBy(x => x)
            .SelectMany(x => x.Scan(new { Chars = string.Empty, Count = 0},(a, chars) => new { Chars = chars, Count = a.Count + 1}))
            .Subscribe(result => Console.WriteLine("You typed {0} {1} times", result.Chars, result.Count));

        while (true)
        {
            subject.OnNext(Console.ReadLine());
        }
    }
share|improve this answer
1  
I never used Rx. However, having used LINQ, I should be able to follow this code ... and I do not really. I can barely tell what it does and how fast it runs. I like an explicit dictionary-based approach. If I had to debug/maintain this method, I would rewrite it. –  Leonid Apr 11 '12 at 3:03
add comment

Ok, here is another one! I think this one is easier to read since we don't depend on the Scan method to carry the string through the computation:

    static void Main(string[] args)
    {
        var subject = new Subject<string>();

        subject
          .GroupBy(x => x)
          .SelectMany(x => x.Scan(0, (count, _) => ++count).Zip(x, (count, chars) => new { Chars = chars, Count = count}))
          .Subscribe(result => Console.WriteLine("You typed {0} {1} times", result.Chars, result.Count));

        while (true)
        {
            subject.OnNext(Console.ReadLine());
        }
    }

Can anyone do even better?

share|improve this answer
3  
You should edit your initial answer and add this answer to it. Multiple answers by one person will trigger flagging (manual and automatic). Also since this is your question you should consider adding both answers into the question, but it's perfectly ok to answer your own question. –  Yannis Nov 14 '11 at 5:17
add comment

This looks a lot cleaner to me. Should also showcase how much more helpful it is when you give more descriptive names to your variables.

static void Main(string[] args)
{
    var lineReader = new Subject<string>();

    lineReader.GroupBy(line => line)
        .Subscribe(lineGroup =>
        {
            lineGroup.Scan(0, (acc, _) => ++acc)
                .Subscribe(count =>
                {
                    var line = lineGroup.Key;
                    var timeSuffix = count == 1 ? "" : "s";
                    Console.WriteLine("You typed {0} {1} time{2}.", line, count, timeSuffix);
                });
        });

    String readLine;
    while ((readLine = Console.ReadLine()) != null)
        lineReader.OnNext(readLine);
}
share|improve this answer
add comment

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.