Scoring a Scrabble Word

Question

I was asked before an interview to create a console application that will work out the score of a word for the board game scrabble. It worked fine but the feedback I received said:

"there were no comments at all, some strange logic and uses exceptions to handle bad input. In summary the Developers reviewing this felt this was not bad, though seemed a little rushed or inexperienced."

In regards to the comments I found that many places recommend naming variable and method names well enough to not need comments?

Also I was wondering what they meant by the weird logic.

Program.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Scrabble
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Welcome to the Scrabble Calculator!");
            do
            {
                try
                {
                    Console.WriteLine("\nEnter a word:");

                Word NewScrabbleWord = new Word(Console.ReadLine());


                Console.WriteLine("Your score for this word is: {0}\n", NewScrabbleWord.Score);

            }
            catch (Exception all)
            {
                Console.WriteLine(all.Message + "\n");
            }

            Console.WriteLine("Hit q to quit or hit any other key to continue.");
        } while (Console.ReadKey(true).Key != ConsoleKey.Q);
    }

}
}

Word.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
namespace Scrabble
{
    class Word
    {
        public string Value { get; private set; }
        public char[] Letters
        {
            get
            {
                if (!Value.Contains(" "))
                    return Regex.Replace(Value.ToLower(), "[^a-z]+", "").ToArray();
                else
                    throw new Exception("Enter only one word");
            }
        }
        public int Length
        {
            get
            {
                if (Letters.Any())
                    return Letters.Count();
                else
                    throw new Exception("You never entered anything");
            }
        }
        public int Score
        {
            get
            {
                return CalculateScore();
            }
        }

        public Word(string Word)
        {
            this.Value = Word;
        }

        private int CalculateScore()
        {
            int CurrentScore = 0;
            CurrentScore += GetLetterPoints();

            if (BonusPoints()) CurrentScore += 3;

            CurrentScore *= GetMultiplier();

            return CurrentScore;
        }

        private int GetLetterPoints()
        {
            int score = 0;
            foreach (char letter in this.Letters)
            {
                switch (letter)
                {
                    case 'a':
                    case 'e':
                    case 'o':
                    case 's':
                        score += 1;
                        break;
                    case 'c':
                    case 'f':
                    case 'g':
                    case 'i':
                    case 'l':
                    case 'r':
                    case 't':
                    case 'u':
                        score += 2;
                        break;
                    case 'd':
                    case 'h':
                    case 'k':
                    case 'm':
                    case 'n':
                        score += 3;
                        break;
                    case 'j':
                    case 'p':
                    case 'w':
                    case 'y':
                        score += 5;
                        break;
                    case 'q':
                        score += 7;
                        break;
                    case 'v':
                    case 'z':
                        score += 8;
                        break;
                }
            }
            return score;
        }

        private bool BonusPoints()
        {
            for (int i = 0; i < this.Letters.Count(); i++)
            {
                if (i != 0)
                    if (Letters[i - 1] == Letters[i])
                        return true;
            }
            return false;
        }
        private int GetMultiplier()
        {
            if (this.Length <= 4)
                return 1;
            else if (this.Length <= 6)
                return 2;
            else if (this.Length <= 9)
                return 3;
            else
                return 4;
        }
    }
}

Answer 1

Whoa!

Don't do this:

    private bool BonusPoints()
    {
        for (int i = 0; i < this.Letters.Count(); i++)
        {
            if (i != 0)
                if (Letters[i - 1] == Letters[i])
                    return true;
        }
        return false;
    }

instead write it like this

    private bool BonusPoints()
    {
        for (int i = 0; i < this.Letters.Count(); i++)
        {
            if (i != 0 && Letters[i - 1] == Letters[i])
                return true;
        }
        return false;
    }

Your nested if statements without bracing is just asking for trouble, and you don't need to nest these 2 if statements anyway, they can be merged into one conditional statement, like I did there.

Edit: Also stealing from the new Code Review user

You should just start from 1 in your for loop and get rid of that extra iteration and the condition in your if statement. Good Catch @guntanis

You would end up with this

    private bool BonusPoints()
    {
        for (int i = 1; i < this.Letters.Count(); i++)
        {
            if (Letters[i - 1] == Letters[i])
                return true;
        }
        return false;
    }

---

This:

    private int GetMultiplier()
    {
        if (this.Length <= 4)
            return 1;
        else if (this.Length <= 6)
            return 2;
        else if (this.Length <= 9)
            return 3;
        else
            return 4;
    }

Would look better if you just incremented the numbers by 1 and used a less than instead of a less than or equal to.

    private int GetMultiplier()
    {
        if (this.Length < 5)
            return 1;
        else if (this.Length < 7)
            return 2;
        else if (this.Length < 10)
            return 3;
        else
            return 4;
    }

or even like reversed it like this,

    private int GetMultiplier()
    {
        if (this.Length > 9)
            return 4;
        else if (this.Length > 6)
            return 3;
        else if (this.Length > 4)
            return 2;
        else
            return 1;
    }

One less character per condition either way. This doesn't really matter, it just depends on how you want the code interpreted by Humans I guess

---

This bit of code is missing something

    private int GetLetterPoints()
    {
        int score = 0;
        foreach (char letter in this.Letters)
        {
            switch (letter)
            {
                case 'a':
                case 'e':
                case 'o':
                case 's':
                    score += 1;
                    break;
                case 'c':
                case 'f':
                case 'g':
                case 'i':
                case 'l':
                case 'r':
                case 't':
                case 'u':
                    score += 2;
                    break;
                case 'd':
                case 'h':
                case 'k':
                case 'm':
                case 'n':
                    score += 3;
                    break;
                case 'j':
                case 'p':
                case 'w':
                case 'y':
                    score += 5;
                    break;
                case 'q':
                    score += 7;
                    break;
                case 'v':
                case 'z':
                    score += 8;
                    break;
            }
        }
        return score;
    }

It is missing 2 whole letters,

1. X
2. B

You should also have a Default case

Answer 2

There are lots of good points in the answers so far and I'll try not to repeat them. Here are a few additional thoughts on how I would have preferred to see this code written.

---

What concept does the Word class represent? I expect the principle concern of a class called Word to be things having to do with words, but this class seems primarily concerned with computing scrabble scores. But that's not the big problem. The big problem is...

---

Why is this a class at all? Why not simply make a method that takes a string and returns a score? Sure, maybe make that method a static method of a class called ScrabbleRules or some such thing, but the design as it stands seems like the object-orientedness was just thrown in for no good reason. OOP is not an end in itself, it's a means to an end.

---

throw new Exception("Enter only one word");

Wait a minute, that exception is being thrown by the Word class. How does that class know that a word was entered on the console? Remember, the job of this class is to compute scores; you've done a good job here of separating the concern of user interfaces from the concern of scrabble policy, and then the exception message goes and conflates them again.

---

This is a minor and subtle point, but worth making. Don't mutate things that you don't need to mutate. Consider this method body:

    {
        int CurrentScore = 0;
        CurrentScore += GetLetterPoints();
        if (BonusPoints()) 
          CurrentScore += 3;
        CurrentScore *= GetMultiplier();
        return CurrentScore;
    }

CurrentScore is mutated four times. It need not be mutated any times:

    {
        int letterPoints = GetLetterPoints();
        int bonusPoints = GetBonusPoints();
        int multiplier = GetMultiplier();
        return (letterPoints + bonusPoints) * multiplier;
    }

You tell me: which version makes it abundantly clear that the rule of the game is "total points is the sum of the letter and bonus points, all multiplied by the score multiplier", yours or mine?

---

Another minor point: in the point above, notice how in your code, GetLetterPoints and GetMultiplier are made responsible for computing a value, but this code is responsible for computing the bonus? Why is there one method that is smart enough to figure out that there is a bonus, and then requires another method to know that the bonus value should be three? My version is cleaner: the helper methods all compute values.

---

Here we have a method that does too much:

    private int GetLetterPoints()
    {
        int score = 0;
        foreach (char letter in this.Letters)
        {
            switch (letter)
            {
                case 'a':
                   score += ...
            }
        }
        return score;
    }

This would be much cleaner if it were simply

    private int GetLetterPoints()
    {
        int score = 0;
        foreach (char letter in this.Letters)
            score += LetterScore(letter);
        return score;
    }

Or for that matter

    private int GetLetterPoints()
    {
        var query = from letter in Letters select LetterScore(letter);
        return query.Sum();
    }

---

Speaking of that switch statement: you might replace it with a table:

static Dictionary<char, int> LetterScores = new Dictionary<char, int>()
{
    { 'a', 1 },
    { 'b', 3 }, 
    ...
};

And now the method above becomes

    private int GetLetterPoints()
    {
        var query = from letter in Letters select LetterScores[letter];
        return query.Sum();
    }

---

Finally, you asked about comments. I found your code easy to read and understand without any comments. If you have to write a comment to point out a tricky bit of code, better to rewrite it so that it doesn't need the comment. I recommend that comments be used mostly to explain why a piece of code was written the way it was written, not what the code does.

The comments I would want to see in your code would be things like why ToLower is used instead of ToLowerInvariant, or for that matter, why it is being lowered at all. Or comments describing which part of the rules of the game are the responsibility of which parts of the code, if that is anywhere unclear.

Answer 3

Some function could be more short.

