6
\$\begingroup\$

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 <iostream> 
#include <cctype> 
#include <string> 
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; 
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Are you aware you keep repeating the same mistakes over and over in your questions? \$\endgroup\$ – Mast Aug 29 '15 at 23:39
  • 1
    \$\begingroup\$ @Mast I keep using #include <iostream> and using namespce std because my professor requires them in the program codes that are turned in. \$\endgroup\$ – Duck Aug 29 '15 at 23:41
  • 2
    \$\begingroup\$ @Duck Just because this is a beginning course doesn't mean it's fine to get into bad habits. \$\endgroup\$ – Ethan Bierlein Aug 29 '15 at 23:46
  • 2
    \$\begingroup\$ You post your code here, which indicates you want your code to be good code. If your professor won't allow it, what is the point of us writing answers? \$\endgroup\$ – Mast Aug 29 '15 at 23:46
  • 2
    \$\begingroup\$ I apologize if the methods in my codes are not the correct way to do things, but this is the layout the professor has given. Although these are bad habits, they are convenient for the moment. The choice to provide a response or answer is yours. I appreciate the feedback I get. \$\endgroup\$ – Duck Aug 29 '15 at 23:49
11
\$\begingroup\$

@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 <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();
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Nice review! I think you deserve the checkmark. :-) \$\endgroup\$ – Ethan Bierlein Aug 30 '15 at 13:55
  • \$\begingroup\$ Every time letterValue is called, charValues is created. Every time letterValue ense, charValues is destroyed. (I think). I think it would be better to move this outside of any functions and to the top of the code. That way, this map will be created at compile time which will make accessing it a lot faster. \$\endgroup\$ – SirPython Aug 30 '15 at 17:34
  • 2
    \$\begingroup\$ @SirPython, The map is a static instance, so it will only be created once the function is entered the first time, lasting until the program quits. That's more or less equivalent of having it as a global, but with the benefit that the name remais at function-level scope. C++11 also guarantees that the first initialization is thread-safe. This answer summarises well the several meanings of static in C++: stackoverflow.com/a/6223689/1198654 \$\endgroup\$ – glampert Aug 30 '15 at 17:48
  • \$\begingroup\$ You're missing an 'n' in the charValues map :) Otherwise, awesome solution. \$\endgroup\$ – DeeKayy90 May 30 '16 at 11:52
  • 1
    \$\begingroup\$ @DeeKayy90, right, good point. I just changed the OP's code and didn't catch that. Luckily, Edward's answer already found that error. Edited my code to include the fix ;) \$\endgroup\$ – glampert May 30 '16 at 18:58
8
\$\begingroup\$

Remove this line:

using namespace std;

This is a horrible, awful habit to get into. So much can go wrong with this line involving name conflicts. After removing this, you should prefix members of the std namespace with std::. See this Stackoverflow question for more detail.

This line:

#include "stdafx.h"

If you aren't developing an MS-specific app, and you don't need better compile time, especially in something as small as this, this line should be removed.

This line:

system("PAUSE");

Is another bad habit to get into. This line of code is not cross-platform, not optimal, and somewhat insecure as well. Preferably, if you need to see the output, you should do something like this:

std::cin.get();

This line:

return 0;

Can be removed from main. Again, main is a special function, so the above line will be automatically inserted.

Finally, you should include a default case in your letterValue function. For example, what happens if the user enters a character like $? To do this, simply add this to the end of your switch statement:

default:

By doing this, you can also remove all the other empty cases.

\$\endgroup\$
  • \$\begingroup\$ just a quick question. Is there any reason to use std::cin.get() over getc() in cstdio? \$\endgroup\$ – an earwig Aug 30 '15 at 2:24
  • \$\begingroup\$ @James_Parsons std::cin.get() doesn't require you to include <cstdio>, and it's also just what I'm used to using. \$\endgroup\$ – Ethan Bierlein Aug 30 '15 at 2:28
  • 3
    \$\begingroup\$ @James_Parsons std::cin.get() is a method from the C++ standard library, while getc is from the C library. std::cin is more pedantic C++, but both provide the same functionality. Unless you have a good reason to use the C library, stick to the C++ one. \$\endgroup\$ – glampert Aug 30 '15 at 4:02
  • 1
    \$\begingroup\$ What empty caseS? \$\endgroup\$ – edc65 Aug 30 '15 at 19:57
  • \$\begingroup\$ @edc65 Go look at the switch in the letterValue function. \$\endgroup\$ – Ethan Bierlein Aug 30 '15 at 22:21
7
\$\begingroup\$

Here are some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It is particularly bad to put it into a header file, so please don't do that.

