7
\$\begingroup\$

In your opinion what is the best way to construct an object given the two examples below:

Option 1: Lots of parameters

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

public Person(string firstname, string surname, Address homeAddress, 
              PhoneNumber homeNumber)
{
    _firstname = firstname;
    _surname = surname;
    _homeAddress = homeAddress;
    _homeNumber = homeNumber;
}

public string getFirstname()
{
    return firstname;
}

public string getSurname()
{
    return surname;
}

public Address getAddress()
{
    return address;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}

Option 2: no parameters and use of set methods

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

public Person()
{
}

public string getFirstname()
{
    return firstname;
}
public void setFirstname(String value)
{
    firstname = value;
}

public string getSurname()
{
    return surname;
}
public void setSurname(String value)
{
    surname = value;
}

public Address getAddress()
{
    return address;
}
public void setAddress(Address value)
{
    address = value;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}
public void setHomeNumber(PhoneNumber value)
{
    _homeNumber = value;
}

Which method do you prefer? In my mind, option 1 is better because you know that the state of your object cannot be changed. I believe this is called immutability and is something that should be desired? On the other hand, it can quickly get out of hand and ugly if you have more than a handful of parameters.

I am aware that some languages such as C# have nice features that would probably make option 1 more appealing such as named and optional parameters however this question was prompted while dealing with Java code.

Update: Thank you for all of your suggestions. I will comment on each answer individually but a few answers suggested using a factory method instead of having the constructor do all of the work. This was actually the pattern I was using because it is a standard at work but I've never really seen the advantage of it as opposed to using the constructor to do the work. This is more how the code looked:

private string firstname;
private string surname;
private Address homeAddress;
private PhoneNumber homeNumber;

private Person()
{
}

public Person factoryCreate(string firstname, string surname, Address homeAddress, 
                            PhoneNumber homeNumber)
{
    Person person = new Person();
    person.setFirstname(firstname);
    person.setSurname(surname);
    person.setHomeAddress(homeAddress);
    person.setHomeNumber(homeNumber);
    return person;
}

public string getFirstname()
{
    return firstname;
}
public void setFirstname(String value)
{
    firstname = value;
}

public string getSurname()
{
    return surname;
}
public void setSurname(String value)
{
    surname = value;
}

public Address getAddress()
{
    return address;
}
public void setAddress(Address value)
{
    address = value;
}

public PhoneNumber getHomeNumber()
{
    return _homeNumber;
}
public void setHomeNumber(PhoneNumber value)
{
    _homeNumber = value;
}

From the discussion about having constructors do little work and using builders, I can see why having the factory method is a good one. My understanding of factory methods was that they were used to create a class given a few options. For example, if you have an Employee class and that is sub-classed by FullTimeEmployee and PartTimeEmployee, you would have a factory() method on Employee to deal with creating the more concrete class.

\$\endgroup\$
  • \$\begingroup\$ Good question! An answer to the downside of option 1 is to require programmers to properly comment their long constructor calls (one comment per parameter). Something that can be enforced only by peer review... I believe modern trend is to use builders instead. \$\endgroup\$ – PhiLho Jul 16 '11 at 9:32
  • \$\begingroup\$ As stated in Clean Code, a comment is a failure to express by the programming language. Luckily for C#programmers there are named parameters that it is the language way to do this. Aditional benefit is that the IDE can detect the change during refactors and prevents bugs if the signature changes \$\endgroup\$ – user119591 Mar 30 '16 at 14:31
3
\$\begingroup\$

When building objects, having a constructor that does very little is ideal. This becomes especially true once you are testing your objects with unit tests.

Like other suggestions, I would leave the proper construction of the object to a builder or factory method. I would emphasize that you should only include parameters that are required for it to be a proper object, and that generally you want no more than 7 (give or take 2) parameters. If you have more parameters than that, your class may be trying to do too much.

However, things like an ID that relates to a database field (or other storage mechanism) are better left to be set by the Data Mapper, Repository, etc. that you are using.

In my native programming language, it would look something like this:

class Person
{
    private $_id; // optional (not necessary for a Person... set once stored)
    private $_name; // required
    private $_address; // optional

    /**
     * @param $name - In this example, I'm just going to pretend like this is required.
     *                It probably doesn't make sense for there to be an instance of a 
     *                Person without a name.
     * @return Person
     */
    public static function factory( $name )
    {
        $Person = new Person();
        $Person->setName($name);

        return $Person;
    }

    public function setId($id)
    {
        $this->_id = $id;
    }

    public function getId()
    {
        return $id;
    }

