Is it a code smell for a factory to have many methods?

Question

I have a testing project, in Katalon Studio, that uses data classes, called models. This is a conscious design decision.

Also a conscious design decision, is the decision to separate the concern of creating models from the concern of the test(s) that uses them. For this, for the most part, I use model factories that have methods to return the model specific to that test.

I'm doing what I outline here

For example, I have a method on my MemberLeadFactory for each scenario with the contact phone number field .

As a matter of fact, that factory in particular has 30 methods in it. I wish to create another one, that simply takes a list of family members, and creates a list of family member leads from them .

Here is the full factory:

public class MemberLeadFactory extends BaseModelFactory<MemberLeadModel> {


    @Override
    public MemberLeadModel createSanityCase() {
        String firstName = SMDStringUtils.CreateFirstName(),
        lastName = SMDStringUtils.CreateLastName();

        return new MemberLeadModel(firstName,
                lastName,
                new ContactInfoFactory().createRandomForPerson(firstName, lastName),
                MemberCategory.MEMBERSHIP,
                SMDDateUtils.DOBForStandardMember(),
                SMDDateUtils.WithinLastMonth(),
                Frequency.ANNUAL,
                MemberContractSignatureMode.ByPost,
                PaymentMode.CHECK,
                );
    }

    public MemberLeadModel createDummyModel() {
        return this.createSanityCase();
    }
    
    public MemberLeadModel createWithStartDate(Date startDate) {
        MemberLeadModel model = this.createDummyModel();

        model.setContractStartDate(startDate);

        return model;
    }

    public MemberLeadModel createBackdatedModel(String firstName, String lastName) {
        return this.createBackdatedModel(firstName, lastName, MemberLeadCallbackFactory.OnRandomBackdate())
    }
    
    public MemberLeadModel createBackdatedModel(String firstName, String lastName, Closure<MemberLeadModel> onBackdateModel) {
        MemberLeadModel model = this.createDummyModel();
        
        model.setFirstName(firstName);
        model.setLastName(lastName);
        model.getContactInfo().setEmail(SMDEmailUtils.CreateEmailFor(firstName, lastName));
        
        return onBackdateModel(model);
    }

    public MemberLeadModel createMainFamilyMemberLead() {
        return this.createMainFamilyMemberLead("Main", "Member", null);
    }
    
    public MemberLeadModel createMainFamilyMemberLead(Closure<MemberLeadModel> onBackdateModel) {
        return this.createMainFamilyMemberLead("Main", "Member", onBackdateModel);
    }

    public MemberLeadModel createMainFamilyMemberLead(String firstName, String lastName, Closure<MemberLeadModel> onBackdateModel) {
        MemberLeadModel model = this.createWithName(firstName, lastName, onBackdateModel);

        model.setMemberLeadType(MemberLeadType.FAMILY);
        model.setMemberType(MemberType.INDIVIDUAL);

        return model;
    }
    
    public MemberLeadModel createWithName(String firstName, String lastName, Closure<MemberLeadModel> onBackdateModel) { 
        if (onBackdateModel != null)
            return this.createBackdatedModel(firstName, lastName, onBackdateModel);
        
        MemberLeadModel model = this.createDummyModel();
        
        model.setFirstName(firstName);
        model.setLastName(lastName);
            
        return model;
    }
    
    public MemberLeadModel createFamilyMemberLead(String firstName, String lastName, boolean isPediatric, BaseMemberModel mainMemberModel) {
        return this.createFamilyMemberLead(firstName, lastName, isPediatric, mainMemberModel, null) 
    }
    
    public MemberLeadModel createFamilyMemberLead(String firstName, String lastName, boolean isPediatric, BaseMemberModel mainMemberModel, Closure<MemberLeadModel> onBackdateModel) {
        MemberLeadModel model = this.createWithName(firstName, lastName, onBackdateModel);
        
        model.setMemberLeadType(MemberLeadType.FAMILY);
        model.setMemberType(MemberType.FAMILY);
        model.setRelatedFamily(SMDStringUtils.GetFullName(mainMemberModel));
        
        if (isPediatric) {
            model.setCategory(MemberCategory.PEDIATRIC);
            model.setDob(SMDDateUtils.DOBForPediatric());
        }
        
        return model;
    }

