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

Getting a scrabble score from a word

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

Problem

My program is complete and runs, and gives me the output I expect. For example, if I enter the string "HELLO" I should get 8. Are there any improvements that can be made?

#include "stdafx.h"
#include  
#include  
#include  
using namespace std; 

int gameScore(string word); 
int letterValue(char ch); 

int main()
{
    while(true)
    {
        char word[80]; 
        cout << "Enter a word: "; 
        cin.getline(word, 80); 

        if(word[0] == ' ')
            break; 

        int score = gameScore(word);
        cout << "The score for '" << word << "' is " << score << "\n"; 
    }

    system("PAUSE"); 
    return 0;
}

int gameScore(string word)
{
    int total = 0; 
    int length = word.length(); 

    for(int i = 0; i < length; i++)
    {
        total += letterValue(word[i]); 
    }

    return total; 
}

int letterValue(char ch)
{
    int value = 0; 

    switch(ch)
    {
        case 'A': case 'a': 
        case 'E': case 'e':  
        case 'I': case 'i': 
        case 'L': case 'l': 
        case 'O': case 'o': 
        case 'R': case 'r': 
        case 'S': case 's': 
        case 'T': case 't': 
        case 'U': case 'u': 
            value += 1; break; 
        case 'D': case 'd': 
        case 'G': case 'g': 
            value += 2; break; 
        case 'B': case 'b': 
        case 'C': case 'c': 
        case 'M': case 'm':  
        case 'P': case 'p': 
            value += 3; break; 
        case 'F': case 'f': 
        case 'H': case 'h': 
        case 'V': case 'v': 
        case 'W': case 'w': 
        case 'Y': case 'y': 
            value += 4; break; 
        case 'K': case 'k': 
            value += 5; break; 
        case 'J': case 'j': 
        case 'X': case 'x': 
            value += 8; break; 
        case 'Q': case 'q':
        case 'Z': case 'z': 
            value += 10; break; 
    }
    return value; 
}

Solution

@Ethan's review already mentioned the basic improvements, but I think that there are other things you could improve and experiment with. At the end of this review I've put together a working example of the refactored code, so you can refer to it for all the things I'm mentioning.

A few other nits...

-
When your program consists of a single source file, you might as well avoid declaring function prototypes and put the other functions above main. This will ease maintenance, since it is one less place to update if you change a function name/parameter type.

-
Instead of passing a fixed char array to getline and limiting yourself to 80 chars max, you can pass a std::string to it and allow for arbitrary length words.

-
Make sure to pass the string parameter of gameScore as a const reference. In C++, when you pass an object to a function, it will be copied unless stated otherwise. So in that case, gameScore is seeing its own copy of the word. But you don't need to pay for that copy since the function is only inspecting each character of the string, without modifying it. Enter references. When passing a read-only reference, also make it const (check the example at the end for the details). It is important to note though that this tip applies to objects only (classes, structs. std::string is a class, mind you). Native types like integers and pointers are actually better if passed by value, since the hardware has registers to store them, so copying an int into a function is free.

-
You did right by storing the length of the string in a local variable inside gameScore to avoid calling the method in a loop, but it is actually not a 100% right. Being pedantic, string.length() returns an unsigned integer type. You have a few options here to match the type: C++11 and above, use auto and let the compiler infer the exact type. Older C++, you can use std::string::size_type (if you really love typing ;)) or shorter, a std::size_t, which is equivalent. The only real implication of using int like you currently do is that on some platforms it might be smaller than the size of the type returned by string, so assuming we are dealing with a Gigabytes long string, it could theoretically truncate the value and loose part of the string's length. Not the case with your program, I'm sure, but it is good to be aware of this when writing portable code.

-
Use const on local variables that are only assigned/initialized once. For instance, that int score. When the scope of a variable is small, it makes little difference, but it is good to get in the habit of using it. When looking at a 100 lines long function, you will be happy to know that the variables you are tracking are not going to be changed after they are declared.

An alternate implementation for letterValue:

I didn't really like that switch in letterValue. It is not hard to understand nor unclear, but it just feels too long and verbose...

