Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Design issues

Design issues

  • You are unnecessarily restricting the Parser interface by forcing fs to be a std::ifstream& when std::istream& would work as well.
  • AParser::DoParsing(AData&, std::ifstream&) should be marked override. Same for BParser::DoParsing(BData&, std::ifstream&).
  • I can't see any specific purpose for IParsing: Why not call AParser::DoParsing or BParser::DoParsing directly?
  • Besides, parameter d of IParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) should be of type T&.
  • FParsing::HandleParsing(std::string, Data&, std::ifstream&) doesn't use any class instance members. Maybe make it static?
  • Also, parameter type from that function should probably be an enum - 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&) and operator>>(std::istream&, BData&) instead?

#Design issues

  • You are unnecessarily restricting the Parser interface by forcing fs to be a std::ifstream& when std::istream& would work as well.
  • AParser::DoParsing(AData&, std::ifstream&) should be marked override. Same for BParser::DoParsing(BData&, std::ifstream&).
  • I can't see any specific purpose for IParsing: Why not call AParser::DoParsing or BParser::DoParsing directly?
  • Besides, parameter d of IParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) should be of type T&.
  • FParsing::HandleParsing(std::string, Data&, std::ifstream&) doesn't use any class instance members. Maybe make it static?
  • Also, parameter type from that function should probably be an enum - 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&) and operator>>(std::istream&, BData&) instead?

Design issues

  • You are unnecessarily restricting the Parser interface by forcing fs to be a std::ifstream& when std::istream& would work as well.
  • AParser::DoParsing(AData&, std::ifstream&) should be marked override. Same for BParser::DoParsing(BData&, std::ifstream&).
  • I can't see any specific purpose for IParsing: Why not call AParser::DoParsing or BParser::DoParsing directly?
  • Besides, parameter d of IParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) should be of type T&.
  • FParsing::HandleParsing(std::string, Data&, std::ifstream&) doesn't use any class instance members. Maybe make it static?
  • Also, parameter type from that function should probably be an enum - 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&) and operator>>(std::istream&, BData&) instead?
Source Link
hoffmale
  • 6.5k
  • 18
  • 41

#Design issues

  • You are unnecessarily restricting the Parser interface by forcing fs to be a std::ifstream& when std::istream& would work as well.
  • AParser::DoParsing(AData&, std::ifstream&) should be marked override. Same for BParser::DoParsing(BData&, std::ifstream&).
  • I can't see any specific purpose for IParsing: Why not call AParser::DoParsing or BParser::DoParsing directly?
  • Besides, parameter d of IParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) should be of type T&.
  • FParsing::HandleParsing(std::string, Data&, std::ifstream&) doesn't use any class instance members. Maybe make it static?
  • Also, parameter type from that function should probably be an enum - 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&) and operator>>(std::istream&, BData&) instead?
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /