Home > Java, Miscellaneous > Case Study: Factory Design Pattern

Case Study: Factory Design Pattern

I had a job to check our project code quality. And have to report it back to my team leader for any obstacle that i found in the project.  I found a lot of leaks and i think would be good to be discussed on the blog. Not to mock the author, but to learn and improve ourselves together.

Like this code, this is the part that i found in our code.

 public ContactInfoBean(final Reseller resellerInfo) {

        switch(resellerInfo.getType()) {

            case PROGRAM_CONTACT:

                readExecutiveInfo(resellerInfo);

                break;

            case FILE_CONTACT:

                readOperationalInfo(resellerInfo);

                break;

            default:

                break;

        }

    }

The code works fine, and do its job pretty well. But some problem will appear by using this code-style. This class will grow tailing the biz changes, as usual, the bigger one class, the “merrier” to maintain it is. And most likely this class, will be having more than one purpose, can be called low-cohesion.

Better OOP Approach

Well the better approach for the case above would be using the Factory Design Pattern.  We can let the factory of  READER to generate every single instance according to their type. It would be easier to grow the instance type, since we just need to create a new class and do a little modification in the Factory class. The caller class, wont grow and will stand still at its current shape.


public interface InfoReader {

	public void readInfo();

}

public class ExecutiveReader implements InfoReader {

	public void readInfo() {

		// override

	}

}

public class OperationalReader implements InfoReader {

	public void readInfo() {

		// override

	}

}

And The Factory

public enum ReaderType {
PROGRAM_CONTACT, FILE_CONTACT
}
public class InfoReaderFactory {

	public static InfoReader getInstance(ReaderType type) {

		InfoReader instance = null;

		switch (type) {

		case PROGRAM_CONTACT:

			instance = new ExecutiveReader();

			break;

		case FILE_CONTACT:

			instance = new OperationalReader();

			break;

		default:

			throw new IllegalArgumentException("Unknown Reseller");

		}

		return instance;

	}

}

And now The Caller


InfoReader reader = InfoReaderFactory.getInstance(resellerInfo.getType());

reader.readInfo();

The Benefits

