11
\$\begingroup\$

I've written some code that uses a SortedDictionary to count the number of occurrences of a character in a string.

How can I improve this code? What should I be doing differently? Any advice at all would be helpful.

    static void Main(string[] args)
    {
        string longText = @"The quick brown fox jumps over the lazy dog";

        var count = CharacterCount.Count(longText);

        foreach (var character in count)
        {
            Console.WriteLine("{0} - {1}", character.Key, character.Value);
        }
    }

CharacterCount class:

class CharacterCount
{
    public static SortedDictionary<char, ulong> Count(string stringToCount)
    {
        SortedDictionary<char, ulong> characterCount = new SortedDictionary<char, ulong>();

        foreach (var character in stringToCount)
        {
            if (!characterCount.ContainsKey(character))
            {
                characterCount.Add(character, 1);
            }
            else
            {
                characterCount[character]++;
            }
        }

        return characterCount;
    }

Here is the output:

enter image description here

\$\endgroup\$
13
\$\begingroup\$

You could do the same thing with LINQ:

var counts = longText.GroupBy(c => c) // put each character into a "bucket"
                     // order the buckets alphabetically
                     .OrderBy(c => c.Key);
                     // then convert to dictionary where key = character, value = count
                     .ToDictionary(grp => grp.Key, grp => grp.Count()) 

For convenience/readability, you could turn this into an extension method on string:

static class Extensions 
{
    public static Dictionary<char, int> CharacterCount(this string text)
    {
        return text.GroupBy(c => c)
                   .OrderBy(c => c.Key)
                   .ToDictionary(grp => grp.Key, grp => grp.Count());
    }
}

And invoke it like this:

var longText = @"The quick brown fox jumps over the lazy dog";

var counts = longText.CharacterCount();

However, note that the above doesn't produce a sorted dictionary in the sense that if you were to add another key/value to it afterwards, the order would no longer be maintained. You could re-sort at that point, or use this SortedDictionary constructor to create a sorted dictionary.

\$\endgroup\$
  • 1
    \$\begingroup\$ Could, yes. Should... not so sure. \$\endgroup\$ – Ben Aaronson Sep 25 '14 at 15:29
  • \$\begingroup\$ @BenAaronson As is usually the case with LINQ. :) In practice, I think this would be a fine approach - less code, fewer potential bugs, etc. without any serious drawbacks. \$\endgroup\$ – Adam Lear Sep 25 '14 at 15:31
  • \$\begingroup\$ I don't think there is any point calling OrderBy before ToDictionary since the latter is not guaranteed to preserve order. \$\endgroup\$ – mjolka Sep 25 '14 at 22:32
  • \$\begingroup\$ @mjolka It does in practice now. Could definitely change down the road. In retrospect, just calling ToDictionary and shoving that into SortedDictionary would probably be better. Or just do the ordering at output time. \$\endgroup\$ – Adam Lear Sep 25 '14 at 22:33
  • \$\begingroup\$ You're right! I'd still prefer swapping the order though (and hence type); code is just as clear and we get the guarantees we want: chars.GroupBy(c => c).ToDictionary(grp => grp.Key, grp => grp.Count()).OrderBy(c => c.Key). \$\endgroup\$ – mjolka Sep 25 '14 at 23:14
5
\$\begingroup\$

You could get rid of the count variable, as it isn't really needed here and just adds clutter to the main method, you would just call the .Count method inside the foreach declaration.

so this:

static void Main(string[] args)
{
    string longText = @"The quick brown fox jumps over the lazy dog";

    var count = CharacterCount.Count(longText);

    foreach (var character in count)
    {
        Console.WriteLine("{0} - {1}", character.Key, character.Value);
    }
}

becomes this

static void Main(string[] args)
{
    string longText = @"The quick brown fox jumps over the lazy dog";

    foreach (var character in CharacterCount.Count(longText))
    {
        Console.WriteLine("{0} - {1}", character.Key, character.Value);
    }
}
\$\endgroup\$
4
\$\begingroup\$

You use ulong in your SortedDictionary<T,TK>, this represents a max value of 18,446,744,073,709,551,615, which seems a little overkill for your situation. I'm pretty sure an int would be enough, as the max value is 2,147,483,647.

Also, your method being named Count, it is difficult to assume that it is going to return a SortedDictionary<char,ulong>. I'd think it would return an int, as all the methods named Count in the .Net Framework. You could name it GetNumberOfOccurencePerCharacter, otherwise it seems like Count will return the number of character in the string instead of the number of times each character appears in the string.

\$\endgroup\$
4
\$\begingroup\$

There's one fundamental thing that this code doesn't handle. While you may have chosen this limitation on purpose, a lot of people aren't quite aware of this so I'll bring it up. There's a fundamental difference between "character" and char. Your question and code treats them as the same thing. Which may be fine for your purposes, but won't handle all the possible characters in Unicode.

In Unicode, what people typically think of as a character is called a grapheme. Each grapheme is represented by one or more sequential char values. Some graphemes even have multiple possible representations. It's actually really straightforward in .NET to separate a string into its graphemes by using StringInfo.GetTextElementEnumerator. But this only handles the first caveat. The second caveat (multiple representations) can be handled with String.Normalize.

I believe these calls can be combined with the LINQ approach suggested by Anna Lear, but I haven't verfied by putting it together just yet.

(See also my related answer on a similar question.)

\$\endgroup\$
2
\$\begingroup\$

Although this question is old, no one mentioned that using ContainsKey() together with the getter of the Item property of a Dictionary<TKey, TValue> should be replaced by a call to TryGetValue() which is faster because the check if the key exists is done by the Item getter too.

Internally these three methods are calling the FindEntry() method to check wether a given key exists. So calling this method only once through the TryGetValue() method should be the way to go like so

public static SortedDictionary<char, int> Count(string stringToCount)
{
    SortedDictionary<char, int> characterCount = new SortedDictionary<char, int>();

    foreach (var character in stringToCount)
    {
        int counter = 0
        characterCount.TryGetValue(character, out counter);
        characterCount[character] = counter +1;
    }

    return characterCount;
}

I have changed the type of TValue of the dictionary to int because that's sufficient.

\$\endgroup\$
  • \$\begingroup\$ @CodesInChaos somehow your edit which would have been good got rejected. I noticed it just this morning so I couldn't accept it. Thanks, good catch about the int vs. ulong. \$\endgroup\$ – Heslacher Feb 5 '16 at 5:02

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