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

  • +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
  • 1
    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

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.