    public MemberLeadModel createFrequencyBased(Frequency frequency, String firstName, String lastName) {
        MemberLeadModel model = this.createBackdatedModel(firstName, lastName);

        model.getContactInfo().setMobilePhoneNumber(SMDNumberUtils.TenDigitPhoneNumber());
        model.setFrequency(frequency);

        return model;
    }

    public MemberLeadModel createWithAlphabeticDOB() {
        return this.createBackdatedModel("Alphabetic", "DateOfBirth");
    }

    public MemberLeadModel createWithImproperDOBFormat() {
        return this.createBackdatedModel("Improper", "DateOfBirth_Format");
    }

    public MemberLeadModel createWithUpcomingBirthday() {
        MemberLeadModel model = this.createBackdatedModel("Upcoming", "Birthday");

        model.setDob(SMDDateUtils.DOBForUpcomingBirthday());

        return model;
    }

    public MemberLeadModel createWithFutureBirthdate() {
        MemberLeadModel model = this.createBackdatedModel("Future", "Birthdate");

        model.setDob(SMDDateUtils.GetFutureDOB());

        return model;
    }

    public MemberLeadModel createWithTooShortPhoneNumber() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("PhoneNumber");
        model.setLastName("TooShort");
        model.getContactInfo().setPhoneNumber("77777");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWith8DigitPhoneNumber() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("PhoneNumber");
        model.setLastName("8Digits");
        model.getContactInfo().setPhoneNumber("77777777");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithAlphabeticPhoneNumber() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("PhoneNumber");
        model.setLastName("Alphabetic");
        model.getContactInfo().setPhoneNumber("PPPPLZFAIL");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithPhoneNumberWithSlashes() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("PhoneNumber");
        model.setLastName("ContainsSlashes");
        model.getContactInfo().setPhoneNumber("317/555.5555");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithoutDomainExtension() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("NoEmail");
        model.setLastName("DomainExtension");
        model.getContactInfo().setEmail("dev+FirstLast@example");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithNullCharacterEmail() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("EmailContains");
        model.setLastName("NullCharacters");
        model.getContactInfo().setEmail("[email protected]\0example.com");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithWhitespaceCharacterEmail() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("EmailContains");
        model.setLastName("Whitespace");
        model.getContactInfo().setEmail("[email protected] example.com");

        model.setDob(SMDDateUtils.DOB());
        model.setLeadDate(new Date());
        model.setPaymentMode(PaymentMode.ACH);

        return model;
    }

    public MemberLeadModel createWithNonStandardAddress() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("AddressContains");
        model.setLastName("Semi-colons");

        model.getContactInfo().setAddress("1222;East;53rd;Street");

        return model;
    }

    public MemberLeadModel createWithPurelyNumericAddress() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("AddressIs");
        model.setLastName("PurelyNumeric");

        model.getContactInfo().setAddress("1234567890");

        return model;
    }

    public MemberLeadModel createWithNumbersInState() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("StateContains");
        model.setLastName("Numbers");

        model.getContactInfo().setState("I2");

        return model;
    }

    public MemberLeadModel createWithFakeState() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("Fake");
        model.setLastName("State");

        model.getContactInfo().setState("JK");

        return model;
    }

    public MemberLeadModel createWithLettersInZipCode() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("ZipCode");
        model.setLastName("ContainsLetters");

        model.getContactInfo().setZip("A622O");

        return model;
    }

    public MemberLeadModel createWithImproperZipCode() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("ZipCode");
        model.setLastName("ImproperFormat");

        model.getContactInfo().setZip("46220.2635");

        return model;
    }

    public MemberLeadModel createWithNonexistentOrgName() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("Nonexistent");
        model.setLastName("OrganizationName");

        return model;
    }

    public MemberLeadModel createWithNonexistentMemberCategory() {
        MemberLeadModel model = this.createDummyModel();

        model.setFirstName("Nonexistent");
        model.setLastName("MemberCategory");

        return model;
    }
}

Would this design decision necessarily be a code smell?

Answer 1

With method names like "createWithImproperZipCode" and "createWithPurelyNumericAddress", you have made something pretty specific. Usually logic like this gets used in multiple places, which makes creating a tool to initialize test data worthwhile.

Providing that these methods are called in more than one test class (note I said "test class" not just test) then this is a useful abstraction. If you find these methods are only called in a single test class, I would recommend moving that logic into the test class as a private method. No need to make programmers jump to another file just to see how the object is initialized.

