A Day Spent Refactoring Code

I spent a good part of today refactoring some code I’ve inherited. It’s a VB.Net wrapper around some commercial address verification software. Two examples of the type of functionality the software provides are address correction and address formatting.

Here is a simplified version of three of the classes I was given to work with: Address, CorrectedAddress and FormattedAddress. An Address is what you would provide to the verification software and the other two classes would be returned from either the address correction or address formatting functionality.

Public Class Address
    Public AddressLine As String
    Public City As String
    Public Province As String
    Public PostalCode As String
    Public Country As String
End Class
Public Class CorrectedAddress : Inherits Address
    Public Status As CorrectedAddress.Status
    Public Message As String

    Public Enum Status
        Valid
        Invalid
        Corrected
        [Error]
    End Enum
End Class
Public Class FormattedAddress
    Public AddressLineOne As String
    Public AddressLineTwo As String
    Public AddressLineThree As String

    Public Status As FormattedAddress.Status
    Public Message As String

    Public Enum Status
        Valid
        Invalid
        [Error]
    End Enum
End Class

Note that CorrectedAddress inherits from Address, but FormattedAddress does not. Both CorrectedAddress and FormattedAddress have a Status and Message properties (implemented as public fields above only for brevity). The Status properties both return similar, but not identical enumerations. In the actual code there were six classes like these two, but I am focusing only on these two for simplicity.

The whole thing didn’t seem quite right to me, and I struggled a bit to put my finger on how this should have been implemented. But then it hit me. CorrectedAddress isn’t really an Address. It’s a result of an address correction (that happens to have an address). “Is A” versus “Has A”. Remember the oft quoted Object-Oriented design principle:

Favour Composition over Inheritance

So I pulled the Address class out of the inheritance hierarchy, renamed CorrectedAddress and FormattedAddress to CorrectedResult and FormattedResult respectively and even threw in some Generics stuff to deal with the similar but different Status enumerations.

The result (the Address class was left as-is):

Public MustInherit Class Result(Of T As Structure)
    Public Status As T
    Public Message As String
End Class
Public Class CorrectedResult : Inherits Result(Of CorrectedResultStatus)
    Public Address As Address

    Public Enum CorrectedResultStatus
        Valid
        Invalid
        Corrected
        [Error]
    End Enum
End Class
Public Class FormattedResult : Inherits Result(Of FormattedResultStatus
    Public AddressLineOne As String
    Public AddressLineTwo As String
    Public AddressLineThree As String

    Public Enum FormattedResultStatus
        Valid
        Invalid
        [Error]
    End Enum
End Class

Much better!

Leave a Reply

Your email address will not be published. Required fields are marked *

*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>