Correct Implementation of a Factory Method

Question

I am trying to understand the theory of what a factory pattern is, am I implementing it correctly here? if not what is wrong if correct what should I change?

Interface

public interface Person {
    public void setName(String name);
    public String getName();
    public void setAge(String age);
    public String getAge();
    public void setGender(String gender);
    public String getGender();
}

Male Object

public class Male implements Person
{
    public String name;
    public String age;
    public String gender;
    @Override
    public void setName(String name) {
        this.name = name;
    }

    @Override
    public String getName() {
        return null;
    }

    @Override
    public void setAge(String age) {
        this.age = age;
    }

    @Override
    public String getAge() {
        return null;
    }

    @Override
    public void setGender(String gender) {
        this.gender = gender;
    }

    @Override
    public String getGender() {
        return null;
    }
}

Female Object

public class Male implements Person
{
    public String name;
    public String age;
    public String gender;
    @Override
    public void setName(String name) {
        this.name = name;
    }

    @Override
    public String getName() {
        return null;
    }

    @Override
    public void setAge(String age) {
        this.age = age;
    }

    @Override
    public String getAge() {
        return null;
    }

    @Override
    public void setGender(String gender) {
        this.gender = gender;
    }

    @Override
    public String getGender() {
        return null;
    }
}

Object Factory

public class PersonFactory {

    public Person getPerson(String type) {
        if(type == "MALE") {
            return new Male();
        }
        else {
            return new Female();
        }
    }
    public Person getMale() {
        return new Male();
    }
    public Person getFemale() {
        return new Female();
    }
}

Main Method

public class main {
    public static void main(String[] args) {
        PersonFactory pf = new PersonFactory();
        Person adam = pf.getPerson("MALE");
    }
}

This: `if(type == "MALE") {` is wrong. You should compare String with `equals`. — Tunaki, Sep 14 '15 at 11:28

score 3 · Answer 1 · edited May 23 '17 at 11:51

3

Apart from the String comparison in PersonFactory and with proper implementation of getters in your factory members, seems a descent implementation.

And in your getPerson() logic can be modified as

 if (type == "MALE") {
        return getMale();
    } else {
        return getFemale();
 }

edited May 23 '17 at 11:51

Community

1
1

answered Sep 14 '15 at 11:34

Suresh Atta

120,458
37
198
307

I'm not sure about the methods for getting the Male and Female, for me it's a duplicate logic, why should you define them when you have already a factory method `getPerson()`, which role is to generate a specific type of person? – drgPP Sep 14 '15 at 11:35
1

@drgPP Perfect catch. Edited my post according to it. – Suresh Atta Sep 14 '15 at 11:37
:-P I put my answer while you were editing your code – Craig Sep 14 '15 at 11:40
1

It shouldn't even be called 'get...' in a Factory. – kevintjuh93 Sep 14 '15 at 11:40
@KevinKal what would the correct suffix be instead of get? – Matthew du Plessis Sep 14 '15 at 11:42
@KevinKal That's debatable point and matter of taste. [Some use](http://docs.jboss.org/hibernate/core/3.6/javadocs/org/hibernate/SessionFactory.html#getCurrentSession()) some and some [may not](https://en.wikipedia.org/wiki/Factory_method_pattern) – Suresh Atta Sep 14 '15 at 11:44
1

@Craig Oh my twin brother :D – Suresh Atta Sep 14 '15 at 11:44
1

@MatthewduPlessis I would use something like 'create'. Get doesn't make it clear enough that a new object is initialized. EDIT: Also, they should just return Male and Female. – kevintjuh93 Sep 14 '15 at 11:45
@KevinKal OP did a cleaner way, in case if anything needs to be add to the person before returning it :) – Suresh Atta Sep 14 '15 at 11:48
@sᴜʀᴇsʜᴀᴛᴛᴀ You can still do that if you use what I do. Not sure we understand each other haha – kevintjuh93 Sep 14 '15 at 11:51
@KevinKal oops you missed my point. ;) Hence agreeing with you to put it to an end :P – Suresh Atta Sep 14 '15 at 11:52

score 1 · Answer 2 · answered Sep 14 '15 at 11:39

Looks pretty good, but I'd go ahead and use the factory methods for your "String" method. In other words:

public Person getPerson(String type) {
    if(type == "MALE") {
        return getMale()
    }
    else {
        return getFemale();
    }
}

That way you only have to update the methods if you want to change things in one place, for example if you decide to do something special in the getFemale() or getMale()s.

score 1 · Answer 3 · answered Sep 14 '15 at 13:46

1

You should use "MALE".equals(type) instead of type == "MALE". Otherwise your are doing good with factory pattern.

answered Sep 14 '15 at 13:46

Kuldeep Singh

199
1
11

score 0 · Accepted Answer · 2015-09-15T14:49:11.587

I would implement another approach. The first thing, I would make gender as an enum:

public enum Gender {
    Male, Female
}

Note, that Male and Female classes share the same data and semantics to work with this data. I mean name, age and gender.

So, I would implement an abstract class that handles this data. Please note, that class fields should be private, not public.

public abstract class AbstractPerson implements Person {
    private String name;
    private String age; // maybe int ?
    private Gender gender;

    public  AbstractPerson() {

    }

    @override
    public void setName(String name) {
        this.name = name;
    }

    @override       
    public String getName() {
        return name;
    }

    @override       
    public void setAge(String age) {
        this.age = age;
    }

    @override
    public String getAge() {
        return age;
    }

    @override
    public Gender getGender() {
        return this.gender;
    }

    @override
    public void setGender(Gender gender) {
        this.gender = gender;
    }
}

And it is better to hide this implementation. So we hide it into the factory class:

public class PersonFactory {

    private static PersonFactory instance = new PersonFactory();

    public static PersonFactory getInstance() {
        return instance;
    }

    public Person getPerson(Gender gender) {
        return new AbstractPerson(gender);
    }

    public Person getMale() {
        return getPerson(Gender.Male);
    }

    public Person getFemale() {
        return getPerson(Gender.Female);
    }

    private PersonFactory {

    }

    private static class AbstractPerson implements Person {
        private String name;
        private String age; // maybe int ??
        private Gender gender;

        public  AbstractPerson(Gender gender) {
            this.gender = gender;
        }

        @override
        public void setName(String name) {
            this.name = name;
        }

        @override
        public String getName() {
            return name;
        }

        @override
        public void setAge(String age) {
            this.age = age;
        }

        @override
        public String getAge() {
            return age;
        }

        @override
        public void setGender(Gender gender) {
            this.gender = gender;
        }

        @override
        public Gender getGender() {
            return this.gender;
        }
    }   
}

And, use case for this factory:

PersonFactory factory = PersonFactory.getInstance();
Person malePerson = factory.getMale();
Person femalePerson = factory.getFemale();

UPDATE: 15 Sept, 2015

Please note, if you have many kinds of objects, to be produced by factory, then it is advised to have a general method like getPerson(Gender gender) I've just added above in the code.

to downvoters, please leave a comment to let me know, what is wrong with my answer. Thank you. — , Sep 15 '15 at 03:07
The purpose of a factory is defeated by adding separate methods for each product implementation. In this case, the user must implement his own factory logic to choose between `getMale()` and `getFemale()`. — jaco0646, Sep 15 '15 at 14:41
@jaco0646 yes, you are right for the general case, when we have many different kind of objects to be produced by the factory. This particular task was related only to male/female case, so I designed the factory with those methods. I've added update to my post. — , Sep 15 '15 at 14:42

Correct Implementation of a Factory Method

4 Answers4