HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Scoring a Scrabble Word

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scoringwordscrabble

Problem

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
{

Solution

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,

  • X



  • B



You should also have a Default case

Code Snippets

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

Context

StackExchange Code Review Q#95615, answer score: 17

Revisions (0)

No revisions yet.