HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Is this a correct way to use Factory Pattern?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisfactorywaycorrectusepattern

Problem

I have an abstract class which implements the IDataCreator interface:

public interface IDataCreator
{
    public String getDataString();
}

public abstract class AbstractCreator implements IDataCreator
{
    protected IProject p;

    public AbstractCreator(IProject p) {
       this.p = p;
    }

    public abstract String getDataString();
}


Then I have 5 concrete classes which extend AbstractCreator and implement the getDataString() method:

public class ProjectManagerCreator extends AbstractCreator
{
    public ProjectManagerCreator(IProject p) {
       super(p);
    }

    @Override
    public String getDataString() {
       if(p.getLead() != null) {
           String lead = p.getLead().getName();
           return lead;
       }
       return "";
    }    
}


Factory class is implemented like this:

public class ProjectDataCreatorFactory
{
    IProject p = null;

    public ProjectDataCreatorFactory(IProject p) {
       this.p = p;
    }

    /**
     * Factory method which constructs a object from a given string value.
     * 
     * @param column string of the column name TODO: Implement a interface which maps the strings into static variables
     * @return object which implements the {@link IDataCreator} interface
     */
    public IDataCreator createFromString(String column) {
        IDataCreator creator = null;
        if(column.equals("name")) {
           return new URLCreator(p);
        } else if(column.equals("status")) {
           return new ProjectStatusCreator(p);
        } else if(column.equals("manager")) {
           return new ProjectManagerCreator(p);
        } else if(column.equals("setupdate")) {
           return new ProjectDateCreator(p, ProjectDateCreator.CREATION);
        } else if(column.equals("updatedate")) {
           return new ProjectDateCreator(p, ProjectDateCreator.UPDATED);
        }
        return creator;     
    }        
}


The factory class is called like this:

```
ProjectDataCreatorFa

Solution

Some design considerations:

-
Returning Null: Your factory method createFromString returns a null creator if the column is unknown. It's good practice to note this in the javadoc or perhaps throw an IllegalArgumentException if a column you can't handle is passed in.

-
Date Abstraction: ProjectDateCreatorlooks like it could use either its own factory or two separate implementations based on the second constructor argument. That class isn't included here, but it appears CREATION and UPDATED are some sort of public static final type constants. Those would be better expressed as an enum:

// A factory can use this instead!
public enum ProjectDateOption {
    CREATION, UPDATED;
}


-
Interface Naming: Interfaces in Java aren't typically prefixed with an I, but if it's consistent through your code then that's a minor quibble.

-
Column Strings: Your TODO mentions this, but the column strings could also be placed into an enum, and then the checking could occur there.

Code Snippets

// A factory can use this instead!
public enum ProjectDateOption {
    CREATION, UPDATED;
}

Context

StackExchange Code Review Q#24031, answer score: 3

Revisions (0)

No revisions yet.