10

I'm new to OOP and learning design patterns so I wrote some simple code to try out a Factory Method, and all seems well, except when I want to add another sub-type. Here's the code so far:

public interface Person {
  public String getDescription();
}

public class Adult implements Person { 
  @Override
  public String getDescription() {
    return "I am an ADULT";
  }
}

public class Child implements Person {
  @Override
  public String getDescription() {
    return "I am a CHILD";
  }
}

public class PersonFactory {
  public Person create(int age) {
    if (age < 18) return new Child();
    return new Adult();
  }
}

public class ClientA {
  public static void main(String[] args) {
    PersonFactory personFactory = new PersonFactory();
    Person person;
    person = personFactory.create(80);
    System.out.println(person.getDescription());   
  }
}

If the requirement changes later to include a sub-class Pensioner for when the age is > 70, I would have to either:

Add the line if (age > 70) return new Pensioner(); to the create() method in the PersonFactory class, which surely breaks the Open-Closed Principle? Or, as suggested in The Gang Of Four Design Patterns book, override the parameterized factory method to selectively extend the products a Creator produces. In this case I think that would mean writing a new class:

public class PersonFactoryWithPensioner extends PersonFactory { 
  @Override
  public Person create(int age) {
    if (age > 70) return new Pensioner();
    return super.create(age);
  }
}

This now means that either all the clients which call the PersonFactory would now have to be changed to use PersonFactoryWithPensioner instead, or I have to accept that new clients could call PersonFactoryWithPensioner whilst the old clients eg. ClientA would still only receive an Adult object when the age is > 70. It gets even worse if later on another sub-class eg. Infant is added. To ensure the new clients receive whichever object of Infant, Child, Adult or Pensioner is appropriate, a new class PersonFactoryWithInfant would have to extend PersonFactoryWithPensioner. This can't be right, seems more likely I have misunderstood what GoF suggest.

My question is: Is there a way to add a new sub-type that can be returned to old clients without changing them, and without breaking the OCP by changing the PersonFactory code to include the new sub-type?

Apologies if I have not posted this correctly, it is my first time posting a question here. I have looked through previous answers for similar problem but they don't seem to quite address this.

alayor
  • 4,537
  • 6
  • 27
  • 47
Andy Tipp
  • 109
  • 4
  • Did you read this : http://softwareengineering.stackexchange.com/questions/302780/does-the-factory-pattern-violate-the-open-closed-principle – jaudo Jan 20 '17 at 16:39
  • i think breaking the open closed principle is the way to go since your current implementation only supports creating children or adults and changing the method by adding in if statements is a lot easier and cleaner then creating multiple classes that extend the factory. – RAZ_Muh_Taz Jan 20 '17 at 17:04
  • This is not the Factory Method pattern. This example is commonly called a Simple Factory. It is not a GoF pattern. – jaco0646 Feb 13 '17 at 16:50

5 Answers5

4

I think OCP doesn't stop from from modifying any method or class.

But, it proposes that if you need to do any modication, you should do it so that you don't have to modify that code again.

Given that you may need to modify PersonFactory later - you could create yet another Factory class to create objects of type PersonFactory. Although this seems like over-engineered solution.

Another possible solution would be that PersonFactory load this rules from some dynamic source, for example, save this rules in a file using JSON format. And then create objects dynamically using reflection.

Something like this:

 private static JSONObject RULES;
   static {
       RULES= JSON.parse(rulesEngine.load());
   } 
    public class PersonFactory {
      public Person create(int age) {
        String personToCreate = RULES.get(age);
        Constructor<?> ctor = Class.forName(personToCreate).getConstructor();
        return (Person) ctor.newInstance();
      }
    }

The json rules would be something like this:

{
  "1":"Child.class",
  "2":"Child.class",
  ...,
  "17":"Child.class",
  "18":"Adult.class",
  ...,
  "69":"Adult.class",
  "70":"Pensioner.class"
}

This way you don't break OCP Principle.

alayor
  • 4,537
  • 6
  • 27
  • 47
  • you are proposing that the rules be reloaded every time the method is called. I doubt the OP was interested in run-time modifications to rules; loading and parsing it once, say via static property, would be enough. – tucuxi Jan 20 '17 at 17:00
  • Yeah, maybe that loading in runtime is not good. But my point is different - my point is that you can change those rules dynamically. – alayor Jan 20 '17 at 17:06
  • 1
    you have shifted the burden from "update the PersonFactory to the latest version" to "update the PersonFactory rules file to the latest version". – tucuxi Jan 20 '17 at 17:08
  • And that was main purpose - to avoid class recompilation on classes that depend on `PersonFactory` (which is the main purpose of OCP). And shift this change to a file. – alayor Jan 20 '17 at 21:25
2

Rules are sometimes meant to be broken, so I say BREAK the Closed Principle to keep it clean and simple. The overhead of creating multiple Factory Classes for each type of person breaks the entire Factory Method purpose in my opinion. Breaking the Closed Principle allows you to have a single class to create any type of person.

  public Person create(int age) {
    if (age < 4) return new Infant();
    if (age < 18) return new Child();
    if (age < 70) return new Adult();
    return new Pensioner();
  }
