2

I am currently trying to refactor some code and one of the problems I came across was the constructors had far too many parameters (15 in fact) and was being initialised by another object which had the same amount of properties.

I am meant to be reducing the amount of classes we have so creating a set of classes which represented the different parts of the parameters seemed silly so I began looking and the best answer I came across was from this question on stackoverflow.

public class DoSomeActionParameters
{
    readonly string _a;
    readonly int _b;

    public string A { get { return _a; } }
    public int B{ get { return _b; } }

    DoSomeActionParameters(Initializer data)
    {
        _a = data.A;
        _b = data.B;
    }

    public class Initializer
    {
        public Initializer()
        {
            A = "(unknown)";
            B = 88;
        }

        public string A { get; set; }
        public int B { get; set; }
    }

    public static DoSomeActionParameters Create(Action<Initializer> assign)
    {
        var i = new Initializer();
        assign(i)

        return new DoSomeActionParameters(i);
    }
}

which can be called like so using a lambda

DoSomeAction(
    DoSomeActionParameters.Create(
        i => {
            i.A = "Hello";
        })
    );

I really liked this method even though it doesn't solve directly the problem of actually initialising the class, it does make it mean that I can have one constructor instead of 3. Also it doesn't require my class to know about objects it doesn't care about.

My problem is however, I do not know what this pattern is called so I can't do any further research on it to find out if it is still valid, is superseded by another pattern or whether it has sever performance issues. From my research the closest pattern I could find is the builder pattern but it seems to be more of an extension on that.

As a bonus point, it would be great if you could give me a hand with how to make certain parameters mandatory as we have about 10 parameters from the database and 5 more which are just flags which we don't really need to know about.

