4 Replies - 809 Views - Last Post: 21 May 2013 - 03:05 AM

#1 toad87  Icon User is offline

  • D.I.C Head

Reputation: 8
  • View blog
  • Posts: 188
  • Joined: 21-May 12

Factory Method Question

Posted 20 May 2013 - 02:45 PM

Hello guys,

I have a question about the Factory Method pattern.

I have this project where sport "Matches" are created. Subclasses of Matches are created such as "HockeyMatch" or "TennisMatch".

I have a simple Factory class right now that creates the appropriate match by taking in a string ("tennis", "hockey, etc). The information comes from reading through a CSV file. Each line starts with the type of sport and then info on
the match.

Now I've read that using simple Factory's violate the Open close principle and should be replaced by a Factory Method or Abstract Method pattern.

But I was thinking, if I were to implement the Factory Method pattern, I would have to subclass my MatchFactory for each concrete sport match. Which means I would still have to decide which concrete subclass of MatchFactory to use during run-time.


When I use my simple factory, the factory takes in a string and creates the appropriate match using that string. Now I have to write if/else code to decide which concrete MatchFactory to use.

For example, my client code would look something like this:


MatchFactory factory = null;
Product aProduct = null;

if(<string>.Equals("tennis")
{
     factory = new TennisMatchFactory();
     aProduct = factory.createMatch("tennis");
}
else if(<string>.Equals("hockey")
{
     factory = new HockeyMatchFactory();
     aProduct = factory.createMatch("hockey");
}




It seems to make the code more complicated. Something about this seems wrong to me...Can someone clarify if I'm using it wrong?

This post has been edited by toad87: 20 May 2013 - 02:54 PM


Is This A Good Question/Topic? 0
  • +

Replies To: Factory Method Question

#2 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1949
  • View blog
  • Posts: 4,048
  • Joined: 11-December 07

Re: Factory Method Question

Posted 20 May 2013 - 04:33 PM

No matter what you do, you need the logic to convert a String to a Match. When in doubt go for the simplest option. That would be a switch or bunch of if ... elses calling the constructors. You could put that in a method and call it a factory.

If you plan to add a lot of kinds of matche then separate factories might be the best way. The if structure is clearly poor though. One solution is to give each factory a boolean accept method. It takes a String and returns true if the string is correct for that kind of factory. Now you can loop over all the factories and use the first one that accepts the string to create your Match object.
Was This Post Helpful? 3
  • +
  • -

#3 toad87  Icon User is offline

  • D.I.C Head

Reputation: 8
  • View blog
  • Posts: 188
  • Joined: 21-May 12

Re: Factory Method Question

Posted 20 May 2013 - 09:03 PM

View Postcfoley, on 20 May 2013 - 04:33 PM, said:

No matter what you do, you need the logic to convert a String to a Match. When in doubt go for the simplest option. That would be a switch or bunch of if ... elses calling the constructors. You could put that in a method and call it a factory.

If you plan to add a lot of kinds of matche then separate factories might be the best way. The if structure is clearly poor though. One solution is to give each factory a boolean accept method. It takes a String and returns true if the string is correct for that kind of factory. Now you can loop over all the factories and use the first one that accepts the string to create your Match object.



Thanks Cfoley! I'll try out the boolean accept method for my factories!
Was This Post Helpful? 0
  • +
  • -

#4 toad87  Icon User is offline

  • D.I.C Head

Reputation: 8
  • View blog
  • Posts: 188
  • Joined: 21-May 12

Re: Factory Method Question

Posted 20 May 2013 - 10:14 PM

Quick question.

To loop over all the factories, I would have to instantiate an object of each factory and add it to a collection right?

This is what I did and it worked great!!


 HockeyMatchFactory hockeyMatchFactory = new HockeyMatchFactory();
TennisMatchFactory tennisMatchFactory = new TennisMatchFactory();

List<MatchFactory> matchFactories = new List<MatchFactory>(); 

matchFactories.Add(hockeyMatchFactory);
matchFactories.Add(tennisMatchFactory);


while (resultsReader.Peek() > 0)
{
Match tempMatch = null;
string lineInfo = resultsReader.ReadLine();
foreach (MatchFactory factory in matchFactories)
{
if(factory.AcceptMatch(lineInfo))
{
         tempMatch = factory.CreateMatch(lineInfo);
}

}
sportsData.Matches.Add(tempMatch);
}







EDIT:

Actually cfoley, I'm wondering whether looping through a list of factories has the disadvantage of using more memory. Since I have to instantiate all my derived factories, they lie in permanent memory right? But if I were to use a polymorphic MatchFactory variable and assign it a concrete factory using if/else statements at runtime, only one factory would exist at a time right?

This post has been edited by toad87: 20 May 2013 - 10:31 PM

Was This Post Helpful? 0
  • +
  • -

#5 cfoley  Icon User is offline

  • Cabbage
  • member icon

Reputation: 1949
  • View blog
  • Posts: 4,048
  • Joined: 11-December 07

Re: Factory Method Question

Posted 21 May 2013 - 03:05 AM

Yes, but then you lose the advantages of the above approach. It's extremely easy to add new factories and you can even see how it would be possible to provide a library where other programmers can use Matches and MatchFactories that you had never thought of.

There is a more important lesson here, and it is that high level languages are a means for you to communicate the solution to a problem. It's as much about communicating with fellow programmers as it is about telling a computer what to do. If you start out compromising your design by thinking about bits, byes and clock cycles, you will end up with software that is hard to understand, extend and maintain. Ironically, it will also be hard to optimise. Performance is important but it can almost always be achieved through the correct choice of data structure and algorithm. These are things that encourage a good design.

Here is my take on what your factory could look like. Bear in mind that with only two kinds of match, you could dispense with all this and write a 3 line if statement that calls constructors directly.

package matchFactory;

import java.io.*;
import java.util.*;

public class SomeProgram {

	private Collection<Match> matches;

	// We won't worry about exactly what kind of factory just now. The user will
	// probably pass an AnyMatchFactory but there is no reason to force the
	// issue.
	private MatchFactory factory;

	public SomeProgram(MatchFactory factory) {
		matches = new ArrayList<>();
		this.factory = factory;
	}

	// This method is pure convenience
	public void readMatches(String filename) throws FileNotFoundException {
		readMatches(new File(filename));
	}

	// Convenience too but most likely the one you want to call.
	public void readMatches(File file) throws FileNotFoundException {
		try (Scanner s = new Scanner(file);)/> {
			readMatches(s);
		}
	}

	// The only essential of the three overloads. Having it separate from the
	// File one is great for testing. You can make a Scanner that reads from a
	// test String and pass it to this method.
	public void readMatches(Scanner in) {
		while (in.hasNextLine()) {
			parseMatchInfo(in.nextLine());
		}
	}

	public void parseMatchInfo(String matchInfo) {
		if (factory.accept(matchInfo)) {
			addNewMatch(matchInfo);
		} else {
			handleInvalidMatchInfo(matchInfo);
		}
	}

	public void addNewMatch(String matchInfo) {
		Match m = factory.make(matchInfo);
		matches.add(m);
	}

	public void handleInvalidMatchInfo(String matchInfo) {
		// What do you want to do here? It might be reasonable to ignore blank
		// lines and log the rest to report to the user later.
	}
}



package matchFactory;

import java.util.*;

public class AnyMatchFactory implements MatchFactory {

	private Collection<MatchFactory> factories;

	public AnyMatchFactory() {
		factories = new ArrayList<>();
	}

	public void addFactory(MatchFactory f) {
		factories.add(f);
	}

	// You might think that calls to this method are wasteful. You are certainly
	// correct but the user won't notice the difference and you are making up
	// for it with flexibility elsewhere.
	@Override
	public boolean accept(String matchInfo) {
		for (MatchFactory f : factories) {
			if (f.accept(matchInfo)) {
				return true;
			}
		}
		return false;
	}

	@Override
	public Match make(String matchInfo) throws MatchFactoryException {
		MatchFactory f = getFactory(matchInfo);
		return f.make(matchInfo);
	}

	public MatchFactory getFactory(String matchInfo) throws MatchFactoryException {
		for (MatchFactory f : factories) {
			if (f.accept(matchInfo)) {
				return f;
			}
		}
		throw new MatchFactoryException(matchInfo);
	}

}

Was This Post Helpful? 0
  • +
  • -

Page 1 of 1