7
\$\begingroup\$

I have the following very simple class:

class accusation
{
private:
    std::string murderer;
    std::string weapon;
    std::string place;
public:
    accusation() = default;
    accusation(std::string, std::string, std::string);
    friend std::ostream& operator<<(std::ostream&, const accusation&);
    friend std::istream& operator>>(std::istream&, accusation&);
};

I have overloaded my extraction from istream operator as follows:

std::istream& operator>>(std::istream& is, accusation& readable)
{
    std::vector<std::string> accusation;
    std::string token, word;
    //divide by commas
    while (std::getline(is, token, ','))
    {
        std::string pushable;
        std::stringstream ss(token);
        while (ss >> word) pushable += word + " ";
        if (pushable.size() != 0) pushable.pop_back(); //remove that last white space
        std::transform(pushable.begin(), pushable.end(), pushable.begin(), ::tolower);
        accusation.push_back(pushable);
    }
    if (accusation.size() == 3)
    {
        is.clear();
        bool valid{ false };
        //check it matches one of the clue::characters
        for (const auto& character : clue::characters)
            if (accusation[0] == character)
            {
                valid = true;
                break;
            }
        if (valid)
        {
            valid = false;
            //check it matches one of the clue::weapons
            for (const auto& weapon : clue::weapons)
                if (accusation[1] == weapon)
                {
                    valid = true;
                    break;
                }
            if (valid)
            {
                valid = false;
                //check it matches one of the clue::places
                for (const auto& place : clue::places)
                    if (accusation[2] == place)
                    {
                        valid = true;
                        break;
                    }
                if (valid)
                {
                    readable.murderer = accusation[0];
                    readable.weapon = accusation[1];
                    readable.place = accusation[2];
                }
                else
                    is.setstate(std::ios_base::failbit);
            }
            else
                is.setstate(std::ios_base::failbit);
        }
        else
            is.setstate(std::ios_base::failbit);
    }
    else
        is.setstate(std::ios_base::failbit);
    return is;
}

I am reading input as green, dagger, kitchen and storing it in my accusation. The first element has to be in clue::characters (an array of possible game characters), second element in clue::weapons, and third element in clue::places.

Can somebody suggest a cleaner way to overload this operator? The code works as expected, but I believe that there is a lot of space for improvements. Any push into the right direction is highly appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CR, Could you please change the title to show the requirement from business/exercise point of view rather than your concerns. Concerns should go into the body of the question. :) \$\endgroup\$ – bhathiya-perera Mar 27 at 15:43
8
\$\begingroup\$

95 percent of programming is looking for redundancies and eliminating them.

For example, why do you bother with reading strings into accusations[] first, and then later copying them into readable.murderer et cetera? Why not just read them directly into readable.murderer? This would have the bonus of eliminating those "magic number" indices 0, 1, and 2, and replacing them with readable (no pun intended) identifiers.

std::getline(is, readable.murderer, ',');
std::getline(is, readable.weapon, ',');
std::getline(is, readable.place, ',');  // shouldn't this last one be '\n' not ','?

You should test your code and see if it does what you wanted.

std::istringstream iss(
    "Mr Green, lead pipe, conservatory\n"
    "Mrs Peacock, noose, kitchen"
);
accusation acc;
iss >> acc;

This reads 5 items into accusation. Is this what you wanted to happen?


Reduce repetition. You have the following snippet repeated three times:

    for (const auto& THING : THINGS)
        if (accusation[INDEX] == THING)
        {
            valid = true;
            break;
        }

So, first of all, we wrap the loop body in curly braces to protect against goto fail; and then we factor it out into a function.

template<class T>
bool vector_contains(const std::vector<T>& vec, const T& value) {
    for (auto&& elt : vec) {
        if (elt == value) {
            return true;
        }
    }
    return false;
}

And then our main function's code can become simply

bool valid = vector_contains(clue::characters, readable.murderer)
          && vector_contains(clue::weapons, readable.weapon)
          && vector_contains(clue::places, readable.place);
