Question Review Required

[james_h]

Hi Guys,

I am brand new to programming and I have been teaching myself VB 2010 for 2 weeks and would appreciate someone that knows what they are doing to have a look to see what they think of my efforts on this little project. It all seems to work with no problems, so I guess that is a good start. I am guessing there may be more efficient ways to achieve the end results maybe?

Not sure if this is the best way to go about this?

Here is the code for the project.

Public Class Form1


    Dim wins As Integer


    Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load


        RadioButton2.Checked = True
        ComboBox1.Text = 1
        Label2.Text = 0


    End Sub


    Private Sub Timer1_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Timer1.Tick


        Dim rnd As New Random


        PictureBox1.Location = New Point(rnd.Next(1, 250), rnd.Next(60, 250))


    End Sub




    Private Sub RadioButton1_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton1.CheckedChanged


        If RadioButton1.Checked = True Then
            Label4.Visible = False
            If wins >= 10 Then
                PictureBox1.BackgroundImage = Image.FromFile("C:\Users\james\Documents\Visual Studio 2010\Icons\Bird-red.png")
            End If
            Timer1.Start()
        End If


    End Sub


    Private Sub RadioButton2_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton2.CheckedChanged


        If RadioButton2.Checked = True Then
            Timer1.Stop()
            PictureBox1.Location = New Point(165, 250)
        End If


    End Sub


    Private Sub ComboBox1_SelectedIndexChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ComboBox1.SelectedIndexChanged


        If ComboBox1.Text = 1 Then
            Timer1.Interval = 1000
        ElseIf ComboBox1.Text = 2 Then
            Timer1.Interval = 750
        ElseIf ComboBox1.Text = 3 Then
            Timer1.Interval = 650
        ElseIf ComboBox1.Text = 4 Then
            Timer1.Interval = 500
        ElseIf ComboBox1.Text = 5 Then
            Timer1.Interval = 350
        End If


    End Sub


    Private Sub PictureBox1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles PictureBox1.Click


        Label2.Text = CInt(wins)


        If RadioButton1.Checked = True And Timer1.Interval = 1000 Then
            RadioButton2.Checked = True
            MessageBox.Show("Congrats!! You have won 1 coin!!")
            wins += 1
            Label2.Text = wins
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 750 Then
            RadioButton2.Checked = True
            MessageBox.Show("Congrats!! You have won 2 coins!!")
            wins += 2
            Label2.Text = wins
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 650 Then
            RadioButton2.Checked = True
            MessageBox.Show("Congrats!! You have won 3 coins!!")
            wins += 3
            Label2.Text = wins
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 500 Then
            RadioButton2.Checked = True
            MessageBox.Show("Congrats!! You have won 4 coins!!")
            wins += 4
            Label2.Text = wins
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 350 Then
            RadioButton2.Checked = True
            MessageBox.Show("Congrats!! You have won 5 coins!!")
            wins += 5
            Label2.Text = wins
        Else
            MessageBox.Show("The Game isn't running! Start Game to Win!!!")
        End If


    End Sub
End Class

And a screen shot of the actual game



Thanks for having a look guys.

 

[jmcilhinney]

First things first, there are no comments. You are asking us to look to see if what you've done is the best way to do what you were trying to do but we don't know what you were trying to do without reading the code. If the code doesn't do what it's supposed to then we have no way of knowing because we can only see what it does do, not what it was supposed to do.

 

[Solitaire]

First and foremost, please turn Option Strict ON. This enforces strict type conversion. With Option Strict Off (the default) you can get all sorts of subtle errors that are hard to debug. Statements such as:
ComboBox1.Text = 1
should not be allowed. It should be:
ComboBox1.Text = "1"

Also, Label2.Text = wins
is wrong. It should be:
Label2.Text = wins .ToString()

If you need to convert a string to an integer, use the Convert method. Example:
wins = Convert.ToInt32(Label2.Text)

You can place the statement at the top of your current program:
Option Strict On

It will then show you all the places where you need to make corrections.


For future programs, select the option as follows:
Click Tools/Options/Projects and Solutions/VB Defaults

 

[james_h]

Yep that makes sense!

I will try again then

I have modified the code a bit from the last post as I worked out how to use the resources folder rather then a path to a image file

Here is the revised Code.

