Refactoring constructor that has too many parameters

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?

see also: [How does one keep argument counts low and still keep third party dependencies separate?](https://softwareengineering.stackexchange.com/q/271548/31260) — gnat, Jul 19 '17 at 11:03
What is the *point of these parameters*? Perhaps you're looking for an array `string[8]`. — amon, Jul 19 '17 at 11:10
Very good question, shows good thought about design too. I'd say this still adheres to SRP. The style is possibly a bit off. If there's some value in treating these strings individually then there's probably not a lot you should feel obliged to do. However, I'd strongly consider using a collection/array as @amon suggested, if not. After that, it's pretty much as clean as it gets. — JᴀʏMᴇᴇ, Jul 19 '17 at 14:22
As an aside, if this was on code review, I'd probably be of the opinion you could further neaten it up with all of your string declarations on one line. But it's not critical obviously. — JᴀʏMᴇᴇ, Jul 19 '17 at 14:23
you should put the true names of your variables, it would be easier to tell you if some can be regrouped or if all are required. — Walfrat, Jul 19 '17 at 16:57
I'm assuming you obtain the data from some sort of device driver. How does it expose the data? As a byte array, stream, XML...? — John Wu, Jul 19 '17 at 18:10
The answer really depends on what you're modeling. Please give a real example per @Walfrat 's comment — Fuhrmanator, Jul 20 '17 at 18:14
This question is impossible to answer when the parameters have meaningless names like a, b, c etc. Without understanding the purpose of the parameters, how would you know if they pertain to the same responsibility? — JacquesB, Jul 20 '17 at 20:39

Graham · Answer 1 · 2019-09-11T13:09:28.650

42

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.

edited Sep 11 '19 at 13:09

answered Jul 20 '17 at 18:22

Graham

3,833
1
21
18

7

This is the correct answer. The number of arguments is not in itself a problem, but they are often a symptom of other problems. Just putting the same set of arguments into an an array or builder pattern solves absolutely nothing. The real problem here is having arguments named a, b, c, d, etc. – JacquesB Jul 20 '17 at 20:26
I agreed. As I commented to Robert Harvey, passing an array should work and is going to be absolutely more simpler than all my builder, factories and classes. But the question is not if having 8 parameters (btw all strings) is ok or not. The OP is asking how can he do It simpler and extensible. And, IMHO, arrays as data structures are neither simpler nor extensibles. They are quite rigid. – Laiv Jul 21 '17 at 19:16
There were some studies by Microsoft regarding API usability, and the conclusion was that large numbers of parameters to any call did have an effect. I don't have a reference. – Frank Hileman Jul 24 '17 at 22:01
1

@FrankHileman Of course fewer parameters is easier than more. But that is because less information is processed. If you need more information to construct the object, then artificially reducing the number of parameters isn't going to help. – gnasher729 Sep 10 '19 at 18:37
There is a precedent for this in the .NET Framework itself: `public Guid(int a, short b, short c, byte d, byte e, byte f, byte g, byte h, byte i, byte j, byte k)`. Of course, the other constructors are used more often, but if all parameters are required and they're all part of the class itself, it should be a valid option. – Vivelin Sep 11 '19 at 07:16
@gnasher729 I think the idea was that if you can reduce the number of parameters by simplifying the design, that is better. And simpler designs are harder to create and take more time. Probably the best example of this type of simplification would be mathematical libraries, where sub objects tend to hold several bits of data, and those are passed to higher level objects, etc. I agree that if you need the data, you need the data. The question is, do you really need the data? Or can it be chunked in a more friendly way? – Frank Hileman Sep 11 '19 at 16:14
2

I work in a project where constructors have 39 to 60 parameters because of answers like yours. It is not fine to let problems scale this much, later the problem becomes almost unsolvable. Developers should strive to write clean code, "code that reads like well written prose". Using creational patterns and reducing the size of the problem the class/method solves is significant for the long run. Simple classes and small methods are easier to read, easier to keep, quicker to debug. – Lucas Montenegro Carvalhaes Sep 01 '20 at 21:18
@LucasMontenegroCarvalhaes I agree that 60 parameters is a lot, however, does that class actually have 60 data inputs? If so, then the problem is probably not the constructor, its the class. What problem did you actually face because of the number of parameters, and would you have faced that same problem if those 60 items were injected through DI somehow? – Graham Jan 05 '21 at 16:32
Sure, the problem is the class! Still, that class is a factory so using the DI the entire class is actually going to be deleted haha. This is why DI and injection would actually solve this issue. The parameter count exists there exactly because it is missing a dependency management system. – Lucas Montenegro Carvalhaes Jan 11 '21 at 16:29

Laiv · Accepted Answer · 2019-09-12T10:00:01.383

25

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.

edited Sep 12 '19 at 10:00

answered Jul 19 '17 at 18:41

Laiv

14,283
1
31
69

1

Be careful with C# optional parameters, at least if you're going to be using multiple assemblies. The value of the parameter is compiled into the call site, so replacing a library DLL will not update default parameters until the callers are recompiled. – Andrew Jul 19 '17 at 18:59
Would you drop them from a data model based on inheritance? – Laiv Jul 19 '17 at 19:00
I would use constructor overloading and chaining to achieve the same effect if I was writing library code. I'm not entirely sure what you mean by "data model based on inheritance", but if I was writing a subclass to a class using default parameters, I would do my best not to expose them. – Andrew Jul 19 '17 at 19:03
Ok, that's what I was asking. I will remove the mention to these parameters. – Laiv Jul 19 '17 at 19:04
9

Builders never fix the problem of class having too many parameters. They merely hide the problem behind a more beautiful facade. It's a dirty fix. – Andy Jul 20 '17 at 07:56
But still better than direct initialization all across the project or fat factories. Plus you can pass them as a parameter for late initializations – Laiv Jul 20 '17 at 07:57
You've clearly put a lot of thought into this, but for this particular problem wouldn't a simple string params array suffice? – Robert Harvey Jul 20 '17 at 16:58
Could work, indeed. But I find arrays as data structures to be a brittle solution. If the structure changes, we have to re-type the mapping. We also have to ensure that the array has at least the required min lentgth (in order to avoid IndexOutOfBound errors). Finally the order of the elements into the array matters. We have to ensure the right order too. IMO, too many things that could change or go wrong. I could consider using Maps (dictionaries). They are more or less the same solution, but a little bit more flexible. – Laiv Jul 20 '17 at 17:11
Note that the only purpose of the examples is to illustrate the SoC or the SRP. Reading the question I came to the conclusion that I had to link both subjects (refactor and SPR). On the other hand, I wonder if the array /map approach could be considered somehow *encapsulation*. Anyways, you are right, as a rule of thumb, we should aim for the simplest solution. – Laiv Jul 20 '17 at 17:23
an "RFID Message" doesn't have any "subject", this is why I asked for the parameters. It will usually contains an EPC whih determine the source of the message and the content. This is basically messages exchanged betweens two software/hardware components. – Walfrat Jul 20 '17 at 18:23
I will stay tunned. If @MHartley edit the question with real attribute names, I could make my answer more acuerated. As I commented, the whole purpose of the examples is illustration. – Laiv Jul 20 '17 at 19:30
This is quite a complex solution to a simple problem. – Frank Hileman Jul 24 '17 at 22:03
@FrankHileman agree. Took me to write my own question to understand why. The code was supposed to be an example of the points I enumerate, not the solution itself. Edited and removed. – Laiv Jul 24 '17 at 22:34

John Wu · Answer 3 · 2017-07-20T16:03:04.547

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.

This seems problematic. `System.IO.Stream` implements `IDisposable` and if it follows the CLR patterns, `RFIDStreamReader` will implement `IDisposable` as well. I feel like `RFIDStreamReader` should be the type of the parameter passed to `FromStream` and that it should be created by the caller in a `using` statement. — Aluan Haddad, Jul 19 '17 at 19:19
Seems Factory pattern to me :-/. The static modifier is what confuse me but still. — Laiv, Jul 19 '17 at 19:48
@Laiv a factory if often `static` in C#. In this case, there is no instance to call it on so either it needs to be `static` or it needs to be a member of another class which there is an instance of. — Aluan Haddad, Jul 19 '17 at 20:39

score 2 · Answer 4 · answered Jul 20 '17 at 07:57

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.

I believe this is useful in unit testing code suite, but hard to find place in production code unless your object needs to be costructed differently each time. — kuskmen, Jul 20 '17 at 17:08
It should be a RFIDMessageBulilder not RFIDMessage and it would be perfeclty fine: `var message = new RFIDMessageBuilder().SetA("Some A").SetC("Some C").Build();` — Mariusz Jamro, Jan 03 '20 at 11:31

score 1 · Answer 5 · answered Jul 19 '17 at 16:16

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.