1
\$\begingroup\$

I have a Scrabble game program which takes two players' words and calculates who has the winning score.

Whichever player has the highest score is the winner; there's a tie if the scores are the same.

It all seems to work how I have written it; however I am sure it can be done better. Just wondering if anyone could give me some ideas on a more efficient way to write this?

#include <ctype.h>
#include <cs50.h>
#include <stdio.h>
#include <string.h>

// Points assigned to each letter of the alphabet
int points[] = {1, 3, 3, 2, 1, 4, 2, 4, 1, 8, 5, 1, 3, 1, 1, 3, 10, 1, 1, 1, 1, 4, 4, 8, 4, 10};


int compute_score(string word);

int main(void)
{

    // Get input words from both players
    string word1 = get_string("Player 1: ");
    string word2 = get_string("Player 2: ");

    // Score both words
    int score1 = compute_score(word1);
    int score2 = compute_score(word2);

    // TODO: Print the winner
    if (score1 < score2)
    {
        printf("Player 2 wins!...");
    }
    else if (score1 == score2)
    {
        printf("Tie!");
    }
    else
    {
        printf("Player 1 wins!...");
    }
}

int compute_score(string word)
{
    // TODO: Compute and return score for string
char letters[26];
int asciiletter = 97;
int score = 0;
int i = 0;
int k = 0;


    for (i = 0; i < 26; i++)
    {
        letters[i] = asciiletter + i; 
        
        
    }

    
    //Get the chars in the string of word and compare to all letters in alphabet letters[26]
    for(int j= 0, n = strlen(word); j < n; j++)
    {
        //if entered values uppercase, convert to lower as points are the same for either case
        word[j] = tolower(word[j]);
        for(k = 0; k < 26; k++ )
        {
                if (word[j] == letters[k])
                {
                    score = score + points[k];
                }
        }
    

    }
    return score;
}


\$\endgroup\$
2
  • \$\begingroup\$ I compiled and executed your code: 1) the code did not cleanly compile. Always enable the warnings, then fix those warnings. 2) I ran your code with inputs of rich and chucj the result is the statement: Player 2 wins!... That seems a bit odd. Suggest: first, display the scrabble board second, instruct the user on what they are expected to input \$\endgroup\$ Apr 3, 2021 at 8:26
  • \$\begingroup\$ regarding: for(int j= 0, n = strlen(word); j < n; j++) the function: strlen() returns a size_t (unsigned long) but this is comparing that to a int (which is signed) ... Not a good idea \$\endgroup\$ Apr 3, 2021 at 8:32

3 Answers 3

3
\$\begingroup\$

and welcome to C programming! Also, to Code Review.

Style

There are a couple of "style" issues with your code. The most obvious one is that your indentation is not consistent. I don't know if that's due to pasting it into the browser, or if it appears that way in your code. But computescore needs to be cleaned up.

Next is an issue of "comprehensibility". You have this array:

// Points assigned to each letter of the alphabet
int points[] = {1, 3, 3, 2, 1, 4, 2, 4, 1, 8, 5, 1, 3, 1, 1, 3, 10, 1, 1, 1, 1, 4, 4, 8, 4, 10};

Quick, how many points for 'S'?

This array might possibly be correct, but it's not comprehensible and it's not modifiable. It would be much better to provide some other mechanism to initialize the array, or initialize some other structure, such that the next guy to maintain the code has a chance of seeing bugs and/or being able to make changes.

For example:

  1. You might create an enum, with point values for each letter, then initialize the array using the enum labels:

    enum {
        A_POINTS = 1,
        B_POINTS = 3,
        C_POINTS = 3,
        :
    };
    
    int points[] = {
        A_POINTS,
        B_POINTS,
        C_POINTS,
        :
    };
    
  2. You might create a clever designated initializer macro that makes things more clear:

    #define TILE(CH) [CH - 'a']
    
    int points[] = {
        TILE('a') = 1,
        TILE('b') = 3,
        TILE('c') = 3,
        :
    };
    
  3. You might consider building a string and parsing it at runtime, like:

    const char * points = 
         "A = 1; B = 3; C = 3; ...;"
         "N = ..."
         ;
    

Be aware: the point of this is purely to make it easy to understand the mapping between letters and points. Ideally, you want to make it easy enough to understand that someone could look at the code and catch a mistake before you compiled it and went to testing.

Magic

