Results 1 to 3 of 3

Thread: Refactor select-if statement with lot of duplicate code

  1. #1

    Thread Starter
    Frenzied Member
    Join Date
    May 2002
    Posts
    1,602

    Refactor select-if statement with lot of duplicate code

    Hi!

    I came across this piece of code today:

    Code:
    Public Class Form1
        'Viewmodel stuff that binds to a UI
        Private Property ShowA1 As Boolean
        Private Property ShowA2 As Boolean
        Private Property ShowB1 As Boolean
        Private Property ShowB2 As Boolean
        Private Property ShowC1 As Boolean
        Private Property ShowC2 As Boolean
    
    
        Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
            Dim dummyList As New List(Of Axis)
    
            dummyList.Add(New Axis With {.Name = "A", .TypeOfUnit = TypeOfUnit.Unit1})
            dummyList.Add(New Axis With {.Name = "B", .TypeOfUnit = TypeOfUnit.Unit2})
            dummyList.Add(New Axis With {.Name = "C", .TypeOfUnit = TypeOfUnit.Unit3})
    
            ShowOrHideUIStuff(dummyList)
        End Sub
    
        Private Sub ShowOrHideUIStuff(list As List(Of Axis))
            For Each axis As Axis In list
                Select Case axis.Name
                    Case "A"
                        If (axis.TypeOfUnit = TypeOfUnit.Unit1) Then
                            ShowA1 = True
                            ShowA2 = False
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit2) Then
                            ShowA1 = False
                            ShowA2 = True
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit3) Then
                            ShowA1 = True
                            ShowA2 = True
                        End If
                    Case "B"
                        If (axis.TypeOfUnit = TypeOfUnit.Unit1) Then
                            ShowB1 = True
                            ShowB2 = False
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit2) Then
                            ShowB1 = False
                            ShowB2 = True
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit3) Then
                            ShowB1 = True
                            ShowB2 = True
                        End If
                    Case "C"
                        If (axis.TypeOfUnit = TypeOfUnit.Unit1) Then
                            ShowC1 = True
                            ShowC2 = False
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit2) Then
                            ShowC1 = False
                            ShowC2 = True
                        ElseIf (axis.TypeOfUnit = TypeOfUnit.Unit3) Then
                            ShowC1 = True
                            ShowC2 = True
                        End If
                End Select
            Next
        End Sub
    End Class
    
    
    Public Class Axis
        Public Property Name As String
        Public Property TypeOfUnit As TypeOfUnit
    
    End Class
    
    Public Enum TypeOfUnit
        Unit1 = 1 ' Show one axis in the plot
        Unit2 = 2 ' Show another type of axis in the plot
        Unit3 = 3 ' Show both axises in the plot
    End Enum

    It is from a viewmodel, where some axis on a graph is bound to boolean properties. The code is rewritten a bit to make it easier to understand, I removed some MVVM stuff. Basically, some curve datapoints and some metadata is fetched from the database through a business object. The viewmodel get a list of axis that it is supposed to show/hide depending on configuration. Each axis has a name, and some unit type which is an enum.

    The list of axis are passed through that big smelly select case statement and everything works fine. But the code itself doesn't look good. I stated thinking about how to refactor the code. Two things came into mind, dictionaties and polymorphism. But I couldn't quite figure out how either of those would help out here. I could subclass axis to a AAxis, BAxis and CAxis but that wouldn't help much, because of the relation to the properties on the viewmodel (the booleans at top) They need to be set properly for the plot to be displayed according to the config. The plot is nothing we can change for now. We simply need to set these pesky booleans.

    One thing I thought about that could help some was to introduce some helper properties to the Axis class like this

    Code:
     Private Sub ShowOrHideUIStuff(list As List(Of Axis))
            For Each axis As Axis In list
                Select Case axis.Name
                    Case "A"
                        ShowA1 = axis.Show1
                        ShowA2 = axis.Show2
                    Case "B"
                        ShowB1 = axis.Show1
                        ShowB2 = axis.Show2
                    Case "C"
                        ShowC1 = axis.Show1
                        ShowC2 = axis.Show2
                End Select
            Next
        End Sub
    
    
    Public Class Axis
        Public Property Name As String
        Public Property TypeOfUnit As TypeOfUnit
        Public ReadOnly Property Show1 As Boolean
            Get
                If (TypeOfUnit = TypeOfUnit.Unit1) Then
                    Return True
                ElseIf (TypeOfUnit = TypeOfUnit.Unit2) Then
                    Return False
                ElseIf (TypeOfUnit = TypeOfUnit.Unit3) Then
                    Return True
                Else : Return False
                End If
            End Get
        End Property
    
        Public ReadOnly Property Show2 As Boolean
            Get
                If (TypeOfUnit = TypeOfUnit.Unit1) Then
                    Return False
                ElseIf (TypeOfUnit = TypeOfUnit.Unit2) Then
                    Return True
                ElseIf (TypeOfUnit = TypeOfUnit.Unit3) Then
                    Return True
                Else : Return False
                End If
            End Get
        End Property
    But that doesn't feel too good either. I dont really like logic in properties, plus we want to keep the data as generic as possible since we might decide to replace the current plot component, and that might not have these pesky booleans that control it's state (not a primary requirement though). It also feels like the code in the Show1 and Show2 properties could be refactored further?

    What do you think, is there cleaner way to handle this? I like polymorphism, but in this case I can't see how it would help us?

    /H

  2. #2
    Bad man! ident's Avatar
    Join Date
    Mar 2009
    Location
    Cambridge
    Posts
    5,401

    Re: Refactor select-if statement with lot of duplicate code

    So is there any thing wrong with your code?
    My Github - 1d3nt

  3. #3

    Thread Starter
    Frenzied Member
    Join Date
    May 2002
    Posts
    1,602

    Re: Refactor select-if statement with lot of duplicate code

    Quote Originally Posted by ident View Post
    So is there any thing wrong with your code?

    Well it works, but code that works is not always good code...

    Like I stated

    But that doesn't feel too good either. I dont really like logic in properties, plus we want to keep the data as generic as possible since we might decide to replace the current plot component, and that might not have these pesky booleans that control it's state (not a primary requirement though). It also feels like the code in the Show1 and Show2 properties could be refactored further?
    Cheers
    /Henrik

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  



Click Here to Expand Forum to Full Width