fairly new to VB, is my coding ok?  
Author Message
Epoch





PostPosted: Visual Basic Language, fairly new to VB, is my coding ok? Top

i am fairly new to VB and i have made a calculator and the form looks like any normal calculator (screen, buttons etc) and it runs fine, but i cant help thinking that my coding isnt as optimal as it should be and it probably contains some 'techniques' which should not be done. any input or feedback on this code would be greatly appreciated.






Public Class CalculatorForm
Private FirstNumber As Double
Private Op As String
Private SecondNumber As Double
Private PointAlreadyUsed As Boolean = False
Private StoredNumber = 0
Private NumberStored As Boolean = False


Private Sub CalculatorForm_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
ScreenLabel.Text = "0"

End Sub

Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 1
End Sub

Private Sub Button2_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button2.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 2
End Sub

Private Sub Button3_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button3.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 3
End Sub

Private Sub Button4_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button4.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 4
End Sub

Private Sub Button5_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button5.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 5
End Sub

Private Sub Button6_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button6.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 6
End Sub

Private Sub Button7_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button7.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 7
End Sub

Private Sub Button8_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button8.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 8
End Sub

Private Sub Button9_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button9.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 9
End Sub

Private Sub Button0_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button0.Click
If ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
ScreenLabel.Text = ""
End If
ScreenLabel.Text = ScreenLabel.Text & 0
End Sub

Private Sub ButtonPoint_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonPoint.Click
If PointAlreadyUsed = True Or ScreenLabel.Text.Length >= 10 Then
Exit Sub
ElseIf ScreenLabel.Text = "error" Then
ScreenLabel.Text = ""
End If
PointAlreadyUsed = True
If ScreenLabel.Text = "" Then
ScreenLabel.Text = "0"
End If
ScreenLabel.Text = ScreenLabel.Text & "."
End Sub

Private Sub ButtonAdd_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonAdd.Click
PointAlreadyUsed = False
'store the number on screen into a variable
If ScreenLabel.Text = "" Or ScreenLabel.Text = "." Or ScreenLabel.Text = "error" Then
ScreenLabel.Text = 0
End If
FirstNumber = CDbl(ScreenLabel.Text)
ScreenLabel.Text = ""
Op = "add"

End Sub

Private Sub ButtonMinus_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonMinus.Click
PointAlreadyUsed = False
'store the number on screen into a variable
If ScreenLabel.Text = "" Or ScreenLabel.Text = "." Or ScreenLabel.Text = "error" Then
ScreenLabel.Text = 0
End If
FirstNumber = CDbl(ScreenLabel.Text)
ScreenLabel.Text = ""
Op = "minus"
End Sub

Private Sub ButtonMultiply_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonMultiply.Click
PointAlreadyUsed = False
'store the number on screen into a variable
If ScreenLabel.Text = "" Or ScreenLabel.Text = "." Or ScreenLabel.Text = "error" Then
ScreenLabel.Text = 0
End If
FirstNumber = CDbl(ScreenLabel.Text)
ScreenLabel.Text = ""
Op = "multiply"
End Sub

Private Sub ButtonDivide_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonDivide.Click
PointAlreadyUsed = False
'store the number on screen into a variable
If ScreenLabel.Text = "" Or ScreenLabel.Text = "." Or ScreenLabel.Text = "error" Then
ScreenLabel.Text = 0
End If
FirstNumber = CDbl(ScreenLabel.Text)
ScreenLabel.Text = ""
Op = "divide"
End Sub

Private Sub ButtonEquals_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonEquals.Click
If FirstNumber = 0 And SecondNumber = 0 Then
Exit Sub
End If
Dim answer As Double
If ScreenLabel.Text = "" Or ScreenLabel.Text = "." Or ScreenLabel.Text = "error" Then
ScreenLabel.Text = 0
End If
SecondNumber = CDbl(ScreenLabel.Text)
ScreenLabel.Text = ""
If Op = "add" Then
ScreenLabel.Text = FirstNumber + SecondNumber
ElseIf Op = "divide" Then
ScreenLabel.Text = FirstNumber / SecondNumber
ElseIf Op = "minus" Then
ScreenLabel.Text = FirstNumber - SecondNumber
ElseIf Op = "multiply" Then
ScreenLabel.Text = FirstNumber * SecondNumber
End If
answer = CDbl(ScreenLabel.Text)
'if the output answer is larger than 9999999999 then display error
If answer >= 10000000000 Then
ScreenLabel.Text = "error"
End If

'if the output answer is larger than 10 digits then chop it down to 10
If ScreenLabel.Text.Length >= 10 Then
ScreenLabel.Text = Microsoft.VisualBasic.Mid(ScreenLabel.Text, 1, 10)
End If


End Sub

Private Sub ButtonCancel_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonCancel.Click
FirstNumber = 0
SecondNumber = 0
ScreenLabel.Text = "0"
PointAlreadyUsed = False
End Sub


