4

Say I have a large class called Root, which has a lot of members & functions:

class Root {
public:
    void func1(); // operates on a and b
    void func2(); 
    ...       
private:
    A a;
    B b;
    C c;
    ...
};

The class Root is getting a bit big so I decide to factor out a few of the member variables and a function into another class:

class AB {
public:
    void func1();
    ...
private:
    A a;
    B b;
};

Then using composition I can make AB ab a member variable of Root. Root is now a bit smaller / cleaner / more manageable. The question is about the access level of ab after this composition refactor.

Is it OK in this case for ab to be a public member? Any downsides? Public members are generally considered bad design from what I've read. But here the overall encapsulation / exposure hasn't actually changed...the objects a and b are private within AB so are still hidden from the outside.

(Sidenote: the reason why I would like to make ab public is that it allows easier traversing of Root's composition hierarchy. For example in Root's member c I might want to call func1:

void C::foo(Root& root) {
    //do some stuff
    root.ab.func1();
}

If ab was private then I'd have to define a pass-through function in root.)

Improve this question
1
  • The answers are good, but I would also say that as an intermediate step of refactoring, what you are doing is okay. Just remember that it's not the end of the road, and more refactoring is required. Commented Dec 14, 2015 at 12:42

2 Answers 2

Reset to default
3

If class C wants to call func1() then pass the ab object to C's foo function instead of passing the root object. This better describes what is going on to other programmers and future you. It tells you that C does't need all of Root to do its job, it only needs AB (and it could conceivably get an AB from some source other than Root.)

void C::foo(AB& ab) {
    //do some stuff
    ab.func1();
}

If I were you, I would also go through and change any other class that is accessing Root merely to call AB's func1() (and whatever other functions you move into the AB class.

Do this with an eye toward breaking up Root in other ways as well. If it is very large like you say, then there are likely lots of little classes trying to get out...

Improve this answer
4
  • Passing ab makes a lot more sense actually, good point! Commented Dec 14, 2015 at 9:24
  • Quick addition, if multiple C functions depend on an AB object, would it be OK to store a reference to an AB as a private member variable of C? Or just pass in ab to the functions that need it? Is either approach preferred? Commented Dec 14, 2015 at 14:53
  • Most of the member-functions in the class should use most of the member-variables most of the time. If most of the functions of C use the ab object (and they all should use the same ab object,) then yes, pass one in using a setter, or maybe better would be to have the C class' constructor take an ab object. Of course if none of the other member-variables of Root need the ab object (or they can deal with a different ab object,) then maybe Root shouldn't maintain a reference to ab at all. Commented Dec 15, 2015 at 1:15
  • I can't upvote yet, so I'll say thanks for your help! Commented Dec 15, 2015 at 7:06
3

From both an academic and a technical point of view, it is not okay.

From an academic point of view, you are violating the principle of encapsulation.

From a technical point of view, if you need to do any further refactoring in the future, you will have a lot of work to do.

That having been said, there is also a practical point of view, which boils down to this: if your IDE gives you the ability to refactor ab from public to private, adding all necessary accessors and replacing all references to root.ab with invocations of these accessors all over your entire code base, and if it can do that with just a couple of clicks and keystrokes, you are fine. If not, you are better off not doing it.

It is not clear to me what you mean by sub-object c, but if you mean a subclass, then consider making ab protected. This way, c will be able to access it, but no outside code will.

Improve this answer
3
  • 1
    Sorry, just edited out the confusing subobject terminology. To be clear there is no inheritance going on, only composition Commented Dec 13, 2015 at 20:13
  • Can you be more specific about how this violates encapsulation? The external access to each object/function to me seems identical before/after the refactor Commented Dec 13, 2015 at 20:17
  • rootInstance.ab.function1() is still violating encapsulation, because users of rootInstance have to know that it contains an instance of ab. It is highly academic, and I personally don't usually mind doing it, provided that my IDE that can hide it from view, should the need arise, at a moments' notice. Nowadays I work mainly with java, where the IDEs offer powerful refactoring features. In C++, the pickings are a bit slim. For example, if you are using Visual Studio, you can't do much unless you add something like "Refactor!" or "Visual Assist". Commented Dec 13, 2015 at 20:25

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.