4
\$\begingroup\$

Got the following piece of code:

          If boolFlag Then
              With offer.Person1
                entity.Birthdate = .BirthDate

                entity.FirstName = .FirstGivenName
                entity.LastName = .FamilyName

                entity.Street = .StreetName
                entity.HousNr = .HouseNumberIdentifier
                entity.BoxNr = .BoxNumberIdentifier
                entity.PostalCode = .PostalCode
                entity.Municipality = .CityName
            End With
        Else
            With offer.Person2
                entity.Birthdate = .BirthDate

                entity.FirstName = .FirstGivenName
                entity.LastName = .FamilyName

                entity.Street = .StreetName
                entity.HousNr = .HouseNumberIdentifier
                entity.BoxNr = .BoxNumberIdentifier
                entity.PostalCode = .PostalCode
                entity.Municipality = .CityName
            End With
        End If

Objects Person1 and Person2 are of different types, and those types do not share a common Interface / class. (And that can't be changed)

How do I avoid the duplicate code inside the With-blocks?

\$\endgroup\$
  • \$\begingroup\$ Can only think of using reflection to map properties but that seems overkill in this case, unless code snippets like this is scattered throughout your code then it may be worth looking into. \$\endgroup\$ – dreza Mar 23 '12 at 20:56
  • 1
    \$\begingroup\$ If you have a lot of right-to-left code like this you should look into Automapper automapper.codeplex.com \$\endgroup\$ – George Mauer Mar 24 '12 at 15:45
  • \$\begingroup\$ Automapper seems like a neat project. \$\endgroup\$ – Randy Mar 24 '12 at 16:50
  • \$\begingroup\$ Jonas, any thoughts or feedback regarding the solutions provided below? \$\endgroup\$ – Randy Mar 24 '12 at 19:56
1
\$\begingroup\$

Options:

  1. Change the original code base (best option, what should happen)
  2. Inherit your own objects from the originals and attach an interface (given your scenario, this is the option to go with)
  3. Use static helper methods ( you can abstract and decorate the routine to make it better in your core programming, but the code will still be duplicated.

These items are listed in order of how you should approach the solution. I hope this helps.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 but I want to make the point that (1) is not necessarily the best option. The similarity in property names could be a coincidence or (as I suspect might be happening here) this could be an integration layer between bounded contexts. In that case, changing the codebase would definitely be a mistake. \$\endgroup\$ – George Mauer Mar 24 '12 at 15:55
  • \$\begingroup\$ Thanks George. I think your point is well taken. Since the example was showing that on the surface person1 and person2 were identical, it seemed that the primary source code was designed incorrectly. Even if this example was integrating two different sources, it seems that something should be done upstream. \$\endgroup\$ – Randy Mar 24 '12 at 16:47
  • \$\begingroup\$ @GeorgeMauer: coincidence or integration layer, I wouldn't say that a common interface was a mistake, possibly not possible in the case of an integration layer where you don't have total control over all of the code, but not a mistake. \$\endgroup\$ – jmoreno Mar 25 '12 at 18:55
  • \$\begingroup\$ In this case we are unable to make any changes to both the class being mapped from, as the class being mapped to. But special thanks for mentioning option 2, since I hadn't thought of that yet, and I'm sure I'll be able to use that solution again in the future. \$\endgroup\$ – Jonas Mar 26 '12 at 6:34
2
\$\begingroup\$

Unless you can make them share a common interface, it is not desirable to eliminate the code duplication -- best case (vb.net late binding on Object), trades your code size for the compilers. Doing the work yourself, simply means more code for you.

That said, where the code duplication is, is something you might want to control. Extract each assignment out into a similarly named method, and call the appropriate one.

Sub UpdateEntityFromPerson(ByVal entity as Entity, ByVal person as Person1)
    With person
        entity.Birthdate = .BirthDate

        entity.FirstName = .FirstGivenName
        entity.LastName = .FamilyName

        entity.Street = .StreetName
        entity.HousNr = .HouseNumberIdentifier
        entity.BoxNr = .BoxNumberIdentifier
        entity.PostalCode = .PostalCode
        entity.Municipality = .CityName
    End With
End Sub

Sub UpdateEntityFromPerson(ByVal entity as Entity, ByVal person as Person2)
    With Person
        entity.Birthdate = .BirthDate

        entity.FirstName = .FirstGivenName
        entity.LastName = .FamilyName

        entity.Street = .StreetName
        entity.HousNr = .HouseNumberIdentifier
        entity.BoxNr = .BoxNumberIdentifier
        entity.PostalCode = .PostalCode
        entity.Municipality = .CityName
    End With
End Sub

        If boolFlag Then
            UpdateEntityFromPerson(entity, offer.person1)
        Else
            UpdateEntityFromPerson(entity, offer.person2)
        End If

All of the code is still duplicated, but it type safe, clear, and easy to use and understand. Of course, the best thing to do is probably to have an entity constructor that takes one or the other, or a builder that does so, but that depends upon other factors, outside your snippet...

\$\endgroup\$
  • \$\begingroup\$ Simplest solution, not the one I had hoped for, but in this particular case the one that's most applicable. Thanks for the good argumentation why to this solution is viable! \$\endgroup\$ – Jonas Mar 26 '12 at 6:52
  • \$\begingroup\$ Yeah, what you really want is the ability to slap an interface onto an existing class, but you can't do that. If you are using these properties a lot, you can implement a Adapter that take either class and then wraps the interaction with them. One advantage of this would be that you could then define your interface, and then if you could later modify one of the classes by adding an interface, you could do so. I'm tempted to post this as an alternate answer just to show that it is equivalent in size to the reflection method, while still being safe. \$\endgroup\$ – jmoreno Mar 26 '12 at 20:14
  • \$\begingroup\$ Well, when I say you can't do that, I mean the compiler doesn't do it for you, you can build your own code that does. As an example of that see this codeProject DuckTyping article. \$\endgroup\$ – jmoreno Mar 26 '12 at 20:26
1
\$\begingroup\$

One possible solution could look like this, where "[yourEntityType]" is the type of "entity" in your code:

Public Class CopyHelper(Of T As Class)
        Public Shared Sub UpdateEntity(person As T, entity As [yourEntityType])

            Dim birthDateProperty As System.Reflection.PropertyInfo = person.GetType().GetProperty("BirthDate")

            If Not IsNothing(birthDateProperty) Then
                entity.Birthdate = birthDateProperty.GetValue(person, Nothing)
            End If

            Dim firstGivenNameProperty As System.Reflection.PropertyInfo = person.GetType().GetProperty("FirstGivenName")
            If Not IsNothing(firstGivenNameProperty) Then
                entity.FirstName = firstGivenNameProperty.GetValue(person, Nothing)
            End If

            ...

        End Sub
End Class

and finally your code will look as below

If boolFlag Then
    CopyHelper(Of Person1).UpdateEntity(offer.Person1, entity)
Else
    CopyHelper(Of Person2).UpdateEntity(offer.Person2, entity)
End If

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Nice additional option. Two items to watch out for 1) the performance hit of using reflection 2) using magic strings which also removes compile time compliance. \$\endgroup\$ – Randy Mar 24 '12 at 17:34
  • \$\begingroup\$ Sweet. I'd take this one step further and move mapping code into it's own method. Something like GetValueOrDefault(person, "FirstGivenName", ""). Help avoid code duplication. \$\endgroup\$ – dreza Mar 24 '12 at 18:43
  • \$\begingroup\$ Dreza, decorating the local object with a helper method or just attaching a method directly to the object would make it more fluent in the code base and achieve the result you were looking for. \$\endgroup\$ – Randy Mar 24 '12 at 19:40
  • \$\begingroup\$ Thanks for the answer, indeed a fine way to solve the code duplication. However, in this case it would decrease readability too much for the small advantage I would get out of it. \$\endgroup\$ – Jonas Mar 26 '12 at 6:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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