patterncppMinor
Simple class exercise using the "this" pointer
Viewed 0 times
thissimpletheexercisepointerusingclass
Problem
This is my solution for an exercise from a book. It's a simple
Is this a proper solution? I'm particularly interested in the
golf.h:
golf.cpp:
Golf class with member variables fullname and handicap. The constructor sets the variables to the provided name and handicap using values passed as arguments. The setgolf() function solicits name and handicap from user, creates a temporary Golf object and assigns it to the invoking object. Is this a proper solution? I'm particularly interested in the
setgolf() method.golf.h:
#include
class Golf
{
private:
std::string fullname;
int handicap;
public:
Golf();
Golf(const std::string name, int hc);
int setgolf();
void sethandicap(int hc);
void showgolf();
};golf.cpp:
#include "golf.h"
#include
#include
Golf::Golf(const std::string name, int hc)
{
fullname = name;
handicap = hc;
}
int Golf::setgolf()
{
std::string name;
int hc;
std::cout > hc;
*this = Golf(name, hc);
return 1;
}Solution
It's hard to say whether the solution is proper when there isn't a problem to solve. How a class should be designed depends on how it will be used; a class in isolation is hard to judge. Even if you want to focus on some specific aspect, try to fit it into the context of a larger problem.
Back to the code: I suspect
Why not just
Speaking of checking whether things succeeded, why not throw an exception if the input is invalid? There are valid reasons, but it's certainly a solution you should consider. (By the way, if my previous solution sounded bad due to the need for exceptions, have no fear:
And, in a similar vein: it sounds like you're trying to prevent the creation of invalid
All in all, I'd suggest dropping
Back to the code: I suspect
showgolf should be a const function. I don't see why printing a Golf would change it. You probably also meant to pass your constructor a const std::string&. Finally, are you sure a showgolf member function is really what you want -- why not an operator> overload or a free queryGolf function sound significantly more sensible to me. More specifically, the usage:Golf golf;
golf.setgolf();Why not just
auto golf = queryGolf();? It's simpler, it means you can't forget to check whether things succeeded, and given setGolf's implementation it is likely to be just as efficient.Speaking of checking whether things succeeded, why not throw an exception if the input is invalid? There are valid reasons, but it's certainly a solution you should consider. (By the way, if my previous solution sounded bad due to the need for exceptions, have no fear:
boost::optional fits the use-case very well.)And, in a similar vein: it sounds like you're trying to prevent the creation of invalid
Golfs. However, your default constructor does exactly that. Are you sure default-construction is more valuable than guaranteed validity? Again, depending on what you do with the class, the answer varies.All in all, I'd suggest dropping
showgolf and writing:Golf promptGolf() {
Golf golf;
// read into fullname and handicap. promptGolf may be a friend.
if (/* golf is not valid, std::cin died, etc. */)
throw TerribleError{};
return golf;
}Code Snippets
Golf golf;
golf.setgolf();Golf promptGolf() {
Golf golf;
// read into fullname and handicap. promptGolf may be a friend.
if (/* golf is not valid, std::cin died, etc. */)
throw TerribleError{};
return golf;
}Context
StackExchange Code Review Q#31100, answer score: 4
Revisions (0)
No revisions yet.