Keithin8a
  • 161
  • 9
  • 3
    > "I am meant to be reducing the amount of classes" Why would you want to do that? – LorToso Jul 20 '15 at 12:24
  • @LorToso The code has became bloated over the years and people have been too afraid to touch it so have just created simple unnecessary variations which don't inherit from the original base class, but use the base methods to mutate itself into some sort of monster message to send to the database. – Keithin8a Jul 20 '15 at 12:32
  • see also: [When to go Fluent in C#?](http://programmers.stackexchange.com/q/69519/31260) and [How can you decompose a constructor?](http://programmers.stackexchange.com/q/231915/31260) – gnat Jul 20 '15 at 17:15

3 Answers3

13

I'd call this the "Elephant in the Room" (anti)pattern. You are focusing on the minutiae whilst ignoring the bigger problem. If you have a class that requires 15 constructor parameters, then this a warning sign that the class is doing too much and thus needs too much configuration.

The "pattern" you need here therefore is the Single Responsibility Principle. Examine the class, work out how many (likely quite distinct) things it's doing and start breaking those out into smaller, more focused, classes. Each will only then need a few constructor parameters.

To begin with, this will create more classes; but they'll be good classes. As you apply this to the mutated mess of similar classes, you'll be able to replace large parts of those with your new classes and what's left will again be better quality, more focused classes.

David Arno
  • 38,972
  • 9
  • 88
  • 121
  • It sort of is structured like this already though. We essentially have a log record which sits in the database. It contains header information such as length, type, origin etc... and has a body object which is a base class with implementations for all the different types of logs. I don't really see how I can break down the header information into smaller groups without it being too unweilding to use. I could maybe have pure header meta params (which is the log type and length), time params and flag params. That just seems unnecessary to me, am I just misunderstanding the situation? – Keithin8a Jul 20 '15 at 13:04
  • I think you are right about the elephant in the room pattern though. I have just attempted to implement it and from a maintenance point of view its horrendous. – Keithin8a Jul 20 '15 at 13:05
  • I am going to mark this as the correct answer because the fact that we had that many parameters is just because it represents a database row so in fact it was following the single responsibility principle. We have just condensed the constructors down to make it more maintainable in the future. – Keithin8a Jul 20 '15 at 13:44
3

Have a look at the Builder Pattern. It solves exactly the problem you are having - too many parameters in a constructor. You wind up with an empty constructor (typically in Java this will be a private method, if you use an inner class as the Builder) and setter methods (which may also remain private), then a Builder class with chained methods which call the setters, and a build() method which constructs the object and populates its fields.

Carl Manaster
  • 4,173
  • 18
  • 31
  • Interesting, I had looked at the Builder Pattern but thought that the chained methods would have made the code difficult to use for those not familiar with it. In reflection though, this is the same with any pattern or methodology. For example when I first saw turnary statements I thought that syntactically they were unreadable but now I use them quite often. The only probably that I can see with the builder pattern though is that a developer won't know which are mandatory fields – Keithin8a Jul 20 '15 at 14:43
  • You could have the build() method throw an exception if any mandatory fields are uninitialized. It's not very expressive, certainly not as clear as a proper constructor, but it's an option. – Carl Manaster Jul 20 '15 at 14:50
  • 1
    Blog post about mandatory methods in a builder... Refers to answers on SO... http://www.jayway.com/2012/02/07/builder-pattern-with-a-twist/ – Fuhrmanator Jul 21 '15 at 01:56
  • @Fuhrmanator all I can say is WOW. That pattern is pretty clever and does solve my problem that I have with builders. NOTE: in the comments section of the blog they have spotted a bug with it but give a solution so if you are implementing this in the future then be sure to look in the comments. – Keithin8a Jul 21 '15 at 10:25
1

I am creating my own answer (after a discussion on meta) because I believe that the correct answer is in fact a combination of everything so far.

Firstly as DavidArno put, the pattern that I have asked about is more of an anti-pattern because it is just hiding the problem somewhere else. However I believe the suggestion of using the builder pattern from the answer by Carl Manaster does solve the issue that I have because I cannot break down the list of parameters without over-engineering the Single Responsibility Principle.

The problem I had with the builder pattern was that there was no way to make it easy for a developer using it to know about the mandatory fields which will mean there will be lots of trial and error which will lead to copy and pasting the initialisation everywhere which will lead to more mistakes. Fuhrmanator however linked to the following blog which demonstrates how to have mandatory fields with the builder pattern (after looking elsewhere I believe this is called a Step Builder).

Essentially the principle of the Step Builder is to guide a developer through the initialisation of all the mandatory fields. Your builder implements various interfaces which act as steps to create the object. They each have a method which have a return type of the next step. This aids intellisense more than the standard builder patter as well because it cuts down the amount of methods you can call. See the following example from the blog.

public class Address {
private String protocol;
private String url;
private int port;
private String path;
private String description;

// only builder should be able to create an instance
private Address(Builder builder) {
    this.protocol = builder.protocol;
    this.url = builder.url;
    this.port = builder.port;
    this.path = builder.path;
    this.description = builder.description;
}

public static Url builder() {
    return new Builder();
}

public static class Builder implements Url, Port, Build{
    private String protocol;
    private String url;
    private int port;
    private String path;
    private String description;

    /** Mandatory, must be followed by {@link Port#port(int)}  */
    public Port url(String url) {
        this.url = url;
        return this;
    }

    /** Mandatory, must be followed by methods in {@link Build}  */
    public Build port(int port) {
        this.port = port;
        return this;
    }

    /** Non-mandatory, must be followed by methods in {@link Build}  */
    public Build protocol(String protocol) {
        this.protocol = protocol;
        return this;
    }

    /** Non-mandatory, must be followed by methods in {@link Build}  */
    public Build path(String path) {
        this.path = path;
        return this;
    }

    /** Non-mandatory, must be followed by methods in {@link Build}  */
    public Build description(String description) {
        this.description = description;
        return this;
    }

    /** Creates an instance of {@link Address} */
    public Address build() {
        return new Address(this);
    }
}

interface Url {
    public Port url(String url);
}

interface Port {
    public Build port(int port);
}

interface Build {
    public Build protocol(String protocol);
    public Build path(String path);
    public Build description(String description);
    public Address build();
}

You have the collection of Mandatory parameters which each return the next statement but also what I think is great about this is that you can have optional parameters as well by making the methods return type the build interface.

This solves my problem because I had mandatory parameters mixed in with optional ones which made expanding it difficult as there was lots of copy and pasting and potential problems. With the step builder I can control how a developer creates my object so that I have facts to use in the future, and I allow them the flexibility of creating the object in as many ways as they possibly need to.

Also it makes it very readable!

Keithin8a
  • 161
  • 9
  • I know the person who marked this as -1 will never see this but if you could please explain why you marked it down? I asked the community on Programmers Meta what I should do and this is what they advised. – Keithin8a Jul 22 '15 at 07:32
  • I didn't vote down but I abstained of upvoting either, because I would prefer this answer to have a [brief overview or summary or excerpt](http://meta.programmers.stackexchange.com/questions/7515/what-to-do-when-the-best-answer-doesnt-solve-the-question#comment22554_7515 "'make sure that your answer... can stand on its own...'") from a linked blog article – gnat Jul 22 '15 at 10:53
  • @gnat Thank you for the feedback. I never thought about adding something from the blog which is a really strange thing for me not to do. When I saw your comment I just had assumed you meant that I should reference who said what and what they said rather than actual content from the blog. I will update my answer now, I appreciate you helping me improve my answer. – Keithin8a Jul 22 '15 at 10:57
  • No idea why it was downvoted. I think it a good summary of your situation and possible solutions, so it gets a +1 from me. – David Arno Jul 22 '15 at 13:44
  • 1
    @DavidArno I think it was because your post was a really good answer and the basis for my discovery of the solution because it made it clear what my problem actually was and allowed me to step back and explore the different options. Also the blog post was quite long so someone who just wants to skim the answer wouldn't get all of the info that they needed to make the assessment of whether it is a valid answer. – Keithin8a Jul 22 '15 at 14:03