0

Factory pattern violates the OCP principle because it uses if() statements, which implies that if any class is added then the factory class has to change, being against SOLID principles. Self registering classes are supposed to address this problem according to this resource: http://www.jkfill.com/2010/12/29/self-registering-factories-in-c-sharp/. The problem is that i don't know C#. Can someone make an example of this in Java? Thanks in advance.

public class ShapeFactory {

   //The purpose of self registering classes is to avoid if's
   public Shape getShape(String shapeType){
      if(shapeType == null){ //Get rid of this
         return null;
      }     
      if(shapeType.equalsIgnoreCase("CIRCLE")){
         return new Circle();

      } else if(shapeType.equalsIgnoreCase("RECTANGLE")){
         return new Rectangle();

      } else if(shapeType.equalsIgnoreCase("SQUARE")){
         return new Square();
      }

      return null;
   }
}
  • so you actually want an abstract factory implementation? – Olimpiu POP May 16 '20 at 01:37
  • classes that when they are created notify some kinda registry class without having to add if statemets or change the factory class. – Tundra Spirit May 16 '20 at 01:42
  • Using switch or enum? – PatrickChen May 16 '20 at 02:21
  • 1
    The GoF Factory Method pattern is based on inheritance. It does not violate the OCP. That is, unfortunately, a very common misconception. – jaco0646 May 16 '20 at 15:39
  • A bit more information: the example here is known as a Simple Factory. It is described in great detail in the book _Head First Design Patterns_ where they mention its popularity, but explicitly point out that it is _not_ a GoF design pattern and _not_ a Factory Method. See also: [What are the differences between Abstract Factory and Factory design patterns?](https://stackoverflow.com/a/50786084/1371329) – jaco0646 May 20 '20 at 13:17
  • "Factory pattern violates the OCP principle because it uses if() statements" - in that case, Java itself violates OCP principle for including the `if` keyword in the language. I think you are taking the OCP principle if you think that simply using `if` statements constitute a violation of OCP. – hfontanez Feb 14 '21 at 21:25
  • Another thing I think people get wrong with factories is that they believe the type of things factories build is boundless. They always argue "what if you need..." So, instead of showing an example of a factory of shapes, you should have a factory per shape type. For example, a factory for triangles, and a separate factory for 4-sided objects. Now, your factories are very finite into what they are responsible to build. You can then have a factory of factories where you can expand its capabilities by the number of sides they support. – hfontanez Jun 14 '21 at 19:00

2 Answers2

5

That self-registering thing is a bad idea. Eventually, it will be extremely difficult to know which factories are actually registered, and what their names are, and which name strings are thereby supported.

It's better, and easy, to keep things straightforward. Usually, it would be something like:

  • ShapeFactory should have a Map<String, Supplier<Shape>> that maps shape type strings to the corresponding factories; and
  • ShapeFactory, or a builder for it, should have an addShapeType(String,Supplier<Shape>) that is used to register all the types while creating a ShapeFactory instance.
Matt Timmermans
  • 53,709
  • 3
  • 46
  • 87
  • "... or a builder for it..." - What do you think of this illustration of the Builder Pattern I came up with in this video? https://youtu.be/2wieA-lr1Uk – hfontanez Feb 15 '21 at 01:30
1

Full Demo in my github project.

Open-Closed Principle states that software entities must be open for extension but closed for modification. Let use the OP's example:

public class ShapeFactory {

   //The purpose of self registering classes is to avoid if's
   public Shape getShape(String shapeType){
      if(shapeType == null){ //Get rid of this
         return null;
      }     
      if(shapeType.equalsIgnoreCase("CIRCLE")){
         return new Circle();

      } else if(shapeType.equalsIgnoreCase("RECTANGLE")){
         return new Rectangle();

      } else if(shapeType.equalsIgnoreCase("SQUARE")){
         return new Square();
      }

      return null;
   }
}

Suppose that I need to add three new shapes: Triangle, Pentagon, and Hexagon. OCP tells me that I should not MODIFY the existing ShapeFactory to support these new shapes, but instead I should EXTEND the existing class to support them. So, how do I do this? I can literally extend ShapeFactory and add the three new shapes to this sublass. That would be a terrible mistake. So instead, I would create an AbstractShapeFactory that ShapeFactory as well as my new factory class will extend. THAT SAID, I prefer using interfaces over abstract classes. So, I would create a shape factory interface, and then my concrete factories will implement said interface.

Without getting too much into Liskov Substitution Principle, you have to make sure you don't end up violating this principle in the process. In short, you will end up with something like this:

interface ShapesFactory {

    // Shape should also be an interface
    public Shape getShape(String name);
}

public class BasicShapesFactory implements ShapesFactory {

   @Override
   public Shape getShape(String name){

      String errorMsg = name + " is not a legal name for a Shape.";
      if(name == null){ //Get rid of this
         throw new IllegalShapeException(errorMsg);
      }

      switch(name.toUpperCase()) {
          case "CIRCLE":
              return new Circle();
          case "RECTANGLE":
              return new Rectangle();
          case "SQUARE":
              return new Square();
          default:
              throw new IllegalShapeException(errorMsg);
       }
   }
}

public class AdvancedShapesFactory implements ShapesFactory {

   @Override
   public Shape getShape(String name){

      String errorMsg = name + " is not a legal name for a Shape.";
      // This null check could be extracted to a default function in the interface
      if(name == null){ //Get rid of this
         throw new IllegalShapeException(errorMsg);
      }

      switch(name.toUpperCase()) {
          case "TRIANGLE":
              return new Triangle();
          case "PENTAGON":
              return new Pentagon();
          case "HEXAGON":
              return new Hexagon();
          default:
              throw new IllegalShapeException(errorMsg);
       }
   }
}

Now that you have this structure, you can either create Abstract Factory that will provide a factory for basic shapes or advances shapes.

I think it is misconception that adding if statements is a violation of OCP. The code is going to be modified. Adding classes is a modification of existing code. The real problem is WHERE and WHY the modification occur. You get no argument from me that using something like a Bean Factory is a better solution, but the concept is the same. With OCP, all you are doing is protecting what "ShapeFactory" is by extending it, and not by modifying it.

hfontanez
  • 5,774
  • 2
  • 25
  • 37
  • You backed up your arguments very well, but new advanced shapes will be added to the switch chain whether you like it or not. I challenge you to write a task to 10 people and ask what would they do. I know you can't control what devs would do next, but I still believe an approach with some kind of mapping strategy would prevent this OCP violation scenarios – Guilherme Alencar Jun 07 '21 at 15:10
  • 1
    @GuilhermeAlencar In my last statement, I mention that there are better solutions. However, the point of my post was to clarify that "closed for modification" doesn't mean what the OP thinks. OP wrote: `Factory pattern violates the OCP principle because it uses if() statements`. This is not true at all. I believe Robert Martin himself has stated so. In this case, the original factory contains a switch that is not modified after initial release. The functionality is extended by EXTENSION, not by MODIFICATION. – hfontanez Jun 07 '21 at 15:43
  • Agreed. I missed the better solutions, can you point out ? – Guilherme Alencar Jun 07 '21 at 16:05
  • 1
    @GuilhermeAlencar IMO, a real self-registering factory will have to use a similar mechanism used by frameworks like JUnit to auto-register tests. For example, you could tag classes with `@Factory` in order to alert your framework that the function of the class is that of a factory. You could even extend this and have factories of factories using such mechanisms. BUT, you may still to do modifications to the framework; i.e. the framework itself might contain `if/else` blocks and this doesn't necessarily mean that it violates OCP (which was the point of my original post). – hfontanez Jun 09 '21 at 13:53
  • 1
    I appreciate your comments – Guilherme Alencar Jun 09 '21 at 14:29