Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 <iostream>
#include <map>
#include <string>

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<char,int> letterValues;

    std::map<char,int> createMap() const {
        std::map<char,int> 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<char,int>::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)){
                numberOfHoles = -1;
                break;
            }               
            numberOfHoles += letterEvaluator.getValueOf(character);
        }       
    }

    bool isNotAnUpperCaseLetter(const char character) const{
        return character < 65 || character > 90;
    }

    void printNumberOfHolesInWord() const {
        if(numberOfHoles == -1) {
            std::cout << "Not all characters in that word are uppercase letters!\n";
        }
        else {
            std::cout << numberOfHoles << '\n';
        }
    }
};


int main() {
    std::ios::sync_with_stdio(false);

    int timesToRun = 0;
    std::cin >> timesToRun;

    HolesInCapitalizedWordFinder holesInCaptilizedWordFinder;

    for(int numberOfTimesRan = 1; numberOfTimesRan <= timesToRun; ++numberOfTimesRan) {
        holesInCaptilizedWordFinder.processWord();
    }
    return 0;
}
share|improve this question
    
You don't need a new version of letterValues for each instance of LetterEvaluator. You can have a single copy used by all instances with judicious use of static. –  Loki Astari Aug 24 at 20:17

3 Answers 3

up vote 4 down vote accepted

LetterEvaluator

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

    std::map<char,int> createMap() const {
            std::map<char,int> 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<char, int> 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<char,int>::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.

share|improve this answer
    
Is the constexpr required? I had tried that without and couldn't get it to work. I'm using visual studio 2012 express and it errors out stating data member initializer not allowed. –  M K Aug 24 at 18:40
    
@MK: I've just realized that constexpr doesn't work here. Sorry about that. I'll correct this. –  Jamal Aug 24 at 18:41
    
@MK: Also, constexpr isn't supported by VS2012. Either way, it's not essential here (even if it worked), as long as you still use const. –  Jamal Aug 24 at 18:49
    
That part about implicit return types would've been nice to know before I spent about 30-45 minutes trying to return a const iterator before I realized it was called const_iterator. Oh well I know now. Thanks. –  M K Aug 24 at 19:26
    
@MK: I've added some new things, though I haven't looked at every bit of the solution. I haven't tested my implementation of retrievedLetter, so let me know if it doesn't work. –  Jamal Aug 24 at 19:50

Rather than a technical review, I'd like to offer a design one (and I'm by no means a design expert, so take this as you will :p).

First off, I'd like to say that it's not bad overall. You've correctly determined the aspects of the problem and broken the functionality and ideas into bite-sized, independent chunks. I just think the pieces could fit together a little more cleanly, and that there's a worrying nagging at the back of my mind that this has been overcomplicated. I know you're going for 'clean code', but I can't help but think this is over engineered. This program has very little to actually do, and while I would advocate heavy object orientation in certain situations, this is not one of them. Put bluntly, there's no particularly compelling reason to OO the hell out of this, and I think it's more confusion than it's worth.

Anyway, on to details.


The entirety of this program can be expressed as a loop and a std::accumulate. Modeling a word as anything more than a string is overkill, and encapsulating the hole counting in anything other than a function is unnecessary cruft. Yes, if this were to grow in complexity in the future, you would likely want to involve OO, but scrapping 15 lines is rather doable.

Think about it this way: if you came across this code 6 months from now without the problem statement, would you have any idea what it does? Probably not.


Example:

#include <algorithm>
#include <iostream>
#include <string>
#include <unordered_map>

int find_or_default(const std::unordered_map<char, int>& m, char key, int default_value) {
    auto iter = m.find(key);
    return iter == m.end() ? default_value : iter->second;
}

int count_holes(const std::string& word) {
    static const std::unordered_map<char, int> hole_counts = {
        {'A', 1}, {'D', 1}, {'O', 1}, {'P', 1}, {'R', 1}, {'B', 2}
    };
    return std::accumulate(word.cbegin(), word.cend(), 0, [&](int sum, char w) {
        return sum + find_or_default(hole_counts, w, 0);
    });
}