Your scoring function searches through a list of characters in order to obtain the index of a particular character. Never do that! You know the index, it's just hidden behind a thin layer of mathematics.

In ASCII, the upper and lowercase letters (also, digits) are runs of encoding values assigned in consecutive, increasing order. Once you know where the start of the run is, you can get to any other character by simple addition. Likewise, you can reverse that trick using simple subtraction:

    score_index = ch - 'a';
    score += points[score_index];
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your comments. A lot of great points for me to revisit. I felt like I was over complicating how I dealt with the ascii chars and your comments help that. \$\endgroup\$
    – Js94
    Apr 2, 2021 at 20:07
0
\$\begingroup\$

Bug - tolower()

    word[j] = tolower(word[j]);

We need to be careful with the functions in <ctype.h>. They generally accept int parameters that hold unsigned representations of characters; the only negative value that may be passed is EOF. So a robust program always converts any char value to unsigned char before allowing it to be promoted to int:

    word[j] = tolower((unsigned char)word[j]);

In practice, you're unlikely to hit this bug if users enter only valid English Scrabble words, on systems using ASCII character coding. Which nicely introduces my two main points:

Character coding

This code assumes the host character set is ASCII, or at least includes ASCII as a subset. That might be acceptable, but we can minimise the code that needs to change when we need to compile for a different platform. Firstly, we could add a big fat warning, to alert the developer that work is required:

#if 'a' != 97
#error compute_score() requires ASCII platform
#endif

Where we use literal 97, we would be better using the character literal 'a' - this also helps show the reader our intent here:

int asciiletter = 'a';

Though I'd get rid of that completely, and convert the loop that depends on letters being consecutive:

for (i = 0; i < 26; i++)
{
    letters[i] = asciiletter + i;
}

into a simple string that's independent of character coding:

static const char *letters = "abcdefghijklmnopqrstuvwxyz";

Having done that, there's no longer any need for the #if/#warning I proposed earlier.

Languages

Although we have provided some flexibility by storing the tile point-values in a table, our code is tied to English, notably in this line:

char letters[26];

Other languages have fewer (e.g. Irish) or more (e.g. Swedish) than 26 different tiles.

One possibility for dealing with the differences is to ask a function for the letter score, and to allow different functions to be swapped in, using a function pointer:

typedef int(*tile_score)(char);

int tile_score_english(char c)
{
    switch (tolower((unsigned char)c)) {
    case 'a': case 'e': case 'i': case 'o': case 'n':
    case 'r': case 't': case 'l': case 's': case 'u':
        return 1;
    case 'd': case 'g':
        return 2;
    case 'b': case 'c': case 'm': case 'p':
        return 3;
    case 'f': case 'h': case 'v': case 'w': case 'y':
        return 4;
    case 'k':
        return 5;
    case 'j': case 'x':
        return 8;
     case 'q': case 'z':
        return 10;
    }
    return 0;
}

int tile_score_french(char c)
{
    switch (tolower((unsigned char)c)) {
    case 'a': case 'e': case 'i': case 'o': case 'n':
    case 'r': case 't': case 'l': case 's': case 'u':
        return 1;
    case 'd': case 'm': case 'g':
        return 2;
    case 'b': case 'c': case 'p':
        return 3;
    case 'f': case 'h': case 'v':
        return 4;
    case 'j': case 'q':
        return 8;
    case 'k': case 'w': case 'x': case 'y': case 'z':
        return 10;
    }
    return 0;
}

⋮

Then we simply pass the language's score function when we want the total score:

int compute_score(string word, tile_score distribution)
{
    int score = 0;
    for (char *p = word;  *p;  ++p) {
        score += distribution(*p);
    }
    return score;
}

As a side benefit, good compilers will turn the switch/case into a simple lookup, which will be more efficient than the linear search we currently have.

We still have some work to do for languages such as Welsh, which have diphthong tiles (DD, FF, TH, CH, LL, NG, RH). I'll leave handling those as an exercise for the reader!

\$\endgroup\$
0
\$\begingroup\$

The simple points array just needs two newlines:

int points[] = {1, 3, 3, 2, 1, 4, 2, 4, 1, 8,   //10 
                5, 1, 3, 1, 1, 3,10, 1, 1, 1,   //20
                1, 4, 4, 8, 4,10};              //26

This is not super user-friendly, but also not the opposite.

How to quite directly map the ASCII to the index has already been answered ("Magic").

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.