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.

• JaDogg: 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. :)

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.

• Daniel Duque: 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.
• Quuxplusone: 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.
• indi: 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.