Is passing too many arguments to the constructor considered an anti-pattern?

Question

I am considering using the factory_boy library for API testing. An example from the documentation is:

class UserFactory(factory.Factory):
    class Meta:
        model = base.User

    first_name = "John"
    last_name = "Doe"

For this to work, we need first_name, last_name, etc to be passed as parameters to the __init__() method of the base.User() class. However, if you have many parameters this leads to something like:

class User(object):

    GENDER_MALE = 'mr'
    GENDER_FEMALE = 'ms'

    def __init__(self, title=None, first_name=None, last_name=None, is_guest=None,
             company_name=None, mobile=None, landline=None, email=None, password=None,
             fax=None, wants_sms_notification=None, wants_email_notification=None,
             wants_newsletter=None, street_address=None):

        self. title = title
        self.first_name = first_name
        self.last_name = last_name
        self.company_name = company_name
        self.mobile = mobile
        self.landline = landline
        self.email = email
        self.password = password
        self.fax = fax
        self.is_guest = is_guest
        self.wants_sms_notification = wants_sms_notification
        self.wants_email_notification = wants_email_notification
        self.wants_newsletter = wants_newsletter
        self.company_name = company_name
        self.street_address = street_address

Now the question is, is this construction considered anti-pattern, and if yes, what alternatives do I have?

Thanks

Eventually

You don't need an __init__ () method on your class to use factory_boy unless it's changed since v2.4.1. – Protuberancy

This isn't an answer, but your issue could be a sign that the User class is trying to do too many things - ideally, you wouldn't want to have to modify the User class if you add, say, notifications over carrier pidgeon. It could also be a sign that its data model can be simplified - you could have a list of phone/fax numbers instead of mobile, landline and fax fields, make guest users a subclass instead of a field, and so on. – Jelks

Maybe this is pedantic, but in python the phrase positional argument is defined as not being a keyword argument. Aside from the obligatory self you have no positional arguments in this __init__ method. No one needs to worry about if mobile goes in position 10 or 14 since it is specified by keyword. – Grotto

You could also use def __init__(self, **kwargs): self.name=kwargs.get('name', None) – Protuberancy

If you igonore for a moment the desirability or not of having all this contact fields in the table, with possible maintenance hassles, what is wrong with the code? These are optional fields so your call will populate as few as them as necessary (which Essence is expressly set to disallow). The intent'is clear. A Pattern would likely make code less clear and still not help with table maintenance. A kwargs approach is nice but hides your field names (what happens if you pass in firstname). – Lida

I think the question here is "what is too many" is having 20 arguments in constructor an anit-pattern? Most probably yes. Is having 7 arguments anti-pattern? In some cases, not really. For the example given I'd pass more objects into the constructor: at least ContactInformation() and SubscritionPreferences(). Basically group some arguments into logical objects. – Dior

Answer 1

You could pack the __init__ method's keyword arguments into one dict, and set them dynamically with setattr:

class User(object):
    GENDER_MALE = 'mr'
    GENDER_FEMALE = 'ms'
    def __init__(self, **kwargs):
        valid_keys = ["title", "first_name", "last_name", "is_guest", "company_name", "mobile", "landline", "email", "password", "fax", "wants_sms_notification", "wants_email_notification", "wants_newsletter","street_address"]
        for key in valid_keys:
            setattr(self, key, kwargs.get(key))

x = User(first_name="Kevin", password="hunter2")
print(x.first_name, x.password, x.mobile)

However, this has the drawback of disallowing you from supplying arguments without naming them - x = User("Mr", "Kevin") works with your original code, but not with this code.

Electromyography

I prefer this solution, but you loose the code completion. Is there a way not to? – Magically

+1 but for completeness, you should throw TypeError if keyword argument was passed and it's not in valid_keys list. – Loftin

altering the __dict__ directly in order to prevent the number of parameters in a signature seems like throwing the baby out with the bathwater. Wouldn't this lead to more buggy code vs the latter which hasn't even been established as a valid anti-pattern? – Softener

