5
\$\begingroup\$

I recently took a ~three month break from professional software development and am feeling a bit rusty. I thought a good way to ease back into things before looking for a job would be to take a swing at one of the excellent coding exercises that The Guardian use in their interview process (GitHub link). I attempted the Scrabble exercise (GitHub link) using .NET Core. So far I've only attempted the first task on the exercise:

Calculate the score for a word. The score is the sum of the points for the letters that make up a word. For example: GUARDIAN = 2 + 1 + 1 + 1 + 2 + 1 + 1 + 1 = 10.

ScoreCalculator.cs

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

namespace GuardianPairingExercises.Scrabble.Core
{
    public sealed class ScoreCalculator
    {
        private static readonly IReadOnlyDictionary<int, char[]> _pointsMapping
            = new Dictionary<int, char[]>
            {
                { 1, new[] { 'E', 'A', 'I', 'O', 'N', 'R', 'T', 'L', 'S', 'U' } },
                { 2, new[] { 'D', 'G' } },
                { 3, new[] { 'B', 'C', 'M', 'P' } },
                { 4, new[] { 'F', 'H', 'V', 'W', 'Y' } },
                { 5, new[] { 'K' } },
                { 8, new[] { 'J', 'X' } },
                { 10, new[] { 'Q', 'Z' } }
            };

        private static IReadOnlyDictionary<char, int> LetterValuesMapping { get; }
            = CreateLetterValuesMapping();

        public int CalculateScore(string word)
        {
            if (!Regex.IsMatch(word, "^[a-zA-Z]+$"))
            {
                throw new ArgumentException(nameof(word));
            }

            var uppercaseWord = word.ToUpper(); // Normalize the case.
            var totalScore = 0;
            foreach (var letter in uppercaseWord)
            {
                totalScore += LetterValuesMapping[letter];
            }

            return totalScore;
        }

        private static IReadOnlyDictionary<char, int> CreateLetterValuesMapping()
        {
            var letterValuesMapping = new Dictionary<char, int>();
            foreach ((int points, char[] letters) in _pointsMapping)
            {
                foreach (var letter in letters)
                {
                    letterValuesMapping[letter] = points;
                }
            }

            return letterValuesMapping;
        }
    }
}

ScoreCalculatorTests.cs

using System;
using System.Collections;
using System.Collections.Generic;
using GuardianPairingExercises.Scrabble.Core;
using Xunit;

namespace GuardianPairingExercises.Scrabble.Tests
{
    public class ScoreCalculatorTests
    {
        [Theory]
        [ClassData(typeof(LetterScoresTestData))]

        public void CalculateScore_WordContainsASingleValidLetter_ReturnsTheScore(
            string word,
            int expectedScore)
        {
            // Arrange & Act
            var actualScore = new ScoreCalculator().CalculateScore(word);

            // Assert
            Assert.Equal(expectedScore, actualScore);
        }

        [Fact]
        public void CalculateScore_WordContainsOnlyUppercaseLetters_ReturnsTheScore()
        {
            // Arrange & Act
            var actualScore = new ScoreCalculator().CalculateScore("GUARDIAN");

            // Assert
            Assert.Equal(10, actualScore);
        }

        [Fact]
        public void CalculateScore_WordContainsOnlyLowercaseLetters_ReturnsTheScore()
        {
            // Arrange & Act
            var actualScore = new ScoreCalculator().CalculateScore("guardian");

            // Assert
            Assert.Equal(10, actualScore);
        }

        [Fact]
        public void CalculateScore_WordContainsInvalidCharacters_ThrowsArgumentException()
        {
            // Arrange
            var calculator = new ScoreCalculator();

            // Act & Assert
            Assert.Throws<ArgumentException>(() => calculator.CalculateScore("!"));
        }

