2

I'm struggling to find good ways to split up classes without exposing private data. Most articles I read about SRP seem to ignore how the new classes that take on the separated responsibilities access the data that once used to be private to the original class.

Take, for example, a Gripper class representing a robotic gripper in a graphical computer simulation. This class handles the logic of a gripper, pickup up items, rotating them, putting them down in a different position, etc. The gripper can also draw itself onto a GUI.

This breaks the SRP, because there are 2 reasons for the Gripper class to change: Changes to the logic as to how a gripper operates, and changes as to how a gripper is drawn. However, Gripper has some private data members that are used by both the logic and the drawing portion. Simply exposing those members trough some (const) getters feels like a step backwards. I'd be exposing implementation details, tying myself down to supporting this new "interface" and it seems downright wrong.

So I came up with this:

class Renderer
{
public:

    /* Takes the data needed to draw a gripper and does so. */
    void
    DrawGripper(const Foo& foo, const Qux& qux);

    /* Additional methods to draw other things. */
};

class Gripper
{
public:

    void
    Draw(Renderer& renderer) const
    {
        renderer.DrawGripper(mFoo, mQux);
    }

private:

    Foo mFoo;
    Bar mBar;
    Qux mQux;
};

Pro's:

  • Better separation of responsibilities. Apart from the Draw function, consisting of 1 line of code, all the drawing code is now gone from Gripper.
  • Renderer could be a abstract interface, easily allowing different implementations.
  • Data can be passed by const reference to DrawGripper whereas a plain memberfunction would have total access to all members.

Con's:

  • Gripper still has a Draw function and knows about Renderer.

I feel the con is manageable tough. In the end, one of the reasons for Gripper to exist is to eventually be drawn onto the screen, so the fact that it still has a Draw function does not seem too bad. Perhaps this is a case of having to choose the lesser of 2 evils? The alternative of exposing private data being far worse imho.

Am I on the right track here? Is this a good system that can be deployed in cases like this? Any problems or better ways?

6
  • 2
    "Simply exposing those members trough some (const) getters feels like a step backwards." Um... why? Feb 8 '20 at 4:35
  • 1
    @NicolBolas because you expose the internals and create a hidden coupling to a particular implementation. For example imagine today you'd use an absolute (x,y,z) cartesian coordinate and tomorrow you'd go for a relative (d, θ, 𝛼) angular system because it represents better the servo's position. You'd no longer be able to provide the absolute x, since you no longer keep track of it. But since it's public, either you change all the client, or you'll have to keep track of your moving origin, even if you'd know that after thousands of operations and cumulated rounding error it's not reliable.
    – Christophe
    Feb 8 '20 at 10:14
  • 2
    Sometimes, a responsibility of a class is to coordinate other classes. When you separate two pieces of logic, but there are still things that are in common, then, depending on the overall design and on what you are truing to do, you can either: (1) have those two classes share a reference to a third object (and possibly consider that object to be internal to the module), or (2) have a third class coordinate the two classes you've separated out by calling methods and passing parameters, or (3) even duplicate the data if the commonality is only superficial, and the classes evolve independently. Feb 8 '20 at 11:33
  • BTW, it all depends on how the code actually changes over time - if the exact way you draw the Gripper changes relatively often, but the signature of the Draw method on Gripper practically never changes, and if Gripper having that method works well within your application, than it's not really a SRP violation. In that case, what you've done is: you've recognized that, in practice, drawing is a separate responsibility that requires more flexibility, and you've decoupled that, separating it out - you've applied SRP strategically, where it mattered. Feb 8 '20 at 11:43
  • @Christophe Exactly! If there was no need for a gripper to be drawn everyone would be talking "Tell, don't ask", "A gripper should be able to perform everything it can do trough it's own public interface without exposing implementation details", etc. But now that we have to be able to draw it it's suddely fine to sprinke getters everywhere. Feb 8 '20 at 12:41
5

IMO, from your new approach, both Renderer and Gripper classes break the SRP:

  1. Renderer will need knowledge of all specifics of each item to be rendered; likely to become a god class for this matter (think of it as: this class has multiple sources or reasons to change);
    • In addition: for every new object to be added in this simulation, you will need to "insert" code into this class (this is what the "O" principle is against: code should be open for extension but closed for modification);
  2. Gripper class, as you said, still has the draw method;

