8
\$\begingroup\$

My goal is to read from standard input, break up the input into words (Unicode letters and apostrophes), case-insensitively, and produce a report to standard output, where each line is a word followed by a space followed by its count. The output should be sorted by words.

This is my code:

using System;
using System.Text.RegularExpressions;
using System.Collections.Generic;

class TraditionalWordCountApp
{
    public static void Main(string[] args)
    {
        SortedDictionary<string, int> counts = new SortedDictionary<string, int>();
        Regex wordRegex = new Regex(@"[\p{L}']+");
        string line;
        while ((line = Console.ReadLine()) != null)
        {
            line = line.ToLower();
            foreach (Match m in wordRegex.Matches(line))
            {
                String word = m.Value;
                int count;
                counts.TryGetValue(word, out count);
                counts[word] = ++count;
            }
        }
        foreach (KeyValuePair<string, int> pair in counts)
        {
            Console.WriteLine("{0} {1}", pair.Key, pair.Value);
        }
    }
}

My concerns:

  • Should things be wrapped in a namespace?
  • Should either the class or the main function or both be public?
  • Is there a nicer way to declare the sorted dictionary? Java has <>, does C#?
  • I used the C-style idiom of assigning to a variable and then checking for null, all inside the while-condition! Surely there is a nicer way to do this in C#. Is there? I like neither the embedded assignment nor the billion-dollar-mistake null. :)
  • I searched StackOverflow a bit for a nicer way to update a word frequency dictionary. Most languages seem to have a built-in get-with-default method on dictionaries, but the answers I saw said no, and they recommended TryGetValue. These answers are old, though. Is there something nicer in the most modern C#?
  • Any nicer way to do the last loop?
  • Would a LINQ-based solution be nicer? More idiomatic? What would it look like? I think (though I am brand new to C#) I could whip something up, but should I? FWIW I do know how to do the LINQ-y stuff in Java but wonder if this is the right way to go in C#.
\$\endgroup\$
10
\$\begingroup\$
  1. It's not mandatory but it's a widely accepted guideline. Break it only if you have a good reason to do so.
  2. No, they don't. Class should be internal (default) and static (because it has not instance methods). Main() method should be private to make clear that it won't be called outside this class but it's not mandatory (someone prefers public because it's logically called from outside).

For the other points things are more complex that a paragraph...

You can create a generator to read from standard input (see very last paragraph for a discussion about this):

private static IEnumerable<string> ReadAllInputs()
{
    while (true)
    {
        string line = Console.ReadLine();
        if (line == null)
            break;

        yield return line;
    }
}

You do not need ToLower() on input, I do not even start to mention all the problems of case insensitive comparison if not done properly: a lower case string may be different from an upper case string (even in number of characters) but they may be compared equal. Don't put yourself in trouble and let the framework do the work for you:

var counts = new SortedDictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);

Note that you do not need a SortedDictionary here, ordering may be done (in this case) when printing output. If performance matters then you should profile by yourself.

When you increment word count you also increment local variable count, not a big performance hit (and compiler may be smart enough to elide local variable) but it's about code clarity. You can do:

counts[word] = count + 1;

Or simply:

++counts[word];

Note that this innocent code ++counts[word] (as well as original counts[word] = ++count) when word doesn't exist in dictionary (TryGetValue returns false) will perform another lookup vanishing part of the benefit of TryGetValue for the first insertion! You have to explicitly call Add().

If you want you can refactor foreach loop to print results with LINQ, not a big gain (IMO), let's see how:

Console.WriteLine(String.Join(Environment.NewLine,
    counts.Select(x => $"{x.Key} {x.Value}")));

If we put everything together with then have something like this:

namespace Test {
    static class TraditionalWordCountApp {
        static void Main(string[] args) {
            var counts = new Dictionary<string, int>(StringComparer.CurrentCultureIgnoreCase);
            var wordRegex = new Regex(@"(?i)[\p{L}']+");

            foreach (var match in ReadAllInputs().SelectMany(x => wordRegex.Match(x))) {
                if (counts.TryGetValue(match.Value, out count))
                    counts[word] = count + 1;
                else
                    counts.Add(match.Value, 1);    
            }

            Console.WriteLine(String.Join(Environment.NewLine,
                counts.OrderBy(x => x.Key).Select(x => $"{x.Key} {x.Value}")));
        }

        static IEnumerable<string> ReadAllInputs() {
            while (true) { // Yes, in C# this is "safe"
                string line = Console.ReadLine();
                if (line == null)
                    break;

                // No need to match any regex against an empty line
                if (line.Length == 0)
                    continue; 

                yield return line;
            }
        }
    }
}