Don't use system("PAUSE")

There are two reasons not to use system("cls") or system("PAUSE"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a separate functions pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example:

void pause() {
    getchar();
}

General portability

This code could be made portable if, in addition to the changes in the previous point, you omit the Windows-only include files #include "stdafx.h".

Use const where practical

The passed std::string is not modified by gameScore() and so it should be declared as const std::string &. Right now, it's passed by value, so the code is making a useless extra copy.

Use "range-for" to simplify your code

Instead of using an index variable, your for loop could use "range-for":

for (const auto &ch : word)

Eliminate "magic numbers"

The code uses "magic numbers" such as 80 that have no obvious meaning. These would be better as named constants. In this particular case, that constant isn't needed at all. Just make word a std::string instead of a fixed size buffer.

Make it clear when loops end

The code currently uses while (true) but it doesn't really loop infinitely. It's better, generally, to have the loop exit condition explicitly stated. For example we could rewrite your main() function with a helper function and a for loop:

std::string ask(const std::string &prompt) {
    std::string line;
    std::cout << prompt;
    std::getline(std::cin, line);
    return line;
}

int main()
{
    const std::string prompt{"Enter a word: "};
    for (std::string word = ask(prompt); word[0] != ' '; word = ask(prompt)) {
        std::cout << "The score for '" << word << "' is " << gameScore(word) << "\n"; 
    }
}

Simplify by using appropriate data structure

Rather than doing all of that calculation for each letter, you could use a simple lookup mechanism instead.

int letterValue(char ch)
{
    const int points[]{
    //  a  b  c  d  e  f  g  h  i  j  k  l  m  
        1, 3, 3, 2, 1, 4, 2, 4, 1, 8, 5, 1, 3, 
    //  n  o  p  q   r  s  t  u  v  w  x  y  z
        1, 1, 3, 10, 1, 1, 1, 1, 4, 4, 8, 4, 10
    };
    return isalpha(ch) ? points[tolower(ch)-'a'] : 0;
}

Using that method, you'd have noticed that your existing code has no value for the letter N.

Use a standard algorithm

You could use your existing gameScore mechanism, but an even more powerful technique would be to use std::accumulate with a lambda. It can be used like this:

int gameScore(const std::string &word)
{
    return std::accumulate(word.cbegin(), word.cend(), 0, [](int total, char a){
        return total + letterValue(a); });
}

Since it appears you are just learning the C++ language, this may not make much sense to you yet, but I hope that at least it inspires you to keep learning this powerful and productive language.

\$\endgroup\$
  • \$\begingroup\$ When I transposed the existing code into the form I used, I noticed that there was no value for the letter N. The original code returns a value of 3 for the word onion but the correct answer is 5. \$\endgroup\$ – Edward Aug 30 '15 at 20:29
2
\$\begingroup\$

You could write your gameScore like:

int gameScore(string word)
{
    auto total = int {0};
    for(auto &ch : word)
        total += letterValue(ch);
    return total;
}

Hence use for range loop and use {} brackets for assignment operation instead of =. Use auto even for explicit types.

\$\endgroup\$
  • 2
    \$\begingroup\$ Can you explain why this is an improvement over the existing code. \$\endgroup\$ – Quill Aug 30 '15 at 0:43
  • \$\begingroup\$ I'm not seeing many improvements here. For example, how is auto total = int {0}; an improvement over int total = 0;? auto is a code smell as well. \$\endgroup\$ – Ethan Bierlein Aug 30 '15 at 1:05
  • \$\begingroup\$ Disclaimer: I am a beginner with C++ but I question what you say @EthanBierlein. Namely I will point you out to Herb Sutter, you could also check what Bjarne has to say about it too. I wont argue this point since I am a beginner but I would trust them more than you, sorry. As for curly brackets, I have read that in Bjarne's book A Tour of C++ but you can find it on this link under "Initialization of variables". Lastly range for loop is also preferred bla bla bla... BUT DON USE STD NAMESPACE! \$\endgroup\$ – firtek Aug 30 '15 at 7:07
  • \$\begingroup\$ @Quill I hope I have addressed some part of your question with my previous answer. In addition I have read that you should pass by reference since it is cheaper, and you don't need a copy of passed value. I think this addresses all the points I have made. I wanted to say that using that switch statement seems unreasonable to me but then again I was downvoted and I though I probably have some serious errors which I don't understand. \$\endgroup\$ – firtek Aug 30 '15 at 7:08
  • \$\begingroup\$ @firtek, you haven't done anything to solve my (and others) concerns about your answer. \$\endgroup\$ – Quill Aug 30 '15 at 7:55

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.