2
\$\begingroup\$

When a user clicks on "Case", it pops up a box where they have to input a number, which is later used to calculate the value to Euro and around to other values. After this, it checks which value is larger than 0 and writes it to the display box with a white line between each value.

I want to optimize this code. What's the best way to do it (meaning fewer double lines)? I am reading through the book but as far as I know, there's nothing on optimizing the code (it seems ridiculous to write those lines every time).

Private Sub munteenheidlistbox_SelectedIndexChanged(sender As Object, e As EventArgs) Handles munteenheidlistbox.SelectedIndexChanged
    ' On case selection pops up the Inputbox to set a number
    ' When a number is set it checks if its inside the paramets if not pops a msgbox and repops the Inputbox for new user input
    ' On case else pops up a msgbox saying the user should click on a valuta instead.
    Select Case munteenheidlistbox.SelectedIndex
        Case 0
            usdvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Amerikaanse dollars (1 Eur = .... Amerikaanse Dollars")
            Do Until usdvalue >= 0 And usdvalue <= 500
                MsgBox("De parameters vallen buiten de wisselkoers. Lees de text nog maals in het volgende scherm")
                usdvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Amerikaanse dollars (1 Eur = .... Amerikaanse Dollars")
            Loop
        Case 2
            gbpvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Britse Ponden (1 Eur = .... Britse Ponden")
            Do Until gbpvalue >= 0 And gbpvalue <= 500
                MsgBox("De parameters vallen buiten de wisselkoers. Lees de text nog maals in het volgende scherm")
                gbpvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Amerikaanse dollars (1 Eur = .... Amerikaanse Dollars")
            Loop
        Case 4
            eurovalue = MsgBox("Wissel koers van Euro naar Euro is altijd 1.")
            eurovalue = 1
        Case 6
            rusvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Russische Roebel (1 Eur = .... Russische Roebel")
            Do Until rusvalue >= 0 And rusvalue <= 500
                MsgBox("De parameters vallen buiten de wisselkoers. Lees de text nog maals in het volgende scherm")
                rusvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Amerikaanse dollars (1 Eur = .... Amerikaanse Dollars")
            Loop
        Case 8
            japyenvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Japanese Yen (1 Eur = .... Japanse Yen")
            Do Until japyenvalue >= 0 And japyenvalue <= 500
                MsgBox("De parameters vallen buiten de wisselkoers. Lees de text nog maals in het volgende scherm")
                japyenvalue = InputBox("Geef de wisselkoers (> 0 en < 500) voor de Euro tov Amerikaanse dollars (1 Eur = .... Amerikaanse Dollars")
            Loop
        Case Else
            MsgBox("Klik alstublieft op een valuta")
    End Select

    'Clear input and check which values are greater -> and rebuild textbox.
    Wisselkoersenlistbox.Clear()
    If usdvalue > 0 Then
        Wisselkoersenlistbox.Text = Wisselkoersenlistbox.Text & "1 euro = " & usdvalue & usdtext & vbCrLf
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If

    If gbpvalue > 0 Then
        Wisselkoersenlistbox.Text = Wisselkoersenlistbox.Text & "1 euro = " & gbpvalue & gbptext & vbCrLf
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If

    If eurovalue > 0 Then
        Wisselkoersenlistbox.Text = Wisselkoersenlistbox.Text & "1 euro = " & eurovalue & eurtext & vbCrLf
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If

    If rusvalue > 0 Then
        Wisselkoersenlistbox.Text = Wisselkoersenlistbox.Text & "1 euro = " & rusvalue & rustext & vbCrLf
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If

    If japyenvalue > 0 Then
        Wisselkoersenlistbox.Text = Wisselkoersenlistbox.Text & "1 euro = " & japyenvalue & japtext & vbCrLf
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If
End Sub
\$\endgroup\$
2
\$\begingroup\$

Here is a possible solution:

Private currencies As New Dictionary(Of String, CurrencyItem)
Private Class CurrencyItem
    Public CurrencyName As String
    Public CurrencyPrompt As String
    Public Sub New(currencyName As String, currencyPrompt As String)
        Me.CurrencyName = currencyName
        Me.CurrencyPrompt = currencyPrompt
    End Sub
End Class

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles Me.Load
    currencies.Add("USD", New CurrencyItem("USD", "Amerikaanse dollars"))
    currencies.Add("GBP", New CurrencyItem("GBP", "Britse Ponden"))
    currencies.Add("RBL", New CurrencyItem("RBL", "Russische Roebel"))
End Sub

Private Sub munteenheidlistbox_SelectedIndexChanged(sender As Object, e As EventArgs) Handles munteenheidlistbox.SelectedIndexChanged
    Try
        If isValidSelection(munteenheidlistbox.SelectedIndex) Then
            calculateCurrency(translateCurrency(munteenheidlistbox.SelectedIndex))
        End If
    Catch ex As Exception
        MessageBox.Show(String.Concat("An error occurred: ", ex.Message))
    End Try
End Sub

''' <summary>
''' check if the selection is value
''' </summary>
''' <param name="selectedIndex"></param>
''' <returns></returns>
Private Function isValidSelection(selectedIndex As Integer) As Boolean
    Select Case selectedIndex
        Case 0, 2, 4, 6, 8
            Return True
        Case Else
            MsgBox("Klik alstublieft op een valuta")
            Return False
    End Select
End Function

''' <summary>
''' get a currency item
''' </summary>
''' <param name="listIndex"></param>
''' <returns></returns>
Private Function translateCurrency(listIndex As Integer) As CurrencyItem
    Select Case listIndex
        Case 0
            Return currencies("USD")
        Case 2
            Return currencies("GBP")
        Case Else    'handle not found
            '''
    End Select
End Function

''' <summary>
''' prompt and display
''' </summary>
''' <param name="currency"></param>
Private Sub calculateCurrency(currency As CurrencyItem)

    Dim curValue As Integer = CInt(InputBox(String.Format("Geef de wisselkoers (> 0 en < 500) voor de Euro tov {0} (1 Eur = .... {1}", currency.CurrencyPrompt, currency.CurrencyPrompt)))

    Do Until curValue >= 0 And curValue <= 500
        MsgBox("De parameters vallen buiten de wisselkoers. Lees de text nog maals in het volgende scherm")
        curValue = CInt(InputBox(String.Format("Geef de wisselkoers (> 0 en < 500) voor de Euro tov {0} (1 Eur = .... {1}", currency.CurrencyPrompt, currency.CurrencyPrompt)))
    Loop

    Wisselkoersenlistbox.Clear()
    If curValue > 0 Then
        Wisselkoersenlistbox.Text = String.Concat(Wisselkoersenlistbox.Text, "1 euro = ", curValue, Environment.NewLine)
        Wisselkoersenlistbox.AppendText(Environment.NewLine & "")
    End If
End Sub

My insightful observation is that the code can be reduced to a number of simpler methods.

The purpose of the CurrencyItem class is to support future changes without having to hard code a number of if statements. Separating the selected index validation, and the "calculation" on the currency allows the program to have distinct purposes for distinct methods.

\$\endgroup\$

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.