2
\$\begingroup\$

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!

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Is your intention to write an "R" to the serial port whenever this class is constructed or deserialized? Why? \$\endgroup\$ May 12, 2016 at 5:01
  • 1
    \$\begingroup\$ "R" is a fixed string needed to interrogate the other end of the device the serial port is communicating to. It's outside of the code's scope. \$\endgroup\$ May 12, 2016 at 5:04
  • \$\begingroup\$ @200_success base class posted. As you can see, I can mock the IRawDataReceiver since the base class has that type as its member. So it kind of complements the restriction on the subclass with respect to serialization \$\endgroup\$ May 12, 2016 at 5:07
  • \$\begingroup\$ @200_success When the class is constructed/deserialized, I want it to immediately interrogate the device it needs to communicate to thus I placed the Task within the set accessor too. \$\endgroup\$ May 12, 2016 at 5:13
  • 1
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ May 12, 2016 at 16:49

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
9
  • \$\begingroup\$ Actually, the 'Task' was initially in a separate 'Initialize' method. I decided to include it to the 'setter' since I found myself always calling the 'Initialize' after the 'deserialization'. Though I get your point and would consider removing the 'Task' in the 'setter' accesor. Thanks! \$\endgroup\$ May 12, 2016 at 12:16
  • \$\begingroup\$ To re-assure you, it is quite normal to construct an object in several stages (calling the constructor, then setting the properties). In OO, the responsibility and rules of constructing an object rest with the "builder" (that is, you). The only catch is that you need to make sure that the building happens in only one place and according to your construction rules, say, a Factory method. \$\endgroup\$
    – RWRkeSBZ
    May 12, 2016 at 12:29
  • \$\begingroup\$ I find it funny that what you just said was my initial approach where I have a 'factory' method and it calls the 'Initialize' method after constructing the object. :)) \$\endgroup\$ May 12, 2016 at 12:39
  • \$\begingroup\$ At the risk of sounding presumptuous, I was in this situation, too, early in my career when a lack of validation of my ideas drove me to go back and forth between good and bad solutions. If you are interested in good design, I would recommend learning about state machines and stick with it until the penny drops. \$\endgroup\$
    – RWRkeSBZ
    May 12, 2016 at 13:01
  • \$\begingroup\$ I've made quite a few refactors now into this program alone, I guess it's part of learning. :) I'm interested on learning about state machines. could you recommend some books/sites/references where I could start? Thanks! \$\endgroup\$ May 12, 2016 at 13:11
1
\$\begingroup\$
  • 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.

\$\endgroup\$
4
  • \$\begingroup\$ As per your first advice, if I put those codes in the 'constructor', it will not get called during 'deserialization' in where I would need to rewrite those codes somewhere to get it called (e.g. '[OnDeserialized]').. \$\endgroup\$ May 12, 2016 at 12:20
  • \$\begingroup\$ As per your 2nd advice, I have three things... 1.) If I make the 'constructor' argument as type 'SerialPortConnection', I will not be able to 'mock' it during test... 2.) I need to put a 'null' checker for the 'set' accessor to avoid 'NRE' in base class... 3.) With number 2 done, if I pass any implementation (e.g. 'mock') of 'IRawDataReceiver' to 'construct' 'Hazen', my 'base' class will still have that implementation but 'Hazen' will not. Though that's ok since all I need to test are exposed in the 'base' class \$\endgroup\$ May 12, 2016 at 12:33
  • \$\begingroup\$ @PaoloGO: If you want to test your base class, your could implement a sub class just for testing. IMHO if Hazen does only work with a special classs, there is no reason to allow to pass an abstraction because it hides the fact that Hazen could not handle other implementations. \$\endgroup\$
    – JanDotNet
    May 12, 2016 at 12:45
  • \$\begingroup\$ I beg to disagree. there is a reason, mock. And classes depending on IO classes, such as SerialPort, should have a way to accept a mock for it. \$\endgroup\$ May 12, 2016 at 13:06
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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