Increment values in a dictionary

Question

In my continued venture to teach myself C# I came across an exercise using Dictionary. I created a class that has a method. In that method it creates a dictionary with keys/values. A string is passed into that method. If any matching character in that string correlates with a key we increment the value of that key, else we throw an ArgumentException. This is my code an my first real attempt and working with a dictionary in C#.

I am interested to see if there is a better, concise, an/or faster way to do the same task. Following is my code and following that is the test. Test can not be altered. I tried turning foreach into linq without luck so that would be interesting to see that as well if you know how or recommend it.

For more information My Github link to this exercise.

using System;
using System.Collections.Generic;

public static class NucleotideCount
{
    public static IDictionary<char, int> Count(string sequence)
    {
        Dictionary<char, int> Dna = new Dictionary<char, int>()
        {
            {'A', 0},
            {'C', 0},
            {'G', 0},
            {'T', 0}
        };

        foreach (char c in sequence)
        {
            switch (c)
            {
                case 'A': Dna[c]++; break;
                case 'C': Dna[c]++; break;
                case 'G': Dna[c]++; break;
                case 'T': Dna[c]++; break;
                default : throw new ArgumentException("Strand has invalid nucleotides.");
            }
        }

        return Dna;
    }
}

// This file was auto-generated based on version 1.3.0 of the canonical data.

using System;
using System.Collections.Generic;
using Xunit;

public class NucleotideCountTests
{
    [Fact]
    public void Empty_strand()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 0,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count(""));
    }

    [Fact]
    public void Can_count_one_nucleotide_in_single_character_input()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 1,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count("G"));
    }

    [Fact]
    public void Strand_with_repeated_nucleotide()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 0,
            ['C'] = 0,
            ['G'] = 7,
            ['T'] = 0
        };
        Assert.Equal(expected, NucleotideCount.Count("GGGGGGG"));
    }

    [Fact]
    public void Strand_with_multiple_nucleotides()
    {
        var expected = new Dictionary<char, int>
        {
            ['A'] = 20,
            ['C'] = 12,
            ['G'] = 17,
            ['T'] = 21
        };
        Assert.Equal(expected, NucleotideCount.Count("AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC"));
    }

    [Fact]
    public void Strand_with_invalid_nucleotides()
    {
        Assert.Throws<ArgumentException>(() => NucleotideCount.Count("AGXXACT"));
    }
}

Answer 1

There is no need for the switch (c). Instead, check to see if c is in the keys of the dictionary using ContainsKey; if not, throw. After that, unconditionally Dna[c]++;.

if (!Dna.ContainsKey(c))
    throw new ArgumentException("Strand has invalid nucleotides.");
Dna[c]++;

Answer 2

Since you make such posts like this in the spirit of learning, my answer will be in that same spirit.

The Count method has these 3 nit picky issues:

• Variable Dna should be dna as the guidelines are that locally named variables begin with a lowercase letter.

• You do not check for string sequence to be null.

• Initializing the dictionary with expected keys and 0 counts is done here and elsewhere. In the name of DRY (Don't Repeat Yourself), this could be a static property.

Links to research:

.NET Naming Guidelines

Don't Repeat Yourself

My biggest issue is with throwing an exception. I tried researching the test exercise and could not find where this is required. My issue is that if you encounter an exception, all subsequent counting is halted. You have provided a custom nicely-worded exception, but if there was a bad character, an exception would be thrown anyway.

Since the exception part of the exercise seems to be something of your own doing, I would suggest not throwing an exception but rather keep a badCount, while continuing to process all the way through sequence. Once you finish sequence, you can include a console message of $"Strand has {badCount} invalid nucleotides.", but only if badCount was > 0.

It's nice to try these exercises and to learn. While it seems you were unaware of ContainsKey or TryGetValue, your own GitHub repo has the link to IDictionary interace, which shows only a 3 Properties and 4 Methods, so you had the information at your fingertips if only you were curious enough to click.

For the record, I would prefer using TryGetValue over ContainsKey but that is my preference. Either is really fine, but in my own personal bag of tricks, I lean more on TryGetValue.

In the spirit of learning more about dictionaries, here is a smattering of some tips:

• This simple example is of char but if you were dealing with string has the key you can construct a case-insensitive dictionary so that "bob" and "BOB" are considered equal. I leave it to you to look up how to do it, but a hint would be to look at the constructor.

• Oddities may arise if you use binary floating point (Single or Double) as the keys, since these are approximations and not exact values.

• If you only want to deal with the Keys, do read about the Keys property. Most of the time I need to process the keys, I end up using something like var keys = dna.Keys.ToList(); so that my list of keys is an independent copy. For example, I could sort my keys list and then process over them in my sorted order.

UPDATE

The OP asked for an example on using a static dictionary for the sake of DRY-ness. Before I do that, let me touch on a related topic. My original answer said to make it a static property. It equally could be a static method. I am guessing the OP is having troubles with it as a static property because he may be referring to the property repeatedly, rather than creating a local instance.

As a Property

public static IDictionary<char, int> Empty => new Dictionary<char, int>()
{
    {'A', 0},
    {'C', 0},
    {'G', 0},
    {'T', 0}
};

The way to use this is not to refer to Empty over-and-over again, because each call to it will return the same "empty" or initial dictionary. Rather assign it to a variable.

var dna = Empty;

foreach (char c in sequence)
{
    if (dna.TryGetValue(c, out int count))
    {
        dna[c] = ++count;
    }
    else
    { 
        throw new ArgumentException("Strand has invalid nucleotides.");
    }
}

While it could work as a property, I think it would be better to make it a method, and provide a better name. Let's step back even more. Your class NucleotideCount has a Count method. This provides the repeated use Count as in NucleotideCount.Count. I think many developers would be expecting an Int32 or Int64 to be returned from a Count method but to their astonishment they get back an IDictionary object.

My use of the word "astonishment" was quite intentional as this violates the Principle of Least Astonishment. I think there is a better name for the class, but I know there can be a better name for the Count method. I am partial to such methods having an ActionVerb+Noun structure, meaning I prefer to see something start with "Get".

In my own example of the Empty property, it could equally work as a method named GetInitialDictionary. Does it matter? Not that much, except it does emphasis to the developer that they need to get that initial dictionary (as my example has shown, and not call it repeatedly.

For example, on the surface, this looks fine except it's not since you won't be modifying the dictionary. (Granted, calling it Empty and adding to it should be a red flag.)

foreach (char c in sequence)
{
    if (Empty.TryGetValue(c, out int count))
    {
        Empty[c] = ++count;
    }
    else
    { 
        throw new ArgumentException("Strand has invalid nucleotides.");
    }
}

return Empty;

Each call to Empty creates an initial dictionary of 0 counts, so the final return Empty simply returns 0 counts. This confusion of usage could perhaps be mitigated if it were named as a method instead.

public static IDictionary<char, int> GetInitialDictionary() => new Dictionary<char, int>()
{
    {'A', 0},
    {'C', 0},
    {'G', 0},
    {'T', 0}
};

Thanks to the addition of () the property is now a method and perhaps how you should use it is more understood:

var dna = GetIntialDictionary();

Because it would be downright awkward to try using

GetInitialDictionary()[c]++;