#Design issues
Design issues
- You are unnecessarily restricting the
Parser
interface by forcingfs
to be astd::ifstream&
whenstd::istream&
would work as well. AParser::DoParsing(AData&, std::ifstream&)
should be markedoverride
. Same forBParser::DoParsing(BData&, std::ifstream&)
.- I can't see any specific purpose for
IParsing
: Why not callAParser::DoParsing
orBParser::DoParsing
directly? - Besides, parameter
d
ofIParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs)
should be of typeT&
. FParsing::HandleParsing(std::string, Data&, std::ifstream&)
doesn't use any class instance members. Maybe make itstatic
?- Also, parameter
type
from that function should probably be anenum
- no need to use strings for that. - Why allocate two
std::shared_ptr
in case a valid parser is found if they won't be shared and will be deallocated once the function returns? - What advantage do you hope to get from this extravagant implementation over overloading
operator>>(std::istream&, AData&)
andoperator>>(std::istream&, BData&)
instead?
#Design issues
- You are unnecessarily restricting the
Parser
interface by forcingfs
to be astd::ifstream&
whenstd::istream&
would work as well. AParser::DoParsing(AData&, std::ifstream&)
should be markedoverride
. Same forBParser::DoParsing(BData&, std::ifstream&)
.- I can't see any specific purpose for
IParsing
: Why not callAParser::DoParsing
orBParser::DoParsing
directly? - Besides, parameter
d
ofIParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs)
should be of typeT&
. FParsing::HandleParsing(std::string, Data&, std::ifstream&)
doesn't use any class instance members. Maybe make itstatic
?- Also, parameter
type
from that function should probably be anenum
- no need to use strings for that. - Why allocate two
std::shared_ptr
in case a valid parser is found if they won't be shared and will be deallocated once the function returns? - What advantage do you hope to get from this extravagant implementation over overloading
operator>>(std::istream&, AData&)
andoperator>>(std::istream&, BData&)
instead?
Design issues
- You are unnecessarily restricting the
Parser
interface by forcingfs
to be astd::ifstream&
whenstd::istream&
would work as well. AParser::DoParsing(AData&, std::ifstream&)
should be markedoverride
. Same forBParser::DoParsing(BData&, std::ifstream&)
.- I can't see any specific purpose for
IParsing
: Why not callAParser::DoParsing
orBParser::DoParsing
directly? - Besides, parameter
d
ofIParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs)
should be of typeT&
. FParsing::HandleParsing(std::string, Data&, std::ifstream&)
doesn't use any class instance members. Maybe make itstatic
?- Also, parameter
type
from that function should probably be anenum
- no need to use strings for that. - Why allocate two
std::shared_ptr
in case a valid parser is found if they won't be shared and will be deallocated once the function returns? - What advantage do you hope to get from this extravagant implementation over overloading
operator>>(std::istream&, AData&)
andoperator>>(std::istream&, BData&)
instead?
#Design issues
- You are unnecessarily restricting the
Parser
interface by forcingfs
to be astd::ifstream&
whenstd::istream&
would work as well. AParser::DoParsing(AData&, std::ifstream&)
should be markedoverride
. Same forBParser::DoParsing(BData&, std::ifstream&)
.- I can't see any specific purpose for
IParsing
: Why not callAParser::DoParsing
orBParser::DoParsing
directly? - Besides, parameter
d
ofIParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs)
should be of typeT&
. FParsing::HandleParsing(std::string, Data&, std::ifstream&)
doesn't use any class instance members. Maybe make itstatic
?- Also, parameter
type
from that function should probably be anenum
- no need to use strings for that. - Why allocate two
std::shared_ptr
in case a valid parser is found if they won't be shared and will be deallocated once the function returns? - What advantage do you hope to get from this extravagant implementation over overloading
operator>>(std::istream&, AData&)
andoperator>>(std::istream&, BData&)
instead?
lang-cpp