7

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?

14

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, ...)

| improve this answer | |
  • +1, absolutely. Whenever possible, abstract away the detailed type of the actor (Shape vs. Rectangle) as well as its collaborators (Canvas vs. System.out). – Kilian Foth Mar 20 '17 at 12:09
  • So: The Shape would be coupled to the DTO, in this case the Canvas, and that would fall under the "original" responsibility of a representation of a shape. Whereas the responsibility of transforming from the DTO into some final product is nearly separated. Did I understand you correctly? – Standback Mar 20 '17 at 12:14
  • 4
    @Standback Yes, you did. Every class is coupled to the DTOs it uses to commincate - just like a getName() method is couled to String (or at least char). Do not think of the GraphicEditor as converting the DTO from the shape to a "final product". Think of the Shape as a service that the Editor uses and the drawn on Canvas as the product of that service. The Editor then is then free to do with that product whatever it wishes (draw it on screen, send it over the network, ...) – marstato Mar 20 '17 at 12:18
  • 1
    This is a great answer, but something about it is still bugging me. You're saying Shape isn't entirely closed - it could always require the addition of a new DTO, depending on how it will be used. So, if yesterday, Shape didn't need to be drawn anywhere (say, I was only using it to calculate stuff, never for any graphical display), and today it does -- then even though this is a "new" responsibility, even though Shape is "closed to modification," I'm still modifying Shape to cooperate with the new Canvas DTO. That feels like coupling the structure, to the way it will be used. No? – Standback Mar 20 '17 at 21:11
  • 2
    If you close all of your domain classes to modification, you'll not be able to implement such big changes as "visualize a calculation process". Design principles are a guidline, not rules written in stone. Also: You could design your shape class to define shapes as a combination of lines and arcs. You could then use the shape as a mere DTO and have a ShapreRenderer and e.g. a ShapeIntersector. Instead of subclasses you now have factory methods such as Shape.createRectangle() or Shape.createEllipsis() If that satisfies your question, I'll edit the answer to include this comment. – marstato Mar 20 '17 at 21:30
0

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.

| improve this answer | |

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.