Worse yet, if the method is only called by a single test, just put the logic in the test method. It is necessary setup to understand the test. Don't even make the programmer jump to another method — until the test method is so long it is difficult to reason about.

This is all a balance between code reuse, readability, and writing a comprehensive test that stands on its own merits without requiring people to jump all over the codebase to see how things are set up.

This is not immediately a code smell, but it would prompt me to find where these methods are called before adding something more to this class. I would expect these methods to be called all over the place, and if not I would consider it over-engineered for the purpose it serves.

Remember you can always copy, paste, and refactor later to centralize logic. Don't immediately jump to creating test abstractions "just in case." YAGNI is a good guideline here.

Answer 2

Very few things are necessarily a code smell - the question you should be asking is "what value does this pattern bring to my code?" Does it improve the readability or maintainability of your code?

If it does improve your code, go for it. If it doesn't and you're doing it because you made an arbitrary decision to separate model creation and testing, then you probably want to rethink that decision.

Answer 3

I think you are on the right path, but you chose the hard way.

What you have implemented is known as factory methods. Factory methods don't need an abstraction (class). They are good enough as static methods. Take a look closer to MemberLeadFactory: it has no state, it has no instance dependencies, and dependencies are hardcoded, resolved statically or by arguments at each method.

If you are coding in Java, you are probably familiar with static factory methods since JUnit and Hamcrest implementations ¹ are built around factory methods. Simply, none call classes factory.

In addition to the previous, I want to give a counterargument for the following quote from Greg Burghardt's answer.

Worse yet, if the method is only called by a single test, just put the logic in the test method.

While I understand the rationale behind the quote, I couldn't disagree more. The experience says that even if these methods are called only from one method or class, the effort of keeping factory methods in a single and reusable source of trust is negligible. But, the risk of developers copy-pasting the "private method" in several tests is high². Later, the effort of getting rid of the dup code (tech debt) will be higher than coding factory methods in a single source of trust right from the beginning.

---

^{1: The facto standard of testing in Java}

^{2: I have seen this almost in every project I have worked on. Devs (you can't imagine how many) are careless about test code. As if test code didn't matter.}

Answer 4

that factory in particular has 30 methods in it.

How many factory method calls are needed to get an instantiated object? If it is more than one then perhaps there is an encapsulation problem.

After the factory returns the object, if that object or client code relies on the factory - calls factory's method(s) - for any state or function on behalf of the object there is probably a serious Single Responsibility Principle (SRP) problem.

The factory pattern provides for a single "create" method. If those 30 methods are "create" for different classes all in one factory, that may be just fine. I can see this if it is about subclasses. If it goes beyond subclasses and involves object composition then perhaps you should look at the Abstract Factory

---

---

Update: OP added complete MemberLeadFactory class and has selected the preferred answer. ...

Yes, it is a code smell

But which one? Possibly "combinatorial explosion" and maybe others.

So the different create methods are about test data not different object types. So WHY 30 methods (and counting) is clear. Every data-variation means a new method that must be fully self-sufficient with often suspiciously redundant pattern feel to the code.

Given the entire code base this obviously does not scale - the time and effort spent to write, debug, and maintain tests is out of control. You must abstract the test data injection; do this and you potentially reduce this to one method for all class-data-variant tests.

Extract all the test data into a data structure - a class. Each "record" in this structure represents a single test case. Yes different properties per test but the property definitions and names match the target class. With a more generic type like Object the record can be just the key:value properties needed. Then the populating code can be made generic via reflection or association array or hash type notation, depending on your language and test frame.

Answer 5

"I use model factories that have methods to return the model specific to that test."

I'm presuming you have methods like

CustomerFactory.SignedUpCustomerWithThreeOrders()
CustomerFactory.NewCustomerThatHasntVerified()

or perhaps

CustomerFactory.AddCompletedOrder(order)
CustomerFactory.WithVerifiedEmailAddres(email)

etc

I think this is just a different use of the term Factory. You don't really have a classic "factory class" here, what you have is static test data.

Having structures like this is a recognised test pattern, although personally I would prefer a more rigorous adherence to Arrange, Act, Assert and have the data setup in the test itself for most purposes.

The danger in the pattern is that you create a link between tests, sharing test setup or test data between individual tests making them connected and easy to break.

If you find these factories are becoming more complex than your tests, then I would say yes, its becoming a code smell and you should look at alternatives. Perhaps simply deserialising the objects from json files?