Clean Code attempt of HOLES problem on codechef.com

The problem asks you to take read an integer (number of words to read) and then process words entered counting the number of "HOLES" exist. Letters with HOLES are A B D O P Q and R. Of those B has 2 holes A D O P Q R have 1 and any other letter not listed counts as 0 even lowercase equivalents.

The scope is fairly limited so try to just assess the code based on the problem listed.

Full problem description: here

```
#include
#include
#include

class LetterEvaluator {
public:
LetterEvaluator()
: letterValues(createMap())
{}

int getValueOf(const char letter) const {
if(retrieveValueOfLetter(letter) == letterValues.end())
return 0;
else{
return retrieveValueOfLetter(letter)->second;
}
}

private:
const std::map letterValues;

std::map createMap() const {
std::map initializedMap;
initializedMap['A'] = 1;
initializedMap['B'] = 2;
initializedMap['D'] = 1;
initializedMap['O'] = 1;
initializedMap['P'] = 1;
initializedMap['Q'] = 1;
initializedMap['R'] = 1;
return initializedMap;
}

std::map::const_iterator retrieveValueOfLetter(const char letter) const {
return letterValues.find(letter);
}
};

class HolesInCapitalizedWordFinder {
public:
HolesInCapitalizedWordFinder()
: numberOfHoles(0)
, letterEvaluator()
{}

void processWord() {
evaluate(readWord());
printNumberOfHolesInWord();
}

private:
LetterEvaluator letterEvaluator;
int numberOfHoles;

std::string readWord() const {
std::string wordToEvaluate;
std::cin >> wordToEvaluate;
return wordToEvaluate;
}

void evaluate(const std::string& wordToEvaluate ) {
numberOfHoles = 0;
for(auto& character : wordToEvaluate) {
if(isNotAnUpperCaseLetter(character)){
nu

LetterEvaluator

-
Since this is a C++11 solution, you no longer need something like createMap():

std::map createMap() const {
        std::map initializedMap;
        initializedMap['A'] = 1;
        initializedMap['B'] = 2;
        initializedMap['D'] = 1;
        initializedMap['O'] = 1;
        initializedMap['P'] = 1;
        initializedMap['Q'] = 1;
        initializedMap['R'] = 1;
        return initializedMap;
    }

Use C++11's list initialization to have it initialized in place:

const std::map letterMap {
    {'A', 1},
    {'B', 2},
    {'D', 1},
    {'O', 1},
    {'P', 1},
    {'Q', 1},
    {'R', 1}
};

-
If you have C++14, you can avoid providing an explicit return type:

std::map::const_iterator retrieveValueOfLetter(const char letter) const {
        return letterValues.find(letter);
    }

You can use automatic return type deduction via auto:

auto retrieveValueOfLetter(const char letter) const {
    return letterValues.find(letter);
}

This does also shorten the line, which could help add more readability.

-
You have two redundant calls to retrieveValueOfLetter():

int getValueOf(const char letter) const {       
        if(retrieveValueOfLetter(letter) == letterValues.end())
            return 0;
        else{
            return retrieveValueOfLetter(letter)->second;
        }
    }

Just retrieve the value from one call and then return the appropriate value:

int getValueOf(const char letter) const {
    const auto retrievedLetter = retrieveValueOfLetter(letter);
    return (retrievedLetter == letterValues.end()) ? 0 : retrievedLetter->second;
}

I've used a ternary here to shorten the expression to just one line. If it looks less-readable, or if you may need to expand on it later, you may still use if/else instead.

HolesInCapitalizedWordFinder

-
It looks like readWord() should be a free function (non-member function). However, if you're trying to read in a word, then you can just overload operator>> for this class.

-
Do you really need isNotAnUpperCaseLetter()? There's already std::isupper() provided in the standard library, which should be safer to use. You may encounter some issues with your own, depending on how it's implemented.

StackExchange Code Review Q#60941, answer score: 5