All these done you may think that...there should be another way to do it. You can use GroupBy(). Profile both versions to see which one has better performance (and the one you feel more easy to understand):

var counts = ReadAllInputs()
    .SelectMany(x => regex.Match(x))
    .GroupBy(x => x.Value, StringComparer.CurrentCultureIgnoreCase)
    .Select(x => new { Word = x.Key, Count = x.Count() })
    .OrderBy(x => x.Word);

Then you print it with:

Console.WriteLine(String.Join(Environment.NewLine,
    counts.Select(x => $"{x.Word} {x.Count}")));

One final note about word counting. It may apply or not to your case but don't forget that word is a concept with different meanings in different languages. Topic is vast but try to paste a - for example - Japanese text in Microsoft Word and check what the word count is: number of characters (another vague concept). If you have to deal with international text you may need to write specific code for different cultures.


Comments. Few notes collected from comments.

Sometimes you see this kind of code:

while (Console.In.Peek() != -1)
   yield return Console.ReadLine();

It works only if console input is redirected, when input is line buffered then it'll end immediately after first line. Nice and short but simply it doesn't work. If you really really love LINQ and you can't live without one-line functions then you may do this:

static IEnumerable<string> ReadAllInputs() {
    return Enumerable.Range(0, Int32.MaxValue)
        .Select(_ => Console.ReadLine())
        .TakeWhile(x => x != null);
}

Can we make our reader more "generic"? The obvious idea is to use it with files. Maybe yes, maybe not. First of all a console application can have piped input (from file or from another application) then we may not need this feature. If we do then we should introduce a separate class InputReader, implementation will be different for ConsoleInputReader, FileInputReader and HttpInputReader (for example); one function with a TextReader parameter will probably be the worst compromise for all our use-cases. What I'm suggesting is just to don't introduce complexity until you need it...