With the Factory Design Pattern to handle this case, we can achieve some benefits,

  • Specifying a class for one task, means, easier to maintain since one class is for one purpose only (modularity/High Cohesion). i.e: Operational Reader is only to read data for Operational only, no other purpose. Just in case, one day in the future we need another Reader (say: NonOperationalReader). We just need create a new Class that extends (or implements) the InfoReader class and then we can override our own readInfo() function. This Caller class will have no impact. We just need to do some modification in the Factory code (and the Enum of course.
public enum ReaderType {
PROGRAM_CONTACT, FILE_CONTACT, NEW_READER
}

 

public class InfoReaderFactory {

	public static InfoReader getInstance(RaederType type) {

		InfoReader instance = null;

		switch (type) {

		case PROGRAM_CONTACT:

			instance = new ExecutiveReader();

			break;

		case FILE_CONTACT:

			instance = new OperationalReader();

			break;

		case NEW_READER:

			instance = new NonOperationalReader();

			break;

		default:

			throw new IllegalArgumentException("Unknown Reseller");

		}

		return instance;

	}

}

  • Higher Reusability of Parent’s Component (Inheritance): Since we have parent class (InfoReader), we can put common functions and thingies inside this InfoReader class, and later all of the derivative classes (ExecutiveReader and OperationalReader) can reuse the common components from InfoReader . Avoid code redundancy and can minimize coding time. Eventhough this one depends on how you do the code and cant be guaranteed :D

But, It’s Run Perfectly, Should We Change It?

Obviously the answer is big NO. This is only the case study and for your further experience and knowledge. OOP is good, do it anywhere it’s applicable. But the most important thing is, if it’s running, dont change it. It would be ridiculous if you ruin the entire working code just to pursue some OOP approach. Dont be naive also, no one can achieve the perfect code. The most important is we know what is the better approach.

This blog has some modifications and improvements caused by some comments in here. Personally, thanks for great input so i can keep the quality of this post.

About these ads
Categories: Java, Miscellaneous Tags: , , ,
  1. Nirav
    18 October 2012 at 00:08 | #1

    Good stuff. Thanks. Like the last like “The most important is we know what is the better approach”.

  2. 18 October 2012 at 00:36 | #2

    I don’t agree at all…

    Firstly the factory is not the the pattern you desire there, because what you did was just move the switch case to other side. You need a strategy there, why are you using the type as an int, it should be an object that knows how to handle a read operation. With the factory you would have to repeat the all the code if for example a write operation was added.

    Secondly. You should change it, we have to always improve your code, that’s the purpose of refactoring. But in order to keep the system working as expected it should be backed up with the unit test an other automatic integration tests.

    Regards

  3. RDeJourney
    18 October 2012 at 09:14 | #3

    Hi Juan,
    Actually in the real case, we use the instanceof to determine the type of parameter, i use int just to simplify the case. In hope i can reduce the confusing thing.

    In my opinion, We can put the WRITE operation in the parent class, and the children just need to override/modify the WRITE according to its need. But as you can see the class name is xxxReader. So it i put the write function, it would disobey the name itself.

    OR

    I will create another class that similar, maybe InfoWriter, ExecutiveWriter, etc. It would be better i think :)

    And for the second, sorry i dont get you, Do you mean we have to provide the unit test to complete it? If yes, sure, we still have to provide unit test, but i think that is out of context of this writing. But let me know, if you are thinking differently.

    This is a very good response, Many thanks for the input :)

    • 19 October 2012 at 21:32 | #4

      Hi !

      Defining a type as an int or String is a bad code smell. Types should be an object that has particular behaviour. For example

      In this case you can make the class ResellerInfoType with the method getReader that has the default behaviour, and there you make the subclasses ProgramContact, FileContact implementing the method getReader for the particular behaviour.

      This link covers exactly your case http://sourcemaking.com/refactoring/replace-type-code-with-state-strategy

      For the last part you are saying the code should not be changed, but in a system the code is continually improving, so you must change it. But prior that you have to create some tests that guarantee the system doesn’t break with the refactor.

      Keep it up!

      • RDeJourney
        19 October 2012 at 21:40 | #5

        Hi Juan,
        So glad to read your comment. In real practice, we indeed use the object as a type. In here, i use int to substitute and make it simpler. Im fully agree, using the int and String isnt good practice. We have same opinion in here.

        Yes, i said dont change the code that is working. Because, this system is running perfectly right now, i dont want to take any risk if something happened and the business goes down just because we want to pursue the best practice of the code. We cant take any risk to do that, in real world, we have to cover million partners around the globe. Agree, the code is bad.. Fully agree. But, business is the most important in here. :)

        Thanks for your comment, really, i learned a lot from you. Many thanks for the link that u gave to me.

  4. Ahmed
    19 October 2012 at 06:23 | #6

    in my opinion, it would be better to pass directly the parameter type instead of the object for the factory method so your Factory will not depend on ResellerInfo …public static InfoReader getInstance(int type) {…}, another improvement is to use Enums instead of static int for the type.

    • RDeJourney
      19 October 2012 at 13:43 | #7

      Oh you are truly right. That would be better to pass the Type. Many thanks for the input, this is so valuable for me.

  5. RDeJourney
    19 October 2012 at 21:53 | #8

    Hi Ahmed and Juan,

    I have modified my post. Many thanks for both of you for the great input !

  6. Frisian
    26 October 2012 at 13:02 | #9

    One more iteration would make it compile-time safe:
    - Add a Visitor to ReaderType (http://stackoverflow.com/questions/859563/java-enums-and-switch-statements-the-default-case/859751#859751)
    - Replace the switch/case statement inside getInstance() with an implementation of said Visitor interface
    - Make sure you annotate all the implementing methods with @Override

    Voilá, every time you change something in the ReaderType enum, the compiler will remind you, that you have to do something in the factory class as well.

  7. 2 August 2013 at 16:43 | #10

    Hi,
    I liked your approach towards learning the alternates, this will definitely help us grow in our coding skills. Ultimately we know which is best for the situation. Good work!

  1. 23 October 2012 at 12:40 | #1

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 724 other followers

%d bloggers like this: