13
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Not relevant to the code itself, but...I don't think you're using the official Scrabble rules. (Actually it would make sense that you were not given the "correct" rules, because they don't want you to just copy and paste someone else's Scrabble code!) But you might want to clarify the task a little, to avoid confusion. \$\endgroup\$ – mathmandan Jul 2 '15 at 23:55
  • \$\begingroup\$ I like how they said it seemed rushed. Did you do this on site during the interview or post interview? \$\endgroup\$ – Thomas Matthews Jul 3 '15 at 18:02
  • \$\begingroup\$ Thanks for all the reply's guys. I did this pre Interview. It was a simple task so I assumed a simple program would be good enough. \$\endgroup\$ – LiverpoolOwen Jul 3 '15 at 18:13
  • \$\begingroup\$ for goodness sake USE AN ARRAY for the 26 score values. Good grief. \$\endgroup\$ – Fattie Jul 5 '15 at 6:07
  • \$\begingroup\$ Yes well I was going to use a dictionary and have many index's all link to the same number but I couldn't figure out \$\endgroup\$ – LiverpoolOwen Jul 5 '15 at 9:38
17
\$\begingroup\$

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

\$\endgroup\$
  • 2
    \$\begingroup\$ I actually think the <= is clearer, especially if the rules are written something like ("Words up to x characters long have a y multiplier."). < would be clearer if the rules said "Words shorter than x", though I think the former is more common. \$\endgroup\$ – Bob Jul 3 '15 at 3:58
  • \$\begingroup\$ Recommending for (int i = 0; i < this.Letters.Count(); i++) is using the extensions from Linq which will enumerate the entire collection every time the for loop moves to the next item. The correct usage here would be to use the .Length indexer from the Letters array: for (int i = 0; i < this.Letters.Length; i++) Here's an example demonstrating the difference: dotnetfiddle.net/izP5sD \$\endgroup\$ – Metro Smurf Jul 4 '15 at 20:22
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ would you recommend adding XML-comments (/// ...) before the method declarations? Because that is something I'd do, especially if I had to hand it in. \$\endgroup\$ – Armin Jul 4 '15 at 13:17
  • \$\begingroup\$ @Armin: That's a good point. XML doc comments first, as you note, demonstrate that you understand the concept. Second, they are useful to more than just people reading the code, as they also show up in IntelliSense. \$\endgroup\$ – Eric Lippert Jul 4 '15 at 15:59
  • \$\begingroup\$ "which version makes it abundantly clear..." couldn't agree more with you Eric, great answer. To write code is to write self-documenting code. it is utterly trivial to achieve the task at hand. The whole, total, complete concept of "engineering" here is to write it in such a way that it is as clear as sunlight, beyond clear, knock-you-on-the-head clear. Good one. \$\endgroup\$ – Fattie Jul 5 '15 at 6:12
  • \$\begingroup\$ We would love to have your wealth of Knowledge to draw from, just a chat ping away, please consider joining us in The 2nd Monitor from time to time \$\endgroup\$ – Malachi Jul 5 '15 at 17:40
12
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ totally got thrown by the double nest and missed the obvious of starting from 1 instead of 0. good catch \$\endgroup\$ – Malachi Jul 2 '15 at 21:44
  • \$\begingroup\$ The shorter version of the GetMultiplier seems harder to read, would you still suggest it? \$\endgroup\$ – LiverpoolOwen Jul 3 '15 at 18:20
  • 1
    \$\begingroup\$ @LiverpoolCoder: I agree with you; I think your version of the code is easier to understand. If there were a thousand ranges then yes, the table-driven technique is superior, but for fewer than ten, I don't see a compelling benefit. I do however agree with MAG's points about the switch and the loop. \$\endgroup\$ – Eric Lippert Jul 4 '15 at 0:03
  • \$\begingroup\$ Yes the main benefit of my version of GetMuliplier would be having all the multipliers at the top of the class as a static member so it would be easier to scale and modify, but in this particular case you guys may be rigth and readability would be preferable. \$\endgroup\$ – MAG Jul 4 '15 at 1:07
12
\$\begingroup\$
            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().

\$\endgroup\$
  • \$\begingroup\$ I don't understand why you would use var instead of word, could you elaborate? \$\endgroup\$ – LiverpoolOwen Jul 3 '15 at 18:17
  • \$\begingroup\$ Some people use var, and others don't. It isn't really needed when you put ` = new Word()` right after the declaration, because you tell yourself what it is. Either always use it when possible, or never use it - just stay consistent. \$\endgroup\$ – Hosch250 Jul 3 '15 at 18:19
7
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thanks. You mention validating in the constructor and robH seems to have put it in the program main, which would be better of the two? \$\endgroup\$ – LiverpoolOwen Jul 3 '15 at 18:24
  • \$\begingroup\$ @LiverpoolCoder This goes back to the "exceptions as flow control" argument. Basically, if you expect to hit the exception condition often, then you should check the conditions before calling the method (or creating the object, etc.). Exceptions should only be thrown rarely. But if you were designing a reusable class you'd want to also check in the constructor, because you can't guarantee the caller has already checked. In your particular case, you'll find some people prefer one way and some prefer the other. See also: msdn.microsoft.com/en-us/library/ms229030%28v=vs.110%29.aspx \$\endgroup\$ – Bob Jul 3 '15 at 18:45
  • \$\begingroup\$ And also programmers.stackexchange.com/questions/189222/… - to sum up, the constructor should always validate the arguments but best practice would also have the caller perform validation where possible, and maybe even have a TryParseWord or similar that returns a bool indicating success and an out param with the Word. But, again, people make arguments for both sides: programmers.stackexchange.com/questions/107723/… \$\endgroup\$ – Bob Jul 3 '15 at 18:48
6
\$\begingroup\$

The BonusPoints method can be rewritten using LINQ:

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

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);
\$\endgroup\$
  • \$\begingroup\$ Very good point \$\endgroup\$ – Heslacher Jul 3 '15 at 15:16

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.