        public class LetterScoresTestData : IEnumerable<object[]>
        {
            private static readonly IReadOnlyDictionary<int, char[]> _pointsMapping
                = new Dictionary<int, char[]>
                {
                    { 1, new[] { 'E', 'A', 'I', 'O', 'N', 'R', 'T', 'L', 'S', 'U' } },
                    { 2, new[] { 'D', 'G' } },
                    { 3, new[] { 'B', 'C', 'M', 'P' } },
                    { 4, new[] { 'F', 'H', 'V', 'W', 'Y' } },
                    { 5, new[] { 'K' } },
                    { 8, new[] { 'J', 'X' } },
                    { 10, new[] { 'Q', 'Z' } }
                };

            public IEnumerator<object[]> GetEnumerator()
            {
                foreach ((int points, char[] letters) in _pointsMapping)
                {
                    foreach (var letter in letters)
                    {
                        yield return new object[] { letter.ToString(), points };
                    }
                }
            }

            IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
        }
    }
}

Questions

  1. Is my approach of storing the point values associated with each letter in a map and performing a lookup preferable to the obvious approach of using a switch statement, or am I over-engineering this?
  2. Is there anything unusual about the formatting?
  3. Any other comments?

Edit: Edited to use an initializer for the LetterValuesMapping auto property on the ScoreCalculator class after reading this excellent SO post.

\$\endgroup\$
3
\$\begingroup\$

I'd be interested to hear why you chose xunit over the more obvious NUnit or MSTest. I tend to avoid data driven tests as the syntax is generally a bit clunky and it always seems to get picked on in review. You could have just done this within a vanilla test and dispensed with the data class seeing as you're only using it once. I used MSTest for these examples, but I'm sure you'll get the idea:

[TestMethod]
public void CalculateScore_WordContainsASingleValidLetter_ReturnsTheScore()
{
    var dict = new Dictionary<int, char[]>
           {
                { 1, new[] { 'E', 'A', 'I', 'O', 'N', 'R', 'T', 'L', 'S', 'U' } },
                { 2, new[] { 'D', 'G' } },
                { 3, new[] { 'B', 'C', 'M', 'P' } },
                { 4, new[] { 'F', 'H', 'V', 'W', 'Y' } },
                { 5, new[] { 'K' } },
                { 8, new[] { 'J', 'X' } },
                { 10, new[] { 'Q', 'Z' } }
           };

    int expectedScore;

    foreach (var kvp in dict)
    {
        expectedScore = kvp.Key;

        foreach (var letter in kvp.Value)
        {
            // Arrange & Act
            var actualScore = new ScoreCalculator().CalculateScore(letter.ToString());

            // Assert
            Assert.AreEqual(expectedScore, actualScore);
        }
    }
}

Another thing, is that I normally have the test data and expected outcomes as constants rather than inline literals. It makes the code more readable. Also, with MSTest & NUnit, you can supply a hint when the test fails in the form of a message. Again, using constants will help keep the code more DRY here:

[TestMethod]
public void CalculateScore_WordContainsOnlyUppercaseLetters_ReturnsTheScore()
{
    const int expectedScore = 10;
    const string wordUnderTest = "GUARDIAN";

    // Arrange & Act
    var actualScore = new ScoreCalculator().CalculateScore(wordUnderTest);

    // Assert
    Assert.AreEqual(expectedScore, actualScore, $"Expected the score for {wordUnderTest} to be {expectedScore}");
}

Finally, a common test (as well as invalid input), is the empty string:

[TestMethod]
public void CalculateScore_WordContainsNoCharacters_ThrowsArgumentException()
{
    // Arrange
    var calculator = new ScoreCalculator();

    // Act & Assert
    Assert.ThrowsException<ArgumentException>(() => calculator.CalculateScore(""));
}

Other than that, it all looks in great shape.