RAZ_Muh_Taz
  • 4,059
  • 1
  • 13
  • 26
  • As mentionned, this modification would break the "Closed" principle – jaudo Jan 20 '17 at 16:54
  • ok i'll have to look into what the closed principle is, thanks @julaudo – RAZ_Muh_Taz Jan 20 '17 at 16:58
  • While this does break OC, I've upvoted because I feel that it is entirely justified to do so - my own answer I see as over-engineered for most requirements, and the same goes with with @alayor 's – tucuxi Jan 20 '17 at 17:50
2

The open-closed principle is good to keep in mind. It does not work nicely with factories, however. One option that sort-of-works is the following, which turns the factory into a registry:

    PersonFactory pf = new PersonFactory();
    // Java 8 lambdas are great!
    pf.register((age) -> age < 18 ? new Child() : null );
    pf.register((age) -> age >= 18 ? new Adult() : null );

    System.out.println(pf.create(10).getDescription());

Similarly to @alayor's answer, the only way to avoid having to modify the logic of the factory, or having to replace the factory altogether and get everyone to use the new version... is for the factory to get its logic from elsewhere. @alayor gets it from a configuration file; I propose adding it to the factory as part of its initialization (could be done in the factory constructor too; changing it to, say, public PersonFactory(PersonCreator ... rules) ).

Full code:

interface PersonCreator {
    Person create(int age);
}

class PersonFactory {
    private List<PersonCreator> pcs = new ArrayList<>();

    public void register(PersonCreator pc) {
        pcs.add(pc);
    }

    public Person create(int age) {
        for (PersonCreator pc : pcs) {
            Person p = pc.create(age);
            if (p != null) {
                return p;
            }
        }
        return null;
    }
}

interface Person {
    public String getDescription();
}

class Adult implements Person {
    @Override
    public String getDescription() {
        return "I am an ADULT";
    }
}

class Child implements Person {
    @Override
    public String getDescription() {
        return "I am a CHILD";
    }
}

public class Main {
    public static void main(String[] args) {
        PersonFactory pf = new PersonFactory();
        // Java 8 lambdas are great!
        pf.register((age) -> age < 18 ? new Child() : null );
        pf.register((age) -> age >= 18 ? new Adult() : null );

        System.out.println(pf.create(10).getDescription());            
    }
}
Community
  • 1
  • 1
tucuxi
  • 17,561
  • 2
  • 43
  • 74
  • I am not the downvoter; however, OCP works perfectly well with the GoF factory patterns. The problem in this thread (and many others on SO) is that the example given is _not_ a GoF factory pattern. The example given is commonly called a Simple Factory, which _does_ violate OCP, but it's critical to note that the Simple Factory is unrelated to the more famous GoF patterns. – jaco0646 Feb 13 '17 at 16:48
  • I fail to see how any factory can avoid coupling with the constructors actually needed to do the building - unless dependency injection or a registry is used instead. Adding a new class will always require making a new constructor call somewhere in the factory. GoF examples invoke constructors, and are therefore not very extensible. Indeed, AFAIK, a major goal of factories is to encapsulate this non-extensible point in a single location. – tucuxi Feb 14 '17 at 08:42
2

All answers here, that suggests some kind of dynamic rules are in fact breaking open-close principle. This principle isn't about "don't change a piece of code that is already written" but "don't change a outcome of code already in use". That said, if a client expects that it only can get two results - Adult or Child then providing third possibility either by hardcoding it into function or by dynamic rulesets is breaking of open-close principle.

But returning to your question - I'll say it depends. Principles and patterns are nice, fun and all but in real day-to-day work one must always look at the big picture and decide if to apply certain rule or not. Treat them as hints, not something that's written in stone.

If your code is somewhat closed, that is you have control of every invocation of PersonFactory, then changes are just normal thing in the lifecycle of your software. I don't recall any real life project I've participated into, that hasn't changed any bit of code created previously. In fact we're doing it on the daily basis :)

Other thing is when your code is used by unknown number of third party clients (public API for example). Then you should be careful not to break something, but also presenting new logic in existing methods (as here when you add new concept of Person) is perfectly acceptable. If those would be breaking changes then consider adding new/upgraded version of changed code alongside old one (and possibly have a plan of deprecating old version sometime in the future as you really don't want to end up maintaining 10000 versions of your code ;)

Also remember about other OOP pieces that should help you avoid some problems. In your example Adult, Child and Pensioner all implements Person interface which is great. So any code that knows only Adult and Child implementations should not have problem with using Pensioner value as all of them are just implementations of Person and that code should treat Pensioner also as Person without even knowing you've introduced a new type.

Vir
  • 642
  • 3
  • 10
1

The question is "do the client expect a Pensioner object to be created without code modification?". If yes, then you should break the "Closed" rule and update you factory code. If not, then you should create a new factory and the clients will use it.

jaudo
  • 2,022
  • 4
  • 28
  • 48