Simple class exercise using the "this" pointer

Question

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;
}

Answer 1

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

Answer 2

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;
}

Answer 3

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