Is it considered a good practice to define virtual get and set functions in C++?

Question

If I have a simple 2 level class hierarchy as, for instance, this one:

// level 1
class Spare_Part{
private:
    string name;
    double price;
public:
    Spare_Part();
    string getName() { return name; }
    double getPrice() { return price; }
    virtual int getQuantity() { return -1; }; // may also define it as pure virtual
};
//level 2
class On_hand : public Spare_Part{
private:
    int quantity;
    string location;
public:
    On_hand();
    int getQuantity(){ return quantity; }
};

I want to have access to the member 'quantity' in the class 'On_hand' using a pointer to the base class 'Spare_part', so I made the 'getQuantity' function a virtual one. The code works fine, however, I have a feeling that I shouldn't have a get/set function (even though a virtual one) to access a member that is defined somewhere down the hierarchy. Is this really considered a bad practice that should be avoided by, for example, redesign of the classes? Edit:The whole hierarchy is a little bit more complex. Alongside the 'On_hand' class there is class for parts available through contracted suppliers. The assumption is that I wouldn't be able to know how many parts are available through the suppliers and that is why 'quantity' is not included in the base class.

The above code is bad practice. It contains flag values (`-1`), and uses polymorphism needlessly (why is a spare part that is not on hand anything more than a spare part with no location and quantity? Having an optional (sub set of) properties is insufficient justification for polymorphism, especially because (in the above case) it is clear the property isn't really optional (as it is exposed in the root type!) Even your accessors: how is spare_part not just flat data? — Yakk - Adam Nevraumont, Feb 18 '15 at 15:59
To add to what Yakk is saying: It sounds like you are polluting your base class with information about a derived class. Perhaps you are trying to get a list of Spare_Part*, but be able to tell something about a On_hand, and you want to do that without using a union or doing dynamic casting. Perhaps you should instead consider another class in the middle of the hierarchy that implements what you need. — AndyG, Feb 18 '15 at 16:05
@Yakk The whole hierarchy is a little bit more complex. Alongside the 'On_hand' class there is class for parts available through contracted suppliers. The assumption is that I wouldn't be able to know how many parts are available through the suppliers and that is why 'quantity' is not included in the base class. — Adam Payne, Feb 18 '15 at 16:06

score 4 · Answer 1 · answered Feb 18 '15 at 16:03

In this case, yes, this is bad practice; if it doesn't make sense to call getQuantity on a Spare_Part, it shouldn't compile. It should definitely not just return a flag value.

A redesign of your classes is probably necessary. Polymorphism should be used to model an is-a relationship, but your use is mostly to tag more data onto the class. You should use composition instead, perhaps making a Part_Store class which contains a Spare_Part, the quantity and location.

Giulio Franco · Answer 2 · 2015-02-18T17:08:00.080

Let's try sorting things out.

There's nothing wrong in having a base calling its derived class to read a property. There are cases when it's known that every implementation of a base class must have a property, and the base class offers some functionalities that require that property, and that would be the same in all reasonable implementation, except for that property. In such cases, there's absolutely nothing wrong in doing that.

As an alternative, if you're really sure that property is always going to be implemented with a simple fixed-type variable in any reasonable implementation, you can just add a protected variable to the base class, and let the implementations write to it.

It's all a matter of how much flexibility you need.

In your case, I think your conceptual inheritance model is flawed, which kind of empties your case of any meaning.

You are saying that On_hand derives from Spare_Part, which means, by definition, that every On_hand is a Spare_Part. This sounds weird to me, as being "on hand" sounds more like a quality of the spare part, than a special case of it. Maybe it'd be better to add an optional<On_Hand> to Spare_Part.

Or, even better, I suspect you want something like

struct On_Hand_Info { int quantity; string location; };
std::map<Spare_Part, On_Hand_Info> on_hand;

EDIT

Seen your last updates, maybe you should do something like:

struct IRetrievalInformation {
  virtual int getQuantity() const=0;
};

class SparePart{
  string name;
  double price;
  std::unique_ptr<IRetrievalInformation> retinfo;
public:
  Spare_Part();
  string const& getName() const { return name; }
  double getPrice() const { return price; }
  IRetrievalInformation const& getRetrievalInformation() const {
    assert(retinfo);
    return *retinfo;
  }
  IRetrievalInformation& getRetrievalInformation() {
    return const_cast<IRetrievalInformation&>(
      const_cast<SparePart const*>(this)->getRetrievalInformation()
    );
  }
};

class OnHandRetrieval : public IRetrievalInformation {
  int quantity;
  string location;
public:
  On_hand();
  int getQuantity() const final override { return quantity; }
};

PS: for God's sake, don't use both camelcase and underscores.

score 0 · Answer 3 · edited May 23 '17 at 12:14

IMO, getters and setters must be considered as a bad practice and even an anti-pattern in almost all cases. The whole point of using OOP is to conceal implementation details (well, not only this, but this is also among others). Getters/setters are intended to expose implementation details, hence it ruins one of OOP principles. In the end, why not using old plain C structs, if you are using getters/setters? In such case, classes don't have any advantages over structs.

Well, there are cases when getters/setters may look good. The point is, there are too many programmers use them as replacement for structs. And the reason usually -- they don't understand how to design application in OOP manner.

In your case you don't need getters/setters. Just think about how you are gonna use this class further. Is it gonna have any extra functionality or just store some data? If so, maybe just use POD structs?

Is it considered a good practice to define virtual get and set functions in C++?

3 Answers3

EDIT

Linked