int main() {
    int num_test_cases;
    if (!(std::cin >> num_test_cases)) { return EXIT_FAILURE; }
    std::string word;
    for (int test_case = 0; test_case < num_test_cases && std::cin >> word; ++test_case) {
        std::cout << count_holes(word) << '\n';
    }
}

As for your existing solution, there are a lot of problems that I see (design oriented -- others have mentioned technical items). In particular, there's a lot of coupling and very little separation of concerns. For example, a class shouldn't be doing logic and reading input. That's two different concerns. Or, with regards to coupling, an example is that HolesInCapitalizedWordFinder creates LetterEvaulator. This means that HolesInCapitalizedWordFinder can only be used with that LetterEvaluator. What if you wanted to evaluate letters a different way?

Or, considering a different route, what if you wanted to do something other than read from std::cin? Or output to something other than std::cout? What if you want to just calculate the holes and not output them at all?

Rule of thumb: if you can't explain what a class does without using a conjunction (and, or, etc), it's typically doing way too much. (What does your class do? It reads in words, calculates the holes in them, and outputs them -- 3 things.)

I've been pretty brief, but if you're interested in more along these lines, look into SOLID.

share|improve this answer
    
I do appreciate the time you took to answer the question and understand what you mean. That's partially why I said try to assess based on the problem description. The problem itself is rather easy. LetterEvaluator Evaluates letters - literally HolesInCapitalizedWordFinder - Finds Holes in Capitalized Words –  M K Aug 24 at 21:31
    
@MK The point is that you should not try to fit every single thing as an object. Sometimes a function is more appropriate than an object. (If you want to get into a flame war, some people in functional programming claim that the fundamental entities of programming are actually functions and not objects.) –  toto2 Aug 27 at 0:49

A couple of things to add to what @Jamal said:

  • In HolesInCapitalizedWordFinder, you can get rid of the default constructor by using in-class member initializers for the class members:

    private:
        LetterEvaluator letterEvaluator;
        int             numberOfHoles = 0;
    

    Here, the default constructor will be automatically generated by the compiler since you don't define any other constructor. The automatically generated default constructor will initialize the values with their default initializers if they have any, or with their default constructor if the member has no in-class initializer.

  • Passing arguments to a function by const copy is useless for built-in types. You may as well drop the const qualification when you pass an argument by value:

    int getValueOf(char letter) const { /* ... */ }
    
  • Overall, I have got the feeling that your solution is overengineered for such a simple problem. You have got classes with long names with methods with long names... I would find a simpler program simpler to read for such a task:

    const std::map<char, int> holes {
        {'A', 1},
        {'B', 2},
        {'D', 1},
        {'O', 1},
        {'P', 1},
        {'Q', 1},
        {'R', 1}
    };
    
    int main() {
        int times;
        std::cin >> times;
        assert(times <= 40);
    
        for (int i = 0 ; i < times ; ++i) {
    
            std::string word;
            std::cin >> word;
            assert(word.size() < 100);
    
            int nb_holes = 0;
            for (char c: word) {
                assert(std::isupper(c)); // only capital letters
                if (holes.find(c) != holes.end()) {
                    nb_holes += holes[c];
                }
            }
    
            std::cout << "There are " << nb_holes << " holes in " << word << ".\n";
        }
    }
    

    I wrote a really simplified version that doesn't tell the user what zhe has done wrong, but it at least reports through assert that there is an error. Frnakly, the problem and the solution are straightforward enough; you don't have to bother with overengineered designs.

share|improve this answer
    
Partially overengineering this simple problem is part of the exercise. I'm trying to write "cleaner" code without going too overboard. –  M K Aug 24 at 21:34
    
@MK But you also said "try to just assess the code based on the problem listed". And that's exactly what I did: the problem is simple, use a simple solution, that will probably be the cleanest thing you can do. Follow the KISS principle. –  Morwenn Aug 24 at 21:53
    
What kind of problem would you suggest I work on as a beginner to prep me for working on large projects that I could post on this site and would be analyzed quickly to let me know I'm at least going in the right direction and aren't so simple that I can avoid over-engineered comments? –  M K Aug 24 at 21:55
1  
@MK I don't know. But generally speaking, programming challenges help to develop your skills in dealing with algorithms, they do not improve your code design skills. I don't know the kind of problems that helps improving your code design skills. –  Morwenn Aug 24 at 22:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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