How do I make this Open/Closed example also obey Single-Responsibility?

Question

This is a simple example, but it reflects a tension between SOLID principles that I often find myself struggling with.

A popular example of the Open/Closed Principle (e.g. [1], [2]) imagines that you have many Shape classes, and a drawShape() method, e.g.:

// Open-Close Principle - Bad example
 class GraphicEditor {

    public void drawShape(Shape s) {
        if (s.m_type==1)
            drawRectangle(s);
        else if (s.m_type==2)
            drawCircle(s);
    }
    public void drawCircle(Circle r) {....}
    public void drawRectangle(Rectangle r) {....}
 }

Using the Open/Closed Principle (OCP), we observe that this code would need to be modified for every new shape we add; we want to close the need for modification, and allow extension as a better way to add new behavior.

The examples then offer the following as an OCP-compliant solution:

// Open-Close Principle - Good example
 class GraphicEditor {
    public void drawShape(Shape s) {
        s.draw();
    }
 }

 class Shape {
    abstract void draw();
 }

 class Rectangle extends Shape  {
    public void draw() {
        // draw the rectangle
    }
 } 

As a demonstration of OCP in the abstract, this is a fine example. However, this solution seems to me a blatant violation of the Single-Responsibility Principle (SRP). We started with the "drawing responsibility" separated into its own class, and solved our OCP problem by lumping that "drawing responsibility" back in with the simple definition of a shape.

For example, if drawing involves interaction with a GUI, this example couples the definition of a simple shape together with GUI operations! That seems wrong.

What would be a good redesign here that would conform to OCP and to SRP?

Answer 1

Draw into a canvas. Drawing should not require interaction with the UI, merely the transfer of the drawn shape to the UI for it to display.

interface Shape {
    void draw(Canvas canvas);
}

class GraphicEditor {
    private Canvas mainCanvas;

    void drawShape(Shape s, Point location) {
        Canvas subCanvas = mainCanvas.subCanvas(location, s.width, s.height);
        s.draw(subCanvas);
        mainCanvas.copy(subCanvas, location);
    }
}

A more general answer:

Design your methods to transfer data, rather than have side effects. Define DTOs (like a Canvas) and pass them up the hierarchy.

Every object has collaborators. Think of them as services the parent object uses to obtain information and to perform tasks that are not part of its core responsibility (SRP). The resulting DTOs are the "product" of those services. The parent object can do with these products whatever it wishes (draw them on screen, send them over the wire, store them in a DB, ...)

Answer 2

There is another way of thinking about this.

Consider the code that is being refactored:

 // Original code
 class GraphicEditor {

    public void drawShape(Shape s) {
        if (s.m_type==1)
            drawRectangle(s);
        else if (s.m_type==2)
            drawCircle(s);
    }
    public void drawCircle(Circle r) {....}
    public void drawRectangle(Rectangle r) {....}
 }

Seems like that original code already "involves interaction with a GUI". The classes Shape, Circle, and Rectangle are already part of the original code. So it seems to me that these classes also "interact with the GUI".

If we change the names into GraphicalShape, GraphicalCircle, and GraphicalRectangle, the confusion might be erased.

---

Another way to phrase it:

What the refactoring did (to conform to the OCP) is to move the code (which "involves interaction with the GUI") from drawCircle(Circle c)...

public void drawCircle(Circle c) {
    // draw the circle, c
    // might have "involved interaction with a GUI"
}

...into the draw() method inside the Circle class

 class GraphicalCircle extends GraphicalShape {
    public void draw() {
        // draw the circle, this
        // might "involve interaction with a GUI"
    }
 } 

And you should remember that there is a goal for doing that. (You do not do it just for the sake of conforming to the SOLID principles.) The goal is so that you will not need to modify the GraphicalEditor class if you need to add another Shape. Your GraphicalEditor class is closed for modification (in relation to drawing new shapes) but open to extension (of drawing new shapes).

(If you knew in the first place that you will be drawing only circles and rectangles, you will not need to do the refactoring to conform to OCP.)

By obeying the OCP you changed the responsibility of the GraphicalEditor. Previously it has two responsibilities: (1) orchestrate the drawing of shapes (2) draw the shapes.

Now it has only one responsibility: orchestrate the drawing of shapes. You removed the responsibility of drawing the circle from the GraphicalEditor into the GraphicalCircle class. And the responsibility of drawing the rectangle into the GraphicalRectangle class.

---

One more way to phrase it:

The purpose of the Circle class is to contain the responsibilities of a GUI for a circle. So it obeys the SRP even when it contains code that interacts with GUI.

The purpose of the Circle.draw() class is to contain the responsibility for drawing GUI for a circle. So it obeys the SRP even when it contains code that interacts with GUI.