Public Class Form1


    Dim wins As Integer




    Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load


        'Set the form controls to the correct settings
        RadioButton2.Checked = True
        ComboBox1.Text = 1
        Label2.Text = 0


    End Sub


    Private Sub Timer1_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Timer1.Tick


        'Set random position for the ball to be placed
        Dim rnd As New Random


        PictureBox1.Location = New Point(rnd.Next(1, 250), rnd.Next(60, 250))


    End Sub




    Private Sub RadioButton1_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton1.CheckedChanged


        'Check if timer needs to be started, hide textbox and background image
        If RadioButton1.Checked = True Then
            Label4.Visible = False
            txtCoinsWon.Hide()
            Me.BackgroundImage = Nothing
            PictureBox1.Show()
            'Display new ball if coins won match 10-19 or 20 and higher
            If wins >= 20 Then
                PictureBox1.BackgroundImage = My.Resources.BirdRed
            ElseIf wins >= 10 Then
                PictureBox1.BackgroundImage = My.Resources.FaceSmile
            End If
            Timer1.Start()
        End If


    End Sub


    Private Sub RadioButton2_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton2.CheckedChanged


        'Check if timer needs to be stopped and reset ball location
        If RadioButton2.Checked = True Then
            Timer1.Stop()
            PictureBox1.Location = New Point(165, 250)
        End If


    End Sub


    Private Sub ComboBox1_SelectedIndexChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ComboBox1.SelectedIndexChanged


        'Set the timer speed depending on the users level choice
        If ComboBox1.Text = 1 Then
            Timer1.Interval = 1000
        ElseIf ComboBox1.Text = 2 Then
            Timer1.Interval = 750
        ElseIf ComboBox1.Text = 3 Then
            Timer1.Interval = 650
        ElseIf ComboBox1.Text = 4 Then
            Timer1.Interval = 500
        ElseIf ComboBox1.Text = 5 Then
            Timer1.Interval = 350
        End If


    End Sub


    Private Sub PictureBox1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles PictureBox1.Click


        'Convert text to an int so it can be added to the wins variable
        Label2.Text = CInt(wins)


        'Check which level the user is playing at and award them the correct score
        'Display a message and picture to tell user what they have won
        'Reset the wins variable to the new total
        'Check if the user has reached 10 or 20 coins and display a message if they have
        If RadioButton1.Checked = True And Timer1.Interval = 1000 Then
            RadioButton2.Checked = True
            PictureBox1.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.SmileyDollar
            txtCoinsWon.Text = ("Congrats!! You have won 1 coin!!")
            wins += 1
            Label2.Text = wins
            If wins = 10 Then
                'I am not sure how to go about making this message display only the once if the score is between 11-19???
                MessageBox.Show("You have 10 coins, you have won a new ball")
            End If
            If wins = 20 Then
                'I am not sure how to go about making this message display only the once if the score is 21 or higher???
                MessageBox.Show("You have 20 coins, you have won a new ball")
            End If
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 750 Then
            RadioButton2.Checked = True
            PictureBox1.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.SmileyDollar
            txtCoinsWon.Text = ("Congrats!! You have won 2 coin!!")
            wins += 2
            Label2.Text = wins
            If wins = 10 Then
                MessageBox.Show("You have 10 coins, you have won a new ball")
            End If
            If wins = 20 Then
                MessageBox.Show("You have 20 coins, you have won a new ball")
            End If
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 650 Then
            RadioButton2.Checked = True
            PictureBox1.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.SmileyDollar
            txtCoinsWon.Text = ("Congrats!! You have won 3 coin!!")
            wins += 3
            Label2.Text = wins
            If wins = 10 Then
                MessageBox.Show("You have 10 coins, you have won a new ball")
            End If
            If wins = 20 Then
                MessageBox.Show("You have 20 coins, you have won a new ball")
            End If
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 500 Then
            RadioButton2.Checked = True
            PictureBox1.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.SmileyDollar
            txtCoinsWon.Text = ("Congrats!! You have won 4 coin!!")
            wins += 4
            Label2.Text = wins
            If wins = 10 Then
                MessageBox.Show("You have 10 coins, you have won a new ball")
            End If
            If wins = 20 Then
                MessageBox.Show("You have 20 coins, you have won a new ball")
            End If
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 350 Then
            RadioButton2.Checked = True
            PictureBox1.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.SmileyDollar
            txtCoinsWon.Text = ("Congrats!! You have won 5 coin!!")
            wins += 5
            Label2.Text = wins
            If wins = 10 Then
                MessageBox.Show("You have 10 coins, you have won a new ball")
            End If
            If wins = 20 Then
                MessageBox.Show("You have 20 coins, you have won a new ball")
            End If


            'If user tries to click ball while game isn't running they get an error message appear
        Else
            PictureBox1.Hide()
            Label4.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.Skull2
            txtCoinsWon.Text = ("The Game isn't running, Start Game to Win Coins")
        End If


    End Sub


