Overloading istream>> to read comma-separated input

Question

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.

Answer 1

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.