    public function setName($name)
    {
        $this->_name = $name;
    }

    public function getName()
    {
        return $this->_name;
    }

    public function setAddress($address)
    {
        $this->_address = $address;
    }

    public function getAddress()
    {
        return $this->_address;
    }
}

When creating a Person:

$PersonRepository = new PersonRepository();
$Person = Person::factory('Sarah');
$PersonRepository->save($Person);   // This generates an ID for $Person before saving

When retrieving a Person:

$PersonRepository = new PersonRepository();
$Person = $PersonRepository->getById($id); 

and the Repository in this case would be responsible for doing something like...

$Person = new Person();
$Person->setId($storageResult['id']);
$Person->setName($storageResult['name']);
$Person->setAddress($storageResult['address']);
return $Person;
\$\endgroup\$
  • \$\begingroup\$ I have updated my original question now to use a factory method and it does look very similar to your answer now. I am marking this as the answer because I like the idea of devolving the creation of an object to a factory method rather than using another class to build a person. \$\endgroup\$ – Stuart Leyland-Cole Jul 24 '11 at 10:28
10
\$\begingroup\$

It depends on whether the data elements are required for the object to be well defined or if they are completely optional.

My personal preference is for the constructor to have parameters for all the required data elements and use set methods for everything else. I also declare the default (parameterless) constructor to be private. That way the compiler guarantees that no one will create an incompletely defined object.

If firstname and lastname are required but address and phone number are optional, I would do the following:

private string _firstname;
private string _surname;
private Address _homeAddress;
private PhoneNumber _homeNumber;

private Person()
{
    // Ensure unitialized Person cannot be created
}

public Person(string firstname, string surname)
{
    _firstname = firstname;
    _surname = surname;
}

// getters and setters omitted for brevity

The disadvantage of using the Builder pattern as recommended in other answers is that the completeness check is deferred to runtime. If you have a code path that is very rarely executed and you change Person so that more data is required, then you could deliver code that will fail in the field. With my approach, you will know about the problem the next time you try to compile your code.

\$\endgroup\$
  • \$\begingroup\$ Thank you for pointing out the problem with the builder pattern. It also seems a bit over-architected to me but maybe that is because of the simplicity of the example here.I have updated the original question with code that utilises a factory method to shift the work away from the constructor. \$\endgroup\$ – Stuart Leyland-Cole Jul 24 '11 at 10:14
2
\$\begingroup\$

I also recommend the Builder pattern, however I prefer to do it this way:

public class Person {
    private int id;
    private String firstname;
    private String surname;
    private Address homeAddress;
    // etc.

    private Person() {// IMPORTANT, private constructor
    }

    public static class Builder {
        private Person person;

        public Builder(int id) {
            if (id == 0) {
                //fuck it
            }
            person = new Person();
        }

        public Builder setFirstname(String firstName) {
            person.firstname = firstName;
            return this;
        }

        public Builder setSurname(String surname) {
            person.surname = surname;
            return this;
        }
        // etc.

        public Person build() {
            // don't return the object if it was not created correctly... for instance:
            if (person.firstname == null) {
                throw new IllegalStateException("You are a fool");
            }
            return person;
        }
    }

    public static void main(String... args) {
        Person person = new Person.Builder(666)
                .setFirstname("foo")
                .setSurname("bar")
                .build();
    }
}

Main difference with regards to the previous response is that:

  • You can't create a Person unless you use a Builder
  • The builder knows when a person is ready, otherwise it won't return anything.
\$\endgroup\$
  • \$\begingroup\$ As far as the builder pattern goes, I prefer this one as it is more complete. However, how do you deal with @jimreed's suggestion that the builder pattern can cause exceptions in production if there is a little used code path that doesn't fully create a person? Is it simply having confidence in your unit tests? \$\endgroup\$ – Stuart Leyland-Cole Jul 24 '11 at 10:16
  • \$\begingroup\$ This code does not even compile. How has it been upvoted? You can't have a non-static constructor nor methods in a static class. \$\endgroup\$ – ryanulit Sep 29 '16 at 20:45
2
\$\begingroup\$

The answer isn't necessarily language agnostic, because it would be silly not to use the facilities of your language to address this issue...
So your question is valid in Java and other intermediate language, but is more pointless in some higher level languages.

I am thinking of Scala, where you have indeed named and default parameters, allowing to have constructors with lot of parameters while keeping readability: you name the parameters on the call site, which similar to the chaining of sets, but safer as mandatory parameters (without default) must be present.