End Class

Form Image



Thanks Guys

 

[jmcilhinney]

1. There should be no need to set control properties in the Load event handler unless based on some external condition. The properties you're setting should be done in the designer.
2. Never create Random objects repeatedly and use them once each. Always create one Random object and use it multiple times.
3. Your SelectedIndexChanged event handler should be using a Select Case.
4. Your Click event handler contains a terrible amount of duplicate code.

 

[james_h]

1. There should be no need to set control properties in the Load event handler unless based on some external condition. The properties you're setting should be done in the designer.
2. Never create Random objects repeatedly and use them once each. Always create one Random object and use it multiple times.
3. Your SelectedIndexChanged event handler should be using a Select Case.
4. Your Click event handler contains a terrible amount of duplicate code.

Ok I understand point Number 1 and 3

Point 2 have I not only created 1 random object??? How should the code look?

Point 4 I hear what your saying about repeated code but how do I avoid using repetitive code and still achieve the end result?

Thanks

 

[Dunfiddlin]

Ok I understand point Number 1 and 3

Point 2 have I not only created 1 random object??? How should the code look?

Point 4 I hear what your saying about repeated code but how do I avoid using repetitive code and still achieve the end result?

Thanks

Yup. I'm with you on No. 2! Looks just fine to me.

As for repeated code ...

Put it in a new sub, eg.

Sub Win(Prize As String, Amount As Integer)
RadioButton2.Checked = True
PictureBox1.Hide()
 txtCoinsWon.Show()
 Me.BackgroundImage = My.Resources.SmileyDollar
 txtCoinsWon.Text = ("Congrats!! You have won " & prize & " !!")
 wins = wins + Amount
Label2.Text = wins
If wins = 10 Then 'I am not sure how to go about making this message display only the once if the score is between 11-19???
MessageBox.Show("You have 10 coins, you have won a new ball")
End If
 If wins = 20 Then
 'I am not sure how to go about making this message display only the once if the score is 21 or higher???
 MessageBox.Show("You have 20 coins, you have won a new ball")
 End If
End Sub

And then in your main procedure call it ...

If RadioButton1.Checked = True And Timer1.Interval = 1000 Then
Win("1 coin", 1)
'etc.

 

[Dunfiddlin]

Oh and for the new ball notices

Dim Ten As Boolean = False
Dim Twenty As Boolean = False



If wins >= 10 AndAlso Ten = False
Ten = True
'etc.

 

[JohnH]

Point 2 have I not only created 1 random object??? How should the code look?

No, you're creating a new Random object for each Timer Tick event. Rather move the declaration of the Random variable to a private class field.

 

[Dunfiddlin]

No, you're creating a new Random object for each Timer Tick event. Rather move the declaration of the Random variable to a private class field.

Oops, quite right. Hadn't noticed that it was in that sub.

 

[james_h]

No, you're creating a new Random object for each Timer Tick event. Rather move the declaration of the Random variable to a private class field.

Thanks for your guidance guys.

The suggestion for JohnH about the Timer Tick event, I am not sure what that means?

Dunfiddlin special thanks to you, I have implemented your suggestions and it has made the code much prettier.

Thanks.

 

[JohnH]

Private rnd As New Random

 

[james_h]

Hi once again Guys,

I think I almost have it right now

Revised code for the program as per suggestions!

Option Strict On

