Overloading istream>> to read comma-separated inputClass to read comma separated data from diskCalculator - C++ operator-overloadingSelecting results as a comma-separated stringCreating an istream peekerCustomized streambuffer for C++ istreamC++ operator overloading for matrix operationsC++ operator overloading for matrix operations - follow-upEncapsulation preserving operator= overloading in C++Operator Overloading Tricks in C++C++ Read istream into string with exceptions

Chess with symmetric move-square

Download, install and reboot computer at night if needed

Can a German sentence have two subjects?

Does the radius of the Spirit Guardians spell depend on the size of the caster?

Is it possible to make sharp wind that can cut stuff from afar?

What is the logic behind how bash tests for true/false?

A Journey Through Space and Time

How to make payment on the internet without leaving a money trail?

Why do we use polarized capacitor?

What do you call something that goes against the spirit of the law, but is legal when interpreting the law to the letter?

Shell script can be run only with sh command

Concept of linear mappings are confusing me

Why is an old chain unsafe?

Prevent a directory in /tmp from being deleted

New order #4: World

Simulate Bitwise Cyclic Tag

Why did the Germans forbid the possession of pet pigeons in Rostov-on-Don in 1941?

Email Account under attack (really) - anything I can do?

What typically incentivizes a professor to change jobs to a lower ranking university?

What is the meaning of "of trouble" in the following sentence?

What are these boxed doors outside store fronts in New York?

Where to refill my bottle in India?

How to use Pandas to get the count of every combination inclusive

Copycat chess is back



Overloading istream>> to read comma-separated input


Class to read comma separated data from diskCalculator - C++ operator-overloadingSelecting results as a comma-separated stringCreating an istream peekerCustomized streambuffer for C++ istreamC++ operator overloading for matrix operationsC++ operator overloading for matrix operations - follow-upEncapsulation preserving operator= overloading in C++Operator Overloading Tricks in C++C++ Read istream into string with exceptions






.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;








6












$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.










share|improve this question











$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$
    – 422_unprocessable_entity
    Mar 27 at 15:43

















6












$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.










share|improve this question











$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$
    – 422_unprocessable_entity
    Mar 27 at 15:43













6












6








6


1



$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.










share|improve this question











$endgroup$




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.







c++ beginner parsing stream overloading






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Mar 27 at 16:39







Daniel Duque

















asked Mar 27 at 14:20









Daniel DuqueDaniel Duque

555




555







  • 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$
    – 422_unprocessable_entity
    Mar 27 at 15:43












  • 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$
    – 422_unprocessable_entity
    Mar 27 at 15:43







1




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$
– 422_unprocessable_entity
Mar 27 at 15:43




$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$
– 422_unprocessable_entity
Mar 27 at 15:43










1 Answer
1






active

oldest

votes


















7












$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, conservatoryn"
"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)
!vector_contains(clue::weapons, a.weapon)
;


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.






share|improve this answer









$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





StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);













draft saved

draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216343%2foverloading-istream-to-read-comma-separated-input%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes









7












$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, conservatoryn"
"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)
!vector_contains(clue::weapons, a.weapon)
;


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.






share|improve this answer









$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















7












$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, conservatoryn"
"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)
!vector_contains(clue::weapons, a.weapon)
;


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.






share|improve this answer









$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













7












7








7





$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, conservatoryn"
"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)
!vector_contains(clue::weapons, a.weapon)
;


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.






share|improve this answer









$endgroup$



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, conservatoryn"
"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)
!vector_contains(clue::weapons, a.weapon)
;


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.







share|improve this answer












share|improve this answer



share|improve this answer










answered Mar 27 at 15:39









QuuxplusoneQuuxplusone

13.4k12266




13.4k12266











  • $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
















  • $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















$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




$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




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$
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




$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

















draft saved

draft discarded
















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid


  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.

Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f216343%2foverloading-istream-to-read-comma-separated-input%23new-answer', 'question_page');

);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







-beginner, c++, overloading, parsing, stream

Popular posts from this blog

Frič See also Navigation menuinternal link

Identify plant with long narrow paired leaves and reddish stems Planned maintenance scheduled April 17/18, 2019 at 00:00UTC (8:00pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?What is this plant with long sharp leaves? Is it a weed?What is this 3ft high, stalky plant, with mid sized narrow leaves?What is this young shrub with opposite ovate, crenate leaves and reddish stems?What is this plant with large broad serrated leaves?Identify this upright branching weed with long leaves and reddish stemsPlease help me identify this bulbous plant with long, broad leaves and white flowersWhat is this small annual with narrow gray/green leaves and rust colored daisy-type flowers?What is this chilli plant?Does anyone know what type of chilli plant this is?Help identify this plant

fontconfig warning: “/etc/fonts/fonts.conf”, line 100: unknown “element blank” The 2019 Stack Overflow Developer Survey Results Are In“tar: unrecognized option --warning” during 'apt-get install'How to fix Fontconfig errorHow do I figure out which font file is chosen for a system generic font alias?Why are some apt-get-installed fonts being ignored by fc-list, xfontsel, etc?Reload settings in /etc/fonts/conf.dTaking 30 seconds longer to boot after upgrade from jessie to stretchHow to match multiple font names with a single <match> element?Adding a custom font to fontconfigRemoving fonts from fontconfig <match> resultsBroken fonts after upgrading Firefox ESR to latest Firefox