Code Smell in Unit Testing Serialized Classes

Question

I can feel that something is wrong with my code. Here's an excerpt where I think the code smell is...

    internal Hazen(IRawDataReceiver rawDataReceiver) : base(rawDataReceiver)
    {
        SerialPortConnection = rawDataReceiver as SerialPortConnection;
    }
    private SerialPortConnection _serialPortConnection;
    [DataMember] private SerialPortConnection SerialPortConnection
    {
        get
        {
            return _serialPortConnection;
        }
        set
        {
            if(value != null)
            {
                _serialPortConnection = value;
                RawDataReceiver = value;
            }
        }
    }

So basically, Hazen inherits from an abstract class. Notice how I pass the rawDataReceiver to its base class when Hazen's constructor is called. But during deserialization, the constructor is bypassed, thus I included the RawDataReceiver = value to the set accessor of the DataMember property. That is where the code smell is. Is there a better way to do it and am I violating any good practices (such as DRY)?

As regards to unit testing. As you can see, Hazen has a property of type SerialPortConnection as opposed to having it as IRawDataReceiver since you can't serialize an interface. Now, I can still test this class since the base class exposes everything I need for testing. But going back to the previous paragraph, I feel like I'm doing something wrong here.

EDIT: Here's a snippet of the base class

    internal TideDevice(IRawDataReceiver rawDataReceiver)
    {
        RawDataReceiver = rawDataReceiver;
    }
    private IRawDataReceiver _rawDataReceiver;
    protected internal IRawDataReceiver RawDataReceiver
    {
        get
        {
            return _rawDataReceiver;
        }
        protected set
        {
            _rawDataReceiver = value;
            RawDataReceiver.RawDataReceived += RawDataReceiver_RawDataReceived;
        }
    }

EDIT: I have extracted the Task to another method to remove the responsibility of the set accessor. But I'm still in a roadblock on fixing the code smell I initially stated. I editted the code to reflect what I have now.

EDIT2: This code is working really fine whether in unit test and integration test, so I'll leave it as is until someone points out that there is something wrong here or until a problem arises. Thank you very much!

Answer 1

I think the bigger issue is that you have an operation with side-effect in the property setter SerialPortConnection, which means that if your unit test fails, it will not be clear whether it is due to a problem with deserialisation or with the operation.

I would suggest moving the operation (i.e. writing "R" to the port) in a separate Initialize method that you can test individually.

Answer 2

• You pass the rawDataReceiver instance to the base class via base(rawDataReceiver). That will attach the event handler by the setter. In setter of property SerialPortConnection, the rawDataReceiver is set again to the base class's property RawDataReceiver and the event handler is added a second time. To avoid that, remove the setter of RawDataReceiver, assign the value (and attach the event handler) in constuctor of the base class and make the field _rawDataReceiver readonly.

• In constructor of Hazen, you assume, that the passed object is of type 'SerialPortConnection'. If that is always the case, you can change the type of the constructor argument to SerialPortConnection. If that is not always the case, you get a NRE when trying to set the base class's DataReceiver property.

Answer 3

So after some thinking, this is now the final code for the Hazen class.. the abstract/base class TideDevice stays the same.

internal Hazen(IRawDataReceiver rawDataReceiver) : base(rawDataReceiver)
{
    _serialPortConnection = rawDataReceiver as SerialPortConnection;
}
[DataMember] private SerialPortConnection _serialPortConnection;

[OnDeserialized] private void OnDeserialized(StreamingContext sc)
{
    RawDataReceiver = _serialPortConnection;
}

So as you can see, the [OnDeserialized] method will act as the constructor after deserialization. Or more specifically, it mimics the base(rawDataReceiver) part of "actual" constructor.

That's it. Thanks @jeyoung and @JNS.