Private Sub ButtonMemPlus_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonMemPlus.Click
If ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
Exit Sub
End If
StoredNumber = ScreenLabel.Text
NumberStored = True
MRLabel.Visible = True
End Sub

Private Sub ButtonMemMinus_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonMemMinus.Click
StoredNumber = 0
NumberStored = False
MRLabel.Visible = False
End Sub

Private Sub ButtonMemRec_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ButtonMemRec.Click
If NumberStored = False Then
Exit Sub
End If
ScreenLabel.Text = StoredNumber
End Sub
End Class




Visual Basic5  
 
 
ahmedilyas





PostPosted: Visual Basic Language, fairly new to VB, is my coding ok? Top

I hope you can take these points into account and improve upon them:

  • you should convert the input text (number) into a true integer/double value then check to see its value.

  • you shouldnt do a direct CDBL() (convert to double) as you may find that the text entered is not numeric therefore if you enter letters then it will throw an exception. Either use the TryParse() method in .NET 2.0 to see if you can successfully parse the input as a true integer/double value or have a keydown event in your textbox so you can check to see if the key being pressed is a number key

    I think these 2 points are the core points you need to look on and improve in the code :-)



  •  
     
    TaDa





    PostPosted: Visual Basic Language, fairly new to VB, is my coding ok? Top

    Just a personal preference, but I prefer Select Case to If ElseIf ElseIf EndIf's. Also using Orelse in place of Or where appropriate would improve performance.
     
     
    spotty





    PostPosted: Visual Basic Language, fairly new to VB, is my coding ok? Top

    A big improvement to be made.

    Look at this code

       Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
            If ScreenLabel.Text.Length >= 10 Then
            Exit Sub
            ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
                ScreenLabel.Text = ""
            End If
            ScreenLabel.Text = ScreenLabel.Text & 1
        End Sub

        Private Sub Button2_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button2.Click
            If ScreenLabel.Text.Length >= 10 Then
                Exit Sub
            ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
                ScreenLabel.Text = ""
            End If
            ScreenLabel.Text = ScreenLabel.Text & 2
        End Sub

    Its the same code except for a single value that is being added to the text.

    Simplify you code / reduce duplication

    You have two options that you can do to remedy this.

    Option 1

    Use a Method to wrap this functionality up and simply change the calls

     

    Sub AddNumberToTextbox(byval NumberToAdd as integer)
            If ScreenLabel.Text.Length >= 10 Then
                Exit Sub
            ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
                ScreenLabel.Text = ""
            End If
            ScreenLabel.Text = ScreenLabel.Text & NumberToAdd.Tostring
    End Sub

    And then change the calls to the following

       Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
            AddNumberToTextbox(1)
        End Sub

        Private Sub Button2_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button2.Click
            AddNumberToTextbox(2)
        End Sub

    But this has some downfalls - still multiple lines of code to handle the different click events.  You passing an integer value that is ultimately being converted to a string - implied conversion will make it work, but why not just pass a string in the first place.   But more of this later.

     

    Option 2

    I'm sure each of these buttons probably has the number as the text property of them - in which case the same code can be used for each button by simply hooking the same event handler up to each button click event reducing this code down to.   If you have different text to what you want to actually use - say the button say No.1 but you want to just pass the value 1 - you may consider using the tag property instead of the text property. 

    Sub AddNumberToTextbox(byval NumberToAdd as String)
            If ScreenLabel.Text.Length >= 10 Then
                Exit Sub
            ElseIf ScreenLabel.Text = "error" Or ScreenLabel.Text = "0" Then
                ScreenLabel.Text = ""
            End If
            ScreenLabel.Text = ScreenLabel.Text & NumberToAdd
    End Sub

        Private Sub Buttonx_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click, Button2.Click, Button3.Click, Button3.Click
            AddNumberToTextbox(Sender.Text)
        End Sub

     

     

    Another Point

    Also the following line

            ScreenLabel.Text = ScreenLabel.Text & 2

    The following is not good as it depends upon a implied type conversion from a integer to a string.  Be specific and use something like

      ScreenLabel.Text = ScreenLabel.Text & Ctype(2, String)

    or if you are using variable use the .tostring property to specifically state what you are trying to do.   In this instance it would work but if you use option strict on it wont and if you get used to implied conversion and default properties - later on it will bite you when you dont get quite the expected result.

     

    Also use option explicit and option strict by adding the following to the top of you code.

    Option explicit On
    Option Strict On

    These should simplify you code down considerably.

     

    Also Put some Exception / Error handling in - there seems to be a lack of consideration for this - think divide by zero error as an example.

     

     


     
     
    Epoch





    PostPosted: Visual Basic Language, fairly new to VB, is my coding ok? Top

    once again thanks alot spotty! i cleared up my code considerably by doing what you suggested in option 2. also i have turned the strict option on in the options menu. im still learning about error handling and cant wait to start implementing it in my code. im getting the hang of casting data types a bit better now too. Thanks alot!