3
\$\begingroup\$

This is my solution for an exercise from a book. It's a simple 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 <string>

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 <cstring>
#include <iostream>

Golf::Golf(const std::string name, int hc)
{
    fullname = name;
    handicap = hc;
}

int Golf::setgolf()
{
    std::string name;
    int hc;
    std::cout << "Enter the name: ";
    getline(cin, name);
    if (name == "")
        return 0;
    std::cout << "Enter the handicap: ";
    std::cin >> hc;
    *this = Golf(name, hc);
    return 1;
}
\$\endgroup\$

3 Answers 3

Reset to default
2
\$\begingroup\$

  • You're not using <cstring> specifically in your implementation, so remove it.

  • If you're not defining sethandicap() anywhere, remove the declaration.

  • The constructor is only initializing data members, so it can be an initializer list:

    Golf::Golf(std::string name, int hc) : fullname(name), handicap(hc) {}

  • Although I agree with @Anton Golov about setgolf(), I'll address some things in it anyway:

    std::cout << "Enter the name: ";
std::string name;        // put declaration here to maintain a closer scope
getline(std::cin, name); // don't forget the "std::"

if (name.empty()) return 0; // empty() is more readable than ""

std::cout << "Enter the handicap: ";
std::cin.ignore(); // getline() is used prior, so add this
int hc;            // put declaration here to maintain a closer scope
std::cin >> hc;
\$\endgroup\$
2
  • \$\begingroup\$ Setters need not break encapsulation; there's no reason the class interface shouldn't have some property that the user may change the value of. For instance, if we have a lexer class, setting the current line number may be a valid operation. \$\endgroup\$ Commented Sep 12, 2013 at 15:59
  • \$\begingroup\$ @AntonGolov: Okay. I suppose I'm still unsure about what exactly breaks encapsulation and what doesn't. Many different contradictions, it seems. \$\endgroup\$ Commented Sep 12, 2013 at 16:03
4
\$\begingroup\$

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

Moving on to the Golf::setgolf function specifically, I don't think such a function is meaningful. 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;
}
\$\endgroup\$
2
\$\begingroup\$
  • Lose the private, class members are private by default.

  • setGolf(): Instead of returning 1 or 0, you can apply exceptions management here. Also, you're doing double jobs by *this = Golf(name, hc);. You call a constructor that creates an instance of Golf and then assign that instance to *this when you don't even have an assignment operator defined. Don't worry, a default version is already generated.

    You can fix that by doing this:

    this->fullname = name;
this->handicap = hc;
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think explicitly making members private just makes it a bit clearer and doesn't really have any drawbacks. \$\endgroup\$ Commented Sep 11, 2013 at 19:11
  • \$\begingroup\$ @Tharwen: Agreed. I think it's also helpful in case you have both public and private functions. \$\endgroup\$ Commented Sep 11, 2013 at 20:45

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.