Suggestion

  1. Don't use only SRP; try using all SOLID principles, let's take for example "I" (program to interfaces, not concretions);
  2. Create an interface (or abstract class in cpp) that represents a drawable object (Renderer);
  3. Implement every concrete renderer (e.g.: a GripperRenderer -- all specific stuff this concrete class needs should be passed on its constructor, otherwise you can't standardize the Draw method signature);
  4. Leave you Gripper class only with the responsibility of handling its behavior/logic; all information that needs to be accessed by other classes should be exposed with getter methods;
  5. On your simulation "engine", when you instantiate a Gripper you should also instantiate its GripperRenderer, and pass to the render all info that it needs for the rendering operation.

Sample source

Please, find below a quick illustration to my suggestion above (I apologize if there are any coding errors):

class Renderer
{
public:

    // Only has a draw method, according to this class' responsibility
    virtual void Draw() = 0;

};

class GripperRenderer : public Renderer
{
public:

    GripperRenderer(const Foo& foo, const Qux& qux) {
        // Receives any data, or pointer to data, needed
        // for the rendering
    }

    void Draw() {
        // draws the gripper, according to class' responsibility
    }

};

class Gripper
{
public:

    // Methods that handle only gripper logic, according to class' responsibility
    ...

    // Getters for stuff used by external objects
    Foo& getFoo();
    Qux& getQux();

private:

    Foo mFoo;
    Bar mBar;
    Qux mQux;
};


IMO, this is understandable and easily maintainable.

Perhaps, you should point to a more complex, real-world example of your problem, if you need further help with this.

5
  • 3
    +1 Excellent analysis: SOLID is just a way to achieve a better separation of concern and they should not be seen in isolation. Gripper now has its own independent purpose, renderer with GripperRenderer taking care for the graphical part, whereas Gripper could be specialized into a RealGripper that drives real robotics ;-)
    – Christophe
    Feb 8 '20 at 12:07
  • I still find the getters unacceptable. To paraphrase a popular example: You wouldn't expose the Dog class's Legs, everything a Dog` can do should go trough Dog's interface. Allowing access to the Legs directly would be universally frowned upon in OOP. Now imagine I need to be able to draw a Dog which means also drawing the Legs. Now all of a sudden it's fine to expose the Legs? Feb 8 '20 at 12:22
  • I was under the impression the SRP means each class should only have a single responsibility, not only have one function. Drawing all the things is a single responsibility is it not? Feb 8 '20 at 12:44
  • 2
    Quoting SRP's author: "The Single Responsibility Principle (SRP) states that each software module should have one and only one reason to change" (blog.cleancoder.com/uncle-bob/2014/05/08/…). You original Renderer class has multiple reasons to change; and its caller would even need to pass the specifics as parameter. It would be better to create smaller classes, and more specific. About the "getters", you can maybe try to find another way to pass this information, but thing is: this information is needed by an external class. Feb 8 '20 at 13:15
  • 2
    About your dog analogy: Dog's legs are totally visible to an external entity seeing the dog. Private stuff means things that are not mean to be seen or to be accessible to anyone externally. If someone needs to see the dog's legs to be able to draw it, what's the problem? You should think about encapsulation: you need to draw a dog, then you should be able to see its publicly visible representation; however you don't need to see/access its internals, because this is actually useless for your task. Feb 8 '20 at 13:18
2

Shall Gripper hold the Renderer? I would suggest to let Gripper provides data Gesture which is immutable and Renderer ask for Gesture. While operation above is done by an object for screen redering.

By exposing Gesture as interface, who changes the Gripper would be aware of the exposing data shall not be changed.

Refactoring Foo would not break Renderer.

Also it reduce the possiblilty letting not related object Bar is used by Renderer in the furure.

0

If you read about SRP, and you think that you should split up a class because of SRP, and then you run into the problem that it causes you to expose private members that you don't want exposed, then you should step back and consider this:

SRP is there to make your life easier. If following SRP makes your life harder, then it is quite likely that you are misunderstanding SRP. SRP is probably the most misunderstood guideline of them all. And even if you got it right, if it makes your life harder instead of easier, then DON'T DO IT.

In your case: Changing how the robotic arm works quite likely makes it necessary to change both the code how the gripper works, and the code how to render it.

Of course you might have written rendering code that renders using OpenGL, and you want to change it to render using Metal. Well, in that case your problem is somewhere else completely. The rendering code should just produce a description of the items to render and should be independent of the technology used.

0

Often, an object containing complex state has the high-level responsibility of delegating things to other objects, which it configures using its knowledge of its state.

A Renderer object could be responsible for creating a graphical representation of the current state of the Gripper and would thus be configured by the grabber with information derived from the grabber's state.

If this other object is part of the same module as the grabber, they can share non-public code for the representation of elements of their internal states.

But except in very simple cases, its rare that two related objects with connected responsibilities share their entire state representation.

The Renderer would in turn also only do what it is capable of due to its knowledge of its configuration provided by the gripper, e.g. creating a geometrical representation in a form compatible with the drawing API in use.

0

I've decided to go with something along these lines:

class GripperRenderer
{
public:

    virtual
    void Draw(const Foo& foo, const Qux& qux) = 0;
};

class Gripper
{
public:

    void
    AcceptRenderer(GripperRenderer& renderer) const
    {
        renderer.Draw(mFoo, mQux);
    }

private:

    Foo mFoo;
    Bar mBar;
    Qux mQux;
};

Having getters as per @Emerson Cardoso's answer is simply unacceptable. It exposes implementation details that, absent the need for drawing, no-one has any business with. For example, a GetPosition() method would expose, and forever tie me down to supporting, a cartesian coordinate system. Clients can (and will) over time start to use the GetPosition() method for their own purposes, while they're not supposed to. Any changes to the internal coordinate system in future now require supporting conversions to keep the GetPosition() method working, which is cumbersome, not even possible in some cases, and definitely does not help maintainability.

This is the thinking/motivation behind the famous article "Why getter and setter methods are evil" by Allen Holub, I believe.

Yes, Gripper has a AcceptRenderer() method and thus knows about GripperRenderer. This is a necessary evil because the only way to access Gripper's private members is trough/with permission of Gripper itself. One can look at it as Gripper's way to explicitly allow access to it's privates for the sole purpose of drawing. Which is more fine-grained then using friend (only the members required for drawing are passed) and definitely much better then exposing our implementation details to everyone. The responsibilities are still split.

Could one write a GripperRenderer implementation that simply accepts the data and then exposes it? Certainly. But you cannot protect against intentional evil/stupid. Doing so should be a code smell to any reasonable programmer, while using a public getter - if it were to be there - is a perfectly reasonable thing to do.

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.