\$\endgroup\$
8
  • \$\begingroup\$ Hi @Robbie, thanks for taking the time to look over my code, much appreciated :). Re my choice of using xUnit - that's interesting, I thought that xUnit was considered the 'standard' unit testing framework to use with .NET Core, but now you've made me re-think. Taking a look at docs.microsoft.com/en-us/dotnet/core/testing it seems Microsoft are touting xUnit, NUnit and MSTest with no clear recommendation on which is 'best'. Are there some docs somewhere I'm missing that point towards recommending NUnit/MSTest over xUnit? \$\endgroup\$ Jan 2 at 11:55
  • \$\begingroup\$ I definitely agree that the syntax of my data driven test is pretty ugly and I think your test reads better. For whatever reason I tend to shy away from using assertions inside a loop when writing UTs, I had this down as an anti-practise in my head, but I can't back it up with any resources so I'll have to re-assess this. Moving the test data and expected outcomes to constants is a definite improvement, and yes shame on me for missing the empty string! Out of interest do you have any thoughts on the merits of using the Dictionary vs a switch statement in ScoreCalculator? \$\endgroup\$ Jan 2 at 11:59
  • \$\begingroup\$ You tend to coalesce on a framework over time that suits your needs. NUnit used to be streets ahead of MSTest but MSTest has caught up a bit recently. I think initially it is enough to just appreciate that there are different frameworks out there. Modern build pipelines are agnostic anyway so you can even mix and match to a degree. \$\endgroup\$
    – Robbie Dee
    Jan 2 at 12:06
  • \$\begingroup\$ Data driven tests are really a style choice. You just need to be sure that you are testing the same functionality in that test. N.B. you have the same data structure defined in the CUT and unit test so this could be made more DRY - perhaps by moving it to another module or exposing it. \$\endgroup\$
    – Robbie Dee
    Jan 2 at 12:11
  • \$\begingroup\$ If this data structure was shared, then it would simply be a Range test (in NUnit speak) and you'd just make sure you got the same score from said structure. \$\endgroup\$
    – Robbie Dee
    Jan 2 at 12:14
2
\$\begingroup\$

These excercises are usually just about getting the right solution so there is rarely enough time to craft some art but if you like to play with the code you could make everything linqy and case-insensitive.

Starting with the Regex and adding the RegexOptions.IgnoreCase. You then can remove the A-Z part.

Regex.IsMatch(word, "^[a-z]+$", RegexOptions.IgnoreCase)

The score could be calculated with the Aggregate extension:

return word.Aggregate(0, (points, letter) => points + LetterValuesMapping[letter.ToString()]);

For this to work you make the point mapping dictionary case-insensitive by specifying a StringComparer and turning the two nested loops into a single compact query:

private static Dictionary<string, int> CreateLetterValuesMapping()
{
    var mappings =
        from item in _pointsMapping
        let points = item.Key
        let letters = item.Value
        from letter in letters
        select (letter, points);


    return mappings.ToDictionary(x => x.letter.ToString(), x => x.points, StringComparer.InvariantCultureIgnoreCase);
}

I also have a sligthly different opinion about the unit-test framework and picking xUnit was in my view the right choice. In my experience it's the fastest one, has the most helpful error messages and supports dependency injection via fixtures. I'd never again use MSTest. It's performance and test discovery are terrible.


Of alls the unit-test naming conventions I prefer the feature-orientend naming convention the most (I cannot find the link to it right now). With this convention you express what your code can and cannot do. I find this is more intuitive and contains less irrelevant noise like implementation details. When something goes wrong you instantly know exactly which feature needs adjusting. With the other convention it's not that obvious .

Example:

        [Theory]
        [ClassData(typeof(LetterScoresTestData))]
        public void Can_calculate_score_for_single_letter(string word, int expectedScore)
        {
            ...
        }

        [Fact]
        public void Can_calculate_score_for_upper_case_letters()
        {
            ...
        }

        [Fact]
        public void Can_calculate_score_for_lower_case_letters()
        {
            ...
        }

        [Fact]
        public void Throws_when_invalid_characters()
        {
            ...
        }

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