13

This question already has an answer here:

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?

marked as duplicate by gnat, Greg Burghardt, Philipp, 8bittree, Robert Harvey Jul 21 '17 at 21:47

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
  • 3
    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
  • 10
    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
  • 3
    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
18

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.

  • 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 Piliser 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 Piliser 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
  • 7
    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 7:56
30

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.

  • 6
    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 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 at 7:16
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.

  • 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
  • 3
    @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
2

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
1

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.

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