@Softener I'm not sure why I originally recommended accessing __dict__ directly. setattr accomplishes the same thing without resorting to double-underscores. But your objection still applies. Even setattr is a little too "spooky" for my tastes, when it comes to production-quality code... I'd much rather redesign the class to use a reasonable number of attributes, or dispense with classes altogether and use a regular dict. – Electromyography

This is bad form a user point of view because code completion no longer can help the user. – Spiegeleisen

I think this is just a bad hack. You need to build a logic which comes for free with using named/required arguments. Autodocs won't understand this - so building a references or documentation will not work. Autocomplete in IDEs won't work either. My take is: introducing a huge list of valid_keys and building an extra logic is worse anti-pattern than having too many arguments. – Dior

Answer 2

In Python 3.7, dataclasses (specified in PEP557) were added. This allows you to only write these arguments once and not again in the constructor, since the constructor is made for you:

from dataclasses import dataclass

@dataclass
class User:
    title: str = None
    first_name: str = None
    last_name: str = None
    company_name: str = None
    mobile: str = None
    landline: str = None
    email: str = None
    password: str = None
    fax: str = None
    is_guest: bool = True
    wants_sms_notification: bool = False
    wants_email_notification: bool = False
    wants_newsletter: bool = False
    street_address: str = None

It also adds a __repr__ to the class as well as some others. Note that explicitly inheriting from object is no longer needed in Python 3, since all classes are new-style classes by default.

There are a few drawbacks, though. It is slightly slower on class definition (since these methods need to be generated). You need to either set a default value or add a type annotation, otherwise you get a name error. If you want to use a mutable object, like a list, as a default argument, you need to use dataclass.field(default_factory=list) (normally it is discouraged to write e.g. def f(x=[]), but here it actually raises an exception).

This is useful where you have to have all those arguments in the constructor, because they all belong to the same object and cannot be extracted to sub-objects, for example.

Csch

Answer 3

Yes, too many arguments is an antipattern (as stated in Clean Code by RObert C. Martin)

To avoid this, you have two design approaches:

The essence pattern

The fluent interface/builder pattern

These are both similar in intent, in that we slowly build up an intermediate object, and then create our target object in a single step.

I'd recommend the builder pattern, it makes the code easy to read.

Faddist

So is an "essence" the same thing as a "parameter object" for a constructor? – Canella

Second link is pretty useless since it includes a lot of jumping through Java-specific hoops. There's a good code example in Wikipedia en.wikipedia.org/wiki/Builder_pattern – Hera

Would you mind detailing here why it's an anti-pattern? I thank you for the link but I couldn't find the bit that refers to it being an anti-pattern and why. In a similar thread most link answers are helpful with the link but also the excerpt in the case the link dies. – Softener

Answer 4

The biggest risk would be if you had a large number of positional arguments and then ended up not knowing which is which.. Keyword arguments definitely make this better.

As suggested by others, the builder pattern also works quite nicely. If you have a very large number of fields, you can also do something more generic, like so:

class Builder(object):

    def __init__(self, cls):
        self.attrs = {}
        self.cls = cls

    def __getattr__(self, name):
        if name[0:3] == 'set':
            def setter(x):
                field_name = name[3].lower() + name[4:]
                self.attrs[field_name] = x
                return self
            return setter
        else:
            return super(UserBuilder, self).__getattribute__(name)

    def build(self):
        return self.cls(**self.attrs)

class User(object):

    def __str__(self):
        return "%s %s" % (self.firstName, self.lastName)

    def __init__(self, **kwargs):
        # TODO: validate fields
        for key in kwargs:
            setattr(self, key, kwargs[key])

    @classmethod
    def builder(cls):
        return Builder(cls)

print (User.builder()
  .setFirstName('John')
  .setLastName('Doe')
  .build()) # prints John Doe

Nitrochloroform

Answer 5

If overloading were not a problem then every class in python could be reduced to a single method, which we could call doIt (....). As with everything, it's best to do things in moderation. Overloading any method with umpteen arguments is bad practice. Instead, allow the user to build up the object in bite-size chunks of related data. It's more logical. In your case, you could split the calls into names, communications, and perhaps other.

Spiegeleisen