\$\endgroup\$
  • \$\begingroup\$ That looks nice. Lots of good stuff here, like the string interpolation I did not know about. BTW I saw on StackOverflow a suggestion for writing the generator as while (Console.In.Peek() >= 0) yield return Console.ReadLine(); Not sure if it's preferable. \$\endgroup\$ – Ray Toal Jul 19 '16 at 9:24
  • \$\begingroup\$ @RayToal Personally I think a better generator would be IEnumerable<string> ReadLines(TextReader reader) { while(reader.Peek() != -1) { yield return reader.ReadLine(); } } because it's more generalised and could be used as ReadLines(Console.In) or ReadLines(File.OpenText(path)). I.e. by using TextReader, the method does not have to care about the origin of its input, merely that it is presented as a TextReader \$\endgroup\$ – Pharap Jul 19 '16 at 11:15
  • \$\begingroup\$ @Pharap no, try yourself and you'll see Console.In.Peek() doesn't behave like you expect (from default stdin input is line buffered) then it will break after first line. Well, even if it worked in that way (it doesn't) I'd avoid any generalization until it's required (for file input I'd go with File.ReadAllLines() instead then I'd need to introduce a class InputReader with ConsoleInputReader and FileInputReader, not just a function parameter. Too much useless code for 20 lines of code), plus console app may have piped input... \$\endgroup\$ – Adriano Repetti Jul 19 '16 at 11:36
  • \$\begingroup\$ @AdrianoRepetti I tested it and it works for command line piping via cmd but fails with the interactive console, so you're right that it's not equivalent to using the Console functions, meaning a specialised version would be required for an interactive console. Unfortunately Console.In assumes end of input when the char buffer is empty instead of upon ctrl-z. I disagree with File.ReadAllLines() as that returns an array, which can be memory hungry with larger files. File.ReadLines() is better since it returns an IEnumerable<string> which can be enumrated lazily. \$\endgroup\$ – Pharap Jul 19 '16 at 11:50
  • 2
    \$\begingroup\$ @Pharap I agree, for large files (or unknown length...) File.ReadLines() is a much better choice! \$\endgroup\$ – Adriano Repetti Jul 19 '16 at 12:22
6
\$\begingroup\$

Starting with your specific questions:

  1. Yes, the class is idiomatically wrapped in a namespace. It's not very important for such a small-scale class as this, but that's the accepted way - the global namespace should only have namespaces in it.
  2. Neither the main class not method need to be public. They're called by the loader, not by other code.
  3. Assuming you're using VS2008 or later, you can use var to shorten the declaration syntax. The type is inferred from the right-hand side of the assignment. var counts = new SortedDictionary();
  4. The more idiomatic way to do the while check would be to remove it from the condition and simply call it twice, once outside the loop and once inside. There's no modern C#ish replacement. Your way is fine too.
  5. TryGetValue or ContainsKey/Get combination are still the way to go, but can be easily wrapped in an Extension Method, which can then be used cleanly: counts[word] = counts.GetOrDefault(word, 0) + 1;
  6. It's a simple, clear loop. No real need to improve it. If you want, you can use string.Join to create one big string and print it out, but there's no real gain.
  7. What would you be using LINQ for here? LINQ is for querying, and you're not doing too much of that here. You could write a method using iterators that returns the next line from the console as an IEnumerable<string>, and call LINQ functions on it, but it won't be clearer or better performing.

And additionally:

  1. One thing I've developed an allergy for over the years is calls to ToUpper/ToLower. In 90% of cases, it's done just to do a case insensitive comparison, which is usually a bad idea - it creates new copies of the string, which means memory and GC pressure. If you don't care about representation, just write a case insensitive regex and keep the original strings intact: Regex.Matches(line, wordRegex, RegexOptions.IgnoreCase); Also, make sure your dictionary is case-insensitive as well, as @t3chb0t reminds me in the comments: SortedDictionary counts = new SortedDictionary(StringComparer.CurrentCultureIgnoreCase);

  2. I'm not sure a a SortedDictionary makes sense here. It has a significantly slower Insert performance than a regular dictionary, and it's not as if you need to access it sorted during insertion. I would create a regular Dictionary<string,int>, and after processing is done, sort it once.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Re LINQ, well maybe not LINQ necessarily, but whatever C# has for processing streams with mapping, filtering, grouping, and ordering. Regarding #8, I want to count words like "For" and "for" as the same. How to do this without lowercasing? \$\endgroup\$ – Ray Toal Jul 19 '16 at 6:51
  • \$\begingroup\$ I've added the case-insensitive Regex line to the answer. \$\endgroup\$ – Avner Shahar-Kashtan Jul 19 '16 at 7:02
  • \$\begingroup\$ And regarding LINQ - I can't think of a LINQ/stream processing version that won't be less readable and more complex than this code. \$\endgroup\$ – Avner Shahar-Kashtan Jul 19 '16 at 7:02
  • \$\begingroup\$ I tried your regex suggestion but in a test file containing one For and two fors I got each version counted. I wanted to get 3 fors. I decided on while ((line = Console.ReadLine()?.ToLower()) != null) Does that look idiomatic? Not sure if it is more or less readable. \$\endgroup\$ – Ray Toal Jul 19 '16 at 7:13
  • 4
    \$\begingroup\$ A case insensitive regex in this case is not enough, you also need a case insensitive dictionary so you should create it with a different constructor and specify the StringComparer. \$\endgroup\$ – t3chb0t Jul 19 '16 at 7:56
1
\$\begingroup\$

The following:

    string line;
    while ((line = Console.ReadLine()) != null)
    {
        line = line.ToLower();
        foreach (Match m in wordRegex.Matches(line))
        {
            String word = m.Value;
            int count;
            counts.TryGetValue(word, out count);
            counts[word] = ++count;
        }
    }

Could be changed to:

for(string line = Console.ReadLine(); line != null; line = Console.ReadLine())
{
        line = line.ToLower();
        foreach (Match m in wordRegex.Matches(line))
        {
            string word = m.Value;
            int count = 0;
            counts.TryGetValue(word, out count);
            counts[word] = count + 1;
        }
}

Note that string should almost always be all lowercase. Note that count + 1 is preferred here because changing the state of the variable count is not the primary intention, the primary intention is the assignment of the value of count + 1 to an entry in counts.

In C#6 you can use the new null-conditional to reduce line = Console.ReadLine(); /*...*/ line = line.ToLower()to line = Console.ReadLine()?.ToLower(). The latter is equivalent to string temp = Console.Readline(); line = (temp == null) ? temp : temp.ToLower();

Other than that I recommend using the Enumerable based techniques mentioned in other answers or at least generalising your solution to the StringReader class rather than aiming it specifically at the Console class. Your solution can then be directed at the standard input stream by using TextReader reader = Console.In; or SolveProblem(Console.In);, and subsequently directed at a given file by using TextReader reader = File.OpenText(path); or SolveProblem(File.OpenText(path));. If you choose a method based on enumerating the lines via an IEnumerable however, note that File.ReadLines(path); returns an IEnumerable<string> that can be enumerated via a foreach.

\$\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.