In this function the else between the ifs statements is redundant.

        private int GetMultiplier()
        {
            if (this.Length <= 4)
                return 1;
            else if (this.Length <= 6)
                return 2;
            else if (this.Length <= 9)
                return 3;
            else
                return 4;
        }

you could transform it to this shorter version:

        private static int GetMultiplier()
        {
            int[] multi = { 4, 6, 9 }; // this could be a static member.
            int i = 0;
            while (i<multi.Length && Length > multi[i++]) ;

            return i;
        }

Also the branch inside the loop could be avoided just by starting at 1

        private bool BonusPoints()
        {
            for (int i = 1; i < this.Letters.Count(); i++)
            {

                //if (i != 0)
                    if (Letters[i - 1] == Letters[i])
                        return true;
            }
            return false;
        }

You could also improve GetLetterPoints long switch cases are ugly and bad for mantainability among other things.

Answer 4

            try
            {
                Console.WriteLine("\nEnter a word:");

            Word NewScrabbleWord = new Word(Console.ReadLine());


            Console.WriteLine("Your score for this word is: {0}\n", NewScrabbleWord.Score);

        }
        catch (Exception all)
        {
            Console.WriteLine(all.Message + "\n");
        }

Your indentation gets off there, and remains off to the rest of the file. Initially, I thought you were missing a brace. Keep your indentation accurate and consistent to help with readability.

---

Word NewScrabbleWord = new Word(Console.ReadLine());

You can use var there - after all, you just say new Word() in the assignment.

---

ifs without braces are ugly. As mentioned in another answer, this is bad:

if (i != 0)
    if (Letters[i - 1] == Letters[i])
        return true;

It wouldn't be bad in the event you had something like this:

if (i != 0)
    if (Letters[i - 1] == Letters[i])
        certain statements here
else
    // other statements here

Where the else must execute if i = 0, regardless of the inner if. Use your braces.

---

Exceptions here?

if (Letters.Any())
    return Letters.Count();
else
    throw new Exception("You never entered anything");

I would just return 0. After all, nothing is the same as 0, right? In fact, you could probably just return Letters.Count(), unless Letters hasn't been initialized yet, in which case it would throw a NullReferenceException().

Answer 5

Use of exceptions

I'll avoid getting into the "exceptions as flow control" arguments here. However, you can still improve on how you use exceptions.

Don't throw Exception. [1]

You should always create (or use existing) subclasses.

For example:

throw new Exception("Enter only one word");

Should be:

throw new TooManyWordsException();

Alternatively:

// should be in the constructor
throw new ArgumentException("Input should only contain a single word.", "Word");

Corollary: don't catch Exception for flow control. The only place you should ever catch Exception is for last-chance logging before exiting the program, because a catch-all handler can never know which specific exception happened or how to deal with it.

Validate input early. Don't create an object with invalid state.

You currently only throw exceptions when Letters or Length are accessed (or one of the many methods that depend on them!). Ideally, you'd do the validation in the constructor (or wherever else you take input).

public Word(string input)
{
    if (input.Length == 0)
    {
        throw new ArgumentException("Input must not be empty.", "input");
    }

    if (input.Contains(" "))
    {
        throw new ArgumentException("Input should only contain a single word.", "input");
    }

    this.Value = input;
}

If you do this, you won't need a try..catch around every invocation of a potentially-dangerous method.

Notice I also avoid naming the parameter Word, which would become confusing (does Word refer to the class/type or the variable?). Ideally, the Value field would also have a better name (though I can't think of one right now), since Value is rather nondescriptive.

Naming conventions

There's no official naming convention for variable identifiers in C#. However, the most commonly used ([2], [3]) conventions typically distinguish local variables by starting with lowercase (or an underscore).

To be clear: what you're doing isn't wrong, per se, but it is a little bit odd compared to the majority of C# code.

For example:

int CurrentScore = 0;

Should be:

int currentScore = 0;

Answer 6

The BonusPoints method can be rewritten using LINQ:

private bool BonusPoints()
{
    return Letters.Zip(Letters.Skip(1), (x, y) => x == y).Any(x => x);
}

Answer 7

There's a lot of good points in the other answers but I just want to add one thing.

Don't throw exceptions from property getters

It's in the official MS Guidelines for Property Design

If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

So this:

public Word(string Word)
{
   this.Value = Word;
}

public char[] Letters
{
    get
    {
        if (!Value.Contains(" "))
            return Regex.Replace(Value.ToLower(), "[^a-z]+", "").ToArray();
        else
            throw new Exception("Enter only one word");
    }
}

Should be rewritten so that you validate the Word in the constructor. Or better yet, avoid the exception altogether:

string userInput = " ";
while (userInput.Contains(" "))
{
    Console.WriteLine("Please enter a word to score...");
    userInput = Console.ReadLine(); 
}
var word = new Word(userInput);