You could use a Standard map to shorten things up a bit. Map each lower case letter to its integer value, them just lookup the map. If you cast each letter tolower before looking up the map, you don't have to repeat every char for upper and lower case.

Sample implementation:

``
#include
#include
#include
#include

int letterValue(char ch)
{
// This map replaces your switch/case.
// The upside is that you don't have to specify
// upper and lower case letters. The downside is
// that the letter values have to be repeated
// for each entry.
//
// Notice that we made it a static constant, so the map
// doesn't get recreated every time we enter the function.
//
// NOTE: To compile this code, you'll need to
// make sure C++11 support is enabled in your
// compiler (should be fine by default with recent
// versions of Visual Studio).
//
// Also notice that your solution is missing the entry
// for 'n', as was pointed out by Edward in his answer.
//
static const std::map charValues = {
{'a',1}, {'e',1}, {'i',1}, {'l',1}, {'o',1},
{'r',1}, {'s',1}, {'t',1}, {'u',1}, {'n',1},
{'d',2}, {'g',2}, {'b',3}, {'c',3}, {'m',3},
{'p',3}, {'f',4}, {'h',4}, {'v',4}, {'w',4},
{'y',4}, {'k',5}, {'j',8}, {'x',8}, {'q',10},
{'z',10}
};

// Since you want to return 0 for a non-mapped letter
// we first find() it, then check the result. The
? :
// thingy is the ternary operator, in case you are not
// familiar with it. It's just like a compact version
// of an if then else...
//
auto iter = charValues.find(std::tolower(ch));
return (iter != std::end(charValues)) ? iter->second : 0;
}

int gameScore(const std::string & word) // <= Here you can see the const
// reference, to avoid copying the string. The
&` means reference.
{
// Notice the const here to enforce one-time initialization.
const std::size_t length = word.length();

Code Snippets

#include <iostream>
#include <cctype>
#include <string>
#include <map>

int letterValue(char ch)
{
    // This map replaces your switch/case.
    // The upside is that you don't have to specify
    // upper and lower case letters. The downside is
    // that the letter values have to be repeated 
    // for each entry.
    //
    // Notice that we made it a static constant, so the map
    // doesn't get recreated every time we enter the function.
    //
    // NOTE: To compile this code, you'll need to
    // make sure C++11 support is enabled in your
    // compiler (should be fine by default with recent
    // versions of Visual Studio).
    //
    // Also notice that your solution is missing the entry 
    // for 'n', as was pointed out by Edward in his answer.
    //
    static const std::map<char, int> charValues = {
        {'a',1}, {'e',1}, {'i',1}, {'l',1}, {'o',1},
        {'r',1}, {'s',1}, {'t',1}, {'u',1}, {'n',1},
        {'d',2}, {'g',2}, {'b',3}, {'c',3}, {'m',3},
        {'p',3}, {'f',4}, {'h',4}, {'v',4}, {'w',4},
        {'y',4}, {'k',5}, {'j',8}, {'x',8}, {'q',10}, 
        {'z',10}
    };

    // Since you want to return 0 for a non-mapped letter
    // we first find() it, then check the result. The `? :`
    // thingy is the ternary operator, in case you are not
    // familiar with it. It's just like a compact version
    // of an if then else...
    //
    auto iter = charValues.find(std::tolower(ch));
    return (iter != std::end(charValues)) ? iter->second : 0;
}

int gameScore(const std::string & word) // <= Here you can see the const 
         // reference, to avoid copying the string. The `&` means reference.
{
    // Notice the const here to enforce one-time initialization.
    const std::size_t length = word.length();

    int total = 0;
    for (std::size_t i = 0; i < length; i++)
    {
        total += letterValue(word[i]);
    }

    return total;
}

int main()
{
    while (true)
    {
        // Using a string instead of a fixed size array.
        std::string word;
        std::cout << "Enter a word: ";
        std::getline(std::cin, word);

        // std::string has this handy method to test if it is... Empty!
        if (word.empty())
        {
            break;
        }

        const int score = gameScore(word);
        std::cout << "The score for '" << word << "' is " << score << "\n";
    }

    std::cin.get();
}

Context

StackExchange Code Review Q#102258, answer score: 11

Revisions (0)

No revisions yet.