1

I am having trouble thinking through what best-practice dictate I do in this situation.

Say I have the following class structure

Class A {
public:
    bool isCompressed() { return compressed_; }
private:
    bool compressed_;
}

Class B {
private:
    ClassA *a_ptr_;
public:
    /* Accessor here */
}

If I needed to get access to the compressed_ variable in ClassA within an implementation having a ClassB object should I have an accessor like this:

ClassA* ClassB::filePointer() const {
    return a_ptr_;
}

and implementation of

if (objectA.filePointer()->isCompressed()) { ... }

or this:

bool ClassB::isFileCompressed() const { 
    return a_ptr_->isCompressed(); 
}

and implementation of

if (objectB.isFileCompressed()) { ... }

I realize this might be a bit of a best-practice question (or maybe it isn't...?) but hopefully can tell me which way one should be going about this!

  • 2
    The former overexposes the implementation details of class b, so the latter is better from that regard. – Erik Eidt Jun 16 '16 at 4:47
  • 1
    Check law of demeter, it pretty much answers your question. – Andy Jun 16 '16 at 6:36
2

Is returning a private pointer is a bad idea ?

By making a member private, you express that this is the class internal business, and that you want to ave the freedom to manage such interals as you want.

The problem when returning a private pointer, you give your control away, breaking your own design intention:

  • What happens if the caller deletes the pointer, accidentally thinking that it was a pointer to copy for his own use ?
  • What happens if the caller invokes members of the pointed object that alter its state (e.g. close the file, while your B class assumes it remains open) ?
  • What happens if the caller stores a copy of the pointer and uses it later ? Especially if you have deleted the object in the meantime ?

You may hope that adding the const keyword avoids these problems. Unfortunately A* B::filePointer() const {...} just means that this member function doesn't change the state of B object.

You could mitigate a little bit the risks, by making it const A* B::filePointer() const {...}. In this way, the caller could only invoke const member functions on the file pointer, and assign it co a pointer to const. Unfortunately, this will not prevent accidental deletion.

Online demo of the issue

The other drawback is that consumers of class B depend also on the internals of that class. This doesn't respect the principle of .

Encapsulation and separation of concerns

Your "Chained accessor" alternative looks much better:

  • it uses encapsulation effectively to hide the implementation details completely. If tomorrow you prefer to use a stream instead of a class A, you can do it: the world outside class B doesn't have to know anything about your change.
  • it promotes separation of concerns : the class B doesn't have to know anything about the internals of class A, because it only uses a public member. And the outside world doesn't have to know anything about class B internals, and thus doesn't depend on class A.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.