On the other hand, you can also use type tricks (called phantom types, if I understood correctly the articles I have read, but not totally assimilated...) to force a builder to get all the required elements. When I write "force", I mean "enforced by the compiler", ie. your code must be correct at compile time, it isn't just checked at run time. Powerful.

I won't address your specific question (solution working for all languages), the answers I see are much better than the one I could make, but I thought I should mention interesting alternatives to your question.

\$\endgroup\$
  • \$\begingroup\$ Thank you for pointing out that this isn't really a language agnostic question. I did intend it to be that way but as I was composing it, I was beginning to realise that it wasn't at all. Your answer did make me think a bit more carefully about different ways to accomplish this in C# (which is my preferred language) and named and default parameters are a much bigger help than I gave credit for in my original question. \$\endgroup\$ – Stuart Leyland-Cole Jul 24 '11 at 10:20
0
\$\begingroup\$

Starting out with immutable classes is typically a good idea. (according to Effective Java). I like to do things that way when I can.

You should also check out the builder pattern.

As it encapsulates your initialization and allows you to add more parameters without affecting the constructors signature.

EDIT: Here is an example based on your code, It might be a cross between C# and java but I think you will get the idea...

public class Person {
    private string firstname;
    private string surname;
    private Address homeAddress;
    private PhoneNumber homeNumber;

    public Person( Builder builder ) {
        super();
        this.firstname = builider.firstname;
        this.surname = builder.surname;
        this.homeAddress = builder.homeAddress;
        this.homeNumber = builder.homeNumber;
    }

    pubilc static class Builder {
        private integer id;//required field.
        private string firstname;//optional field.
        private string surname;//optional field.
        private Address homeAddress;// optional field.
        private PhoneNumber homeNumber;// optional field.

        public Builder( integer id ) {
            super();
            if ( id == null ) {
                //deal with it.
            }
            this.id = id;
        }
        public Builder setFirstname( string firstName ) {
            this.firstname = firstname;
            return this;
        }
        public Builder setSurname ( string surname ) {
            this.surname = surname;
            return this;
        }
        public Builder setHomeAddress( Address homeAddress ) {
            this.homeAddress = homeAddress;
            return this;
        }
        public Builder setHomeNumber( PhoneNumber homeNumber ) {
            this.homeNumber = homeNumber;
            return this;
        }
    }
    // Person.getXXX() goes here.

    public static void main( String...args ) {

        Person.Builder builder = new Person.Builder( new integer( 123 ) );
        builder.setFirstname( "foo" ).setSurname( "bar" ).setHomeAddress( new HomeAddress() ).setHomeNumber( new PhoneNumber() );

        // always the same
        Person person = new Person( builder );
    }
}
\$\endgroup\$
  • \$\begingroup\$ I'm not so sure that I can see the benefits of a builder class. I suppose you are making the Person class immutable which is good, especially if it's a database class but then you have another class which is immutable. It seems over-architected to me but do you think that is mostly because of the fairly simplistic example? Also, what about the problem of a little used code path that doesn't set all of the necessary fields? How do you spot that? Unit tests? \$\endgroup\$ – Stuart Leyland-Cole Jul 24 '11 at 10:24
  • \$\begingroup\$ YMMV. I tend to go immutable by default since I can always expose properties and functionality as I needed. When I do expose it I know for what specific reasons I expose it (comment that) and I have intimate knowledge of that change.It's the builder's responsibility to ensure all required fields are populated. That's why the builder's constructor takes the required fields and the optional fields are setter injected. The builder can contain more involved validation logic if needed. \$\endgroup\$ – HeathLilley Jul 24 '11 at 13:19
0
\$\begingroup\$
  • Making a constructor do as less work as possible is good. To me, simply assigning parameters to local fields is not work. That to me, is zero work - No logic is involved.
  • Depending on your programming language, marking the fields as final is good, if you are not modifying them. My big reason for this is for readability - its immediately clear that the final fields are the easy bits - can't be modified and less hanging threads to keep in my thread as I read the rest of the code. This doesn't seem like a big deal for your current class, but I am guessing that the your code snippet is part of a much bigger class, and even if it wasn't, its pretty much something I always do.
  • Exposing the setters just to use them by the builder and not anywhere else introduces more complexity(see point 2).
  • Using the builder brings more verbosity. I wouldn't use it just for optional fields - constructor overloading/passing in null is much cleaner for this. In general, I use the builder pattern when the construction process is more laborious then just setting some fields.
  • Too many paramters is a code smell but 4 parameters seems okay. Especially, if they actually seem like they belong to this class, and they have different types amongst them(not all are strings for e.g.).

My conclusion: I would just mark the fields final and stick with your option 1. :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.