Refactoring constructor that has too many parameters [duplicate]

Question

I'm in my first two months as a software engineer and just wanted to get advice on if this can be improved upon.

I've created a class that represents data from RFID in the form of a message:

 class RFIDMessage
 {

        string _a;
        string _b;
        string _c;
        string _d;
        string _e;
        string _f;
        string _g;
        string _h;

        public RFIDMessage(string a, string b, string c, string d, string e, string f, string g, string h)
        {
            _a = a;
            _b = b;
            _c = c;
            _d = d;
            _e = e;
            _f = f;
            _g = g;
            _h = h;
        }
}

I've been reading about the Single Responsibility Principle and can't work out if this constructor can be improved upon by providing it a new data type. This message is parsed from the data read from an RFID tag and having eight parameters seems a little too many. The parameters won't change in this instance but there are other message types - this is just the default one.

My questions are:

1) Is there a way to reduce the number of parameters that makes the code more maintainable?

2) How can I change or improve the design of this class to accommodate more types of messages?

Answer 1

You are right, 8 parameters -all strings- makes the constructor a good candidate for a code review.

Consider some of the next points.

Essential attributes first

Look the message model and figure out which attributes are necessary to initialize an instance in a consistent state. Reduce the number of arguments to the essential. Add setters or functions for the rest.

If all 8 attributes are required and read-only, there's not too much to do.

Encapsulation

Consider encapsulating correlated parameters. For example, A, B and C might be placed together into a new class. Find out which parameters change together, at the same time, due to the same reasons.

It reduces the number of arguments at the cost of one more class (complexity).

Use creational patterns

Instead of initializing messages directly from any place in the code source, do It from factories or builders.

Arrays

If none of the above works, try an array of parameters. Given the lack of meaningful param's names, it's probably the simplest solution.

Regarding arrays, I posted a question where I ask about its suitability. I was reluctant to trust in such a solution. However, the answers helped me out to be less reticent, so check it out if you also dislike this solution. It might change your mind about this.

Inheritance

Eventually, you will realise that messages are good candidates for inheritance. Segmenting messages by attribute generates a little overhead because sooner than later you ends up asking for the type if(message.getType() == ...) all over the code.

Answer 2

Here's a heretical answer: I see nothing inherently wrong with have lots of parameters.

Having a function that takes lots of parameters is often a target for refactoring, but just because we've refactored something into a cooler chunk of code doesn't mean we should have done so.

Using an array or array args instead of 8 separate params means that you'll need extra logic to enforce that there's the proper # of params passed in. Using a builder pattern is all well and good, except that if I come behind you to maintain the code and you have a builder pattern for every single stupid little data structure then I'm likely to get lost in your bajillion lines of code. If you really need all 8 arguments, then you can't move some to just setter props, because you could have an instance of the class without all the necessary data in it.

I know for my own experience that if I'm inheriting a codebase, I would much rather the code be set up in somewhat of a boring, tedious, obvious style (like your 8 constructor args, etc etc) instead of with some cool/complex new DI framework/pattern I've never used before.

If your class above is a simple (hopefully immutable) data structure that truly needs exactly 8 string arguments to be valid, then I say that having 8 parameters is a perfectly fine way of accomplishing that.

Answer 3

I'll work with the assumption that you want this class to be immutable.

If you want "true" immutability, you will need to stick with the constructor arguments, since the constructor is the only piece of code that can set a readonly field.

However, you can also just implement public getters and private setters. For all intents and purposes that makes the object immutable too. You'd set the private fields in a static factory(ish) method.

In this example I assume the RFID data arrives at your application in a stream, but you could easily modify this to work with a byte array, series of structs, XML, whatever.

class RFIDMessage
{
    private string _a;
    private string _b;
    private string _c;
    //etc

    private RFIDMessage() 
    {
        //Declare default constructor private so nobody else can instantiate an RFIDMessage on their own
    }

    static public RFIDMessage FromStream(Stream stream)
    {
        var reader = new RFIDStreamReader(stream);
        var newObject = new RFIDMessage();
        newObject._a = reader.ReadA();
        newObject._b = reader.ReadB();
       //etc.

       return newObject;
    }
}

void SampleUsage()
{
    var rfidStream = RFIDAPI.GetStream(); //Or whatever
    RFIDMessage message = RFIDMessage.FromStream(rfidStream);
}

I don't know the name of this pattern, but it is common in the .NET CLR, e.g. FromBase64String.

Answer 4

Though I think that your code could be OK depending on the actual situation, I'd like to show another possible solution: "fluent syntax". The idea behind fluent syntax is to provide methods for changing the state of the object which return the changed object. E.g.

public RFIDMessage SetA(string a)
{
    this._a = a;
    return this;
}

It allows for code like

RFIDMessage message = new RFIDMessage().SetA("Some A").SetC("Some C").SetH("Some H");

Note that now the instance can be changed after creation, which might be desired or a problem depending on your requirements. If it is a problem, you may create a "read-only interface" which the class implements (and those SetA methods mentioned above are not part of the interface) such that consumers of the instance knowing that read-only interface only can't change it.

Answer 5

Let us assume you need all these bits of data in one instance, and avoid questioning the design of the class. Perhaps the class could have less members, and the overall amount of code could be smaller, if you created another lower layer class collecting a subset of these members. That is not the question.

If you read the .net framework design guidelines regarding constructors, you can see that smaller numbers of parameters are preferred, both because default member values may reduce code, and because it is difficult to pass many parameters without mixing up the order. The alternative is to use properties to set up the instance before use. You set the properties that need non-default values, and leave the others as is. This can make it easier to use.

Since readonly fields should be preferred over mutable fields, when mutable fields are not needed, there is a trade off. You can only initialize readonly fields via constructors. You have not used any readonly fields, but you may wish to reconsider that decision. It is a weakness of the environment that forces you to make the decision.