if (!valid) {
    is.setstate(std::ios_base::failbit);
}

The body of vector_contains could also be implemented simply by using an STL algorithm, e.g.

template<class T>
bool vector_contains(const std::vector<T>& vec, const T& value) {
    return std::count(vec.begin(), vec.end(), value);
}

or

template<class T>
bool vector_contains(const std::vector<T>& vec, const T& value) {
    return std::find(vec.begin(), vec.end(), value) != vec.end();
}

or

template<class T>
bool vector_contains(const std::vector<T>& vec, const T& value) {
    return std::any_of(vec.begin(), vec.end(), [&](const auto& elt) {
        return elt == value;
    });
}

I named the function vector_contains, rather than simply contains, because in my estimation there is a very real possibility that C++2a might add std::contains to the library and thus break any code using ADL calls to contains.


Minor nits:

  • I strongly recommend making all your constructors explicit, to eliminate bugs from unintentional implicit conversions. (Yes, even your multi-argument constructors.)

  • I strongly recommend making operator>> and operator<< into inline friend functions — define them right inside the body of your class. This will make them findable only via ADL, and is generally what you want. It'll look a lot more reasonable, too, once you've refactored your operator>> to be only five or six lines long! :)


You're also doing something weird with stringstream to remove whitespace from the ends of each piece of the string. You should factor that out into a helper function, and then simplify it. Say,

std::string strip(const std::string& s)
{
    int i = 0;
    while (isspace(s[i])) ++i;
    int j = s.size();
    while (j >= 1 && isspace(s[j-1])) --j;
    return s.substr(i, j-i);
}

https://wandbox.org/permlink/uVSolN0Nepk48Mgm

class accusation
{
private:
    std::string murderer;
    std::string weapon;
    std::string place;
public:
    accusation() = default;
    explicit accusation(std::string, std::string, std::string);
    friend std::ostream& operator<<(std::ostream&, const accusation&);
    friend std::istream& operator>>(std::istream& is, accusation& a) {
        std::getline(is, a.murderer, ',');
        std::getline(is, a.weapon, ',');
        std::getline(is, a.place);
        if (!vector_contains(clue::characters, a.murderer) ||
            !vector_contains(clue::weapons, a.weapon) ||
            !vector_contains(clue::places, a.place)) {
            is.setstate(std::ios_base::failbit);
        }
        return is;
    }
};

Deciding whether your std::transform lowercasing should be removed, kept, or folded into the helper function vector_contains (renaming that function to indicate its new purpose, and using a non-mutating facility such as strcasecmp) is left as an exercise for the reader.

\$\endgroup\$
  • \$\begingroup\$ Thanks for all your feedback, it has been very eye-opening reading all your suggestions. Regarding your 'strip' function, it only removes white spaces from the beginning and the end of a string; opposed to what I was doing which deleted extra white space between words as well. Nevertheless I get your point, and will improve my code from all your suggestions. \$\endgroup\$ – Daniel Duque Mar 27 at 17:01
  • 1
    \$\begingroup\$ Given that you made mr green acceptable as a synonym for Mr Green, maybe you should consider whether mrgreen should be acceptable as well. Then you wouldn't even need to remove spaces; you could just write a non-mutating string comparison, similar to strcasecmp, that ignores all whitespace too. Personally, I would go the other direction and force the user to enter Mr Green using that one exact spelling, to increase simplicity and ease-of-teaching-the-interface. If you do want to do clever fuzzy matching, look at en.wikipedia.org/wiki/Approximate_string_matching for ideas. \$\endgroup\$ – Quuxplusone Mar 27 at 17:25
  • \$\begingroup\$ Writing directly into the object means you lose the strong exception guarantee. You also risk creating invalid accusation objects with broken invariants. Writing into a temp object then moving into a, or, even better, writing into three strings then doing validation on them (using a function you should already have for validating the constructor args), is a better plan. \$\endgroup\$ – indi Mar 29 at 23:23

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.