Public Class Form1




    Dim wins As Integer
    Dim Ten As Boolean = False
    Dim Twenty As Boolean = False


    Sub Win(ByVal Prize As String, ByVal Amount As Integer)


        RadioButton2.Checked = True
        PictureBox1.Hide()
        txtCoinsWon.Show()
        Me.BackgroundImage = My.Resources.SmileyDollar
        txtCoinsWon.Text = ("Congrats Dude!! You have won " & Prize & " !!")
        wins += Amount
        Label2.Text = CStr(wins)
        'If wins >= 10 then display a message
        If wins >= 10 AndAlso Ten = False Then
            Ten = True
                MessageBox.Show("You have 10 coins, you have won a new ball")
        End If


        'If wins >= 20 then display a message
        If wins >= 20 AndAlso Twenty = False Then
            Twenty = True
            MessageBox.Show("You have 20 coins, you have won a new ball")
        End If


    End Sub


    Private Sub Timer1_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Timer1.Tick


        'Set random position for the ball to be placed
        Dim rnd As New Random


        PictureBox1.Location = New Point(rnd.Next(1, 250), rnd.Next(60, 250))


    End Sub




    Private Sub RadioButton1_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton1.CheckedChanged


        'Check if timer needs to be started, hide textbox and background image
        If RadioButton1.Checked = True Then
            Label4.Visible = False
            txtCoinsWon.Hide()
            Me.BackgroundImage = Nothing
            PictureBox1.Show()
            'Display new ball if coins won match 10-19 or 20 and higher
            If wins >= 20 Then
                PictureBox1.BackgroundImage = My.Resources.BirdRed
            ElseIf wins >= 10 Then
                PictureBox1.BackgroundImage = My.Resources.FaceSmile
            End If
            Timer1.Start()
        End If


    End Sub


    Private Sub RadioButton2_CheckedChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles RadioButton2.CheckedChanged


        'Check if timer needs to be stopped and reset ball location
        If RadioButton2.Checked = True Then
            Timer1.Stop()
            PictureBox1.Location = New Point(165, 250)
        End If


    End Sub


    Private Sub ComboBox1_SelectedIndexChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles ComboBox1.SelectedIndexChanged


        'Set the timer speed depending on the users speed choice
        Dim speed As String


        speed = ComboBox1.Text


        Select Case speed
            Case "1"
                Timer1.Interval = 1000
            Case "2"
                Timer1.Interval = 750
            Case "3"
                Timer1.Interval = 650
            Case "4"
                Timer1.Interval = 500
            Case "5"
                Timer1.Interval = 350
        End Select


    End Sub


    Private Sub PictureBox1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles PictureBox1.Click


        'Check if radiobutton1 is checked and what speed game is running at
        'then award coins to suit by calling the win sub.
        If RadioButton1.Checked = True And Timer1.Interval = 1000 Then
            Win("1 coin", 1)
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 750 Then
            Win("2 coins", 2)
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 650 Then
            Win("3 coins", 3)
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 500 Then
            Win("4 coins", 4)
        ElseIf RadioButton1.Checked = True And Timer1.Interval = 350 Then
            Win("5 coins", 5)
        Else
            PictureBox1.Hide()
            Label4.Hide()
            txtCoinsWon.Show()
            Me.BackgroundImage = My.Resources.Skull2
            txtCoinsWon.Text = ("Hey the Game isn't running, Start the Game")
        End If


    End Sub


    Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load


        'Set combobox1 text to 1 as can not be set in properties as DropDownStyle is set to DropDownList
        'and will not allow me to set text property to a value in design view
        ComboBox1.Text = "1"


    End Sub
End Class

Please humor me a little longer with a couple of more questions!

Tell me if I understand this correctly?
The program checks if wins is >=10 and if Ten = False and if this is the case
then it changes Ten to True and displays the message, but if when it checks Ten is True
then it just skips the message and exits the if statement hence only ever displaying the
message one time?

Dim Ten As Boolean = False

'If wins >= 10 then display a message        
        If wins >= 10 AndAlso Ten = False Then
            Ten = True
                MessageBox.Show("You have 10 coins, you have won a new ball")
        End If

And last question!

Private rnd As New Random

I still do not understand what I am suppose to do here?
If I change Dim rnd As New Random to Private rnd as New Random I get an error?

 
   Private Sub Timer1_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Timer1.Tick

        'Set random position for the ball to be placed
        Dim rnd As New Random


        PictureBox1.Location = New Point(rnd.Next(1, 250), rnd.Next(60, 250))


    End Sub

Thanks again Guys you are a great help, I have learnt a lot with this so far.

 

[jmcilhinney]

If it's inside the method then it's executed every time the method's executed. If you don't want it executed every time the method's executed then don't put it in the method. Not in the method means outside the method.

 

[Solitaire]

RE: AndAlso:

If the first part of the statement is False then the entire statement must be False and it doesn't bother to test the second part of the statement. In order to test both parts of the statement no matter what, use And instead of AndAlso.


RE: Private vs. Dim:

Public and Private are used with form-level (class-level) variables and affect the declared variables inside all the subs. If you are declaring a local variable inside a sub, use Dim, which only affects the local variable inside that sub.