patterncsharpModerate
Scoring a Scrabble Word
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:
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
{
"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:
instead write it like this
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
This:
Would look better if you just incremented the numbers by 1 and used a less than instead of a less than or equal to.
or even like reversed it like this,
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
It is missing 2 whole letters,
You should also have a Default case
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.