Performing serial communication in constructor to initialize an object

Question

I have a class UnitInfo which represents a collection of unit information with methods to get the unit information in a structured way, such as a specific encoding, etc. This unit info consists of information which must be read from the unit via serial port communication. The constructor of the class is given the serial number, which cannot be queried from the unit, and an instance serial communication interface, which it then uses to query the unit for the information:

public class UnitInfo {
    public int SerialNumber { get; }
    public int InternalMemorySize { get; }
    // And more properties...

    public UnitInfo(ISerialCom serialCom, int serialNumber) {
        SerialNumber = serialNumber;
        InternalMemorySize = serialCom.GetInternalMemorySize();
        // More serial communication to set more properties
    }

    // Get methods to retrieve all the unit information in a specific encoding
    // i.e a collection of bytes.
}

From my browsing SO and Google, I understand that it is usually okay to do work in a constructor, but I've yet to find an example of performing communcation via serial, GPIB, etc. in a constructor. I'm also thinking this may be a violation of the Dependency Injection principle as the class is dependent upon some implementation of an interface to initialize itself; is it better to pass all of the unit information to the class?

Answer 1

This design is perfectly fine. Though you are communicating with external ports, it is hardware presumably plugged directly in to the computer, so communication happens quickly.

Since you are working in C#, exceptions in the constructor as a result of the user pulling the plug (literally) aren't a concern, because the Common Language Runtime is responsible for cleanup, although I would make sure the ISerialCom object implements IDisposable if you need to manage the connection (but that's not the responsibility of this constructor).

The nice thing about accepting an object that provides this vital information is it makes constructing this object pretty bullet-proof. You can't accidentally pass the internal memory size in place of the serial number.

Answer 2

Doing work in the constructor

This is one of those things which is ultimately a matter of opinion. While your currently accepted answer is one opinion, it's not one that I would share, and I'm not alone with that:

Work in the constructor such as: creating/initializing collaborators, communicating with other services, and logic to set up its own state removes seams needed for testing, forcing subclasses/mocks to inherit unwanted behavior. Too much work in the constructor prevents instantiation or altering collaborators in the test.

(Emphasis mine)

However, I would not concur 100% with Misko Hevery either (different opinions...), who sees "Anything more than field assignment" as a warning sign. "Short and sweet" validation like null checks or asserting an int to be in a certain range are acceptable.

My favorite metaphor for this: A dentist's job is not to construct a dentist's office.

Practical Advice

It should not be too difficult to create a UnitInfoFactory which is responsible for creating those UnitInfos. If UnitInfo is very small, a static Factory method is fine, too.

You would instantly be able to create a UnitInfo with just two ints instead of having to get an implementation of ISerialCom somehow (disregarding the code you left out in the example of course). This is gold if you're doing unit tests, because even with a mocking framework it is still additional work to create that mock. However, even if you're not unit testing, code written to be testable tends to be cleaner than other code.

No Violation of DI principles

However, this part here is not a matter of opinion:

I'm also thinking this may be a violation of the Dependency Injection principle as the class is dependent upon some implementation of an interface to initialize itself.

As far as your example code goes, I could provide a mock implementation of ISerialCom that always returns zero for GetInternalMemorySize() and it would still work. As such, your class is not "dependent upon some implementation of an interface", it is only dependent upon the interface itself.

I think the misunderstanding lies in the fact that the class still needs an(y) implementation to actually work, but that's not what the DI principle is about. As David Arno already mentioned in the comments, problems arise when you use new in the constructor.

The best mnemonic I found for this is "new is glue": Every time you use new in your code, that code is tied down to that specific implementation. If you repeatedly use new in constructors, you will create a chain of specific implementations. And because you can't "have" an instance of a class without constructing it, you cant separate that chain.

But it seems like you're not doing that, so that's fine.