[Essel]

I am trying to login with this code which is working alright but the problem is anytime i make a mistake in trying to login with a wrong password is gives me an exception that the connection was not close but it is open i have try to fix it but to no avail . i need hlep please

Private Sub OK_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles OK.Click
        Try
            connection.Open()
            If Me.PasswordTextBox.Text.Length < 6 Then
                MsgBox("Password too short", MsgBoxStyle.Critical, "Error")
                Me.PasswordTextBox.SelectAll()
                Me.PasswordTextBox.Focus()
                Exit Sub
            End If
            Dim cmd As New SqlCommand("Select * From Account where Password='" & Me.PasswordTextBox.Text & "' and UserName= '" & Me.UsernameTextBox.Text & "'", connection)
            Dim ReadLogin As SqlDataReader = cmd.ExecuteReader

            If ReadLogin.HasRows Then
                Me.Hide()
                Dim Main As New MainForm
                Main.Show()

            Else
                MsgBox("Error logging in to the sytem.Please check username and password", MsgBoxStyle.Critical, "Cannot login")

                connection.Close()
                cmd.Dispose()
                ReadLogin.Close()
            End If
                catch ex As Exception
            MessageBox.Show("An Error Occured,Restart Application and Login Again", "Restart Application")
            Me.Close()
        End Try
    End Sub

This post has been edited by AdamSpeight2008: 10 June 2010 - 10:04 AM
Reason for edit:: Corrected Code Tags

[CharlieMay]

You don't have a connection.close in your code and a you're trying to open it again somewhere.

You close it if there is an error, but you don't close it if everything was successful.

This post has been edited by CharlieMay: 10 June 2010 - 10:56 AM

[demausdauth]

First off, try not to use Exit Sub, it's not always bad, but it does indicate spaghetti code.
Good use of Try...Catch, but Finally helped out also

Private Sub OK_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles OK.Click
        'Good to use the Try..Catch
        ' but make use of the Finally also
        Dim MessageString As String = String.Empty
        Try
            

            If Me.PasswordTextBox.Text.Length < 6 Then
                'Password fails first test 

                ' Set the message value, select and focus
                Me.PasswordTextBox.SelectAll()
                Me.PasswordTextBox.Focus()

                'do not give them to much information about how the password 
                ' is wrong, just makes the illegitmate user aware of how to 
                ' hack it
                MessageString = "Password is invalid."

            Else
                'Password passes first test
                'This is prone to SQL injection techniques 
                ' Use of sql parameters is a better option
                'Dim cmd As New SqlCommand("Select * From Account where Password='" & Me.PasswordTextBox.Text & "' and UserName= '" & Me.UsernameTextBox.Text & "'", connection)

                'Using statement will automatically close and dispose of the command object when it reaches the 'end using' even if it produces an error
                Using cmd As New SqlCommand("SELECT * FROM Account WHERE Password=@PassWord AND UserName=@UserName", connection)

                    'now add the sql parameters

                    'username parameter
                    cmd.Parameters.AddWithValue("@UserName", Me.UserName.Text)

                    'password parameter
                    cmd.Parameters.AddWithValue("@PassWord", Me.PasswordTextBox.Text)

                    'I would check to make sure you have an open connection before just opening it
                    ' also the connection can wait to be opened until just before you need to use it
                    If Not connection.State = ConnectionState.Open Then connection.Open()

                    Dim ReadLogin As SqlDataReader = cmd.ExecuteReader()

                    If ReadLogin.HasRows Then
                        Me.Hide()
                        Dim Main As New MainForm
                        Main.Show()

                    Else
                        MessageString = "Error logging in to the sytem.Please check username and password"

                    End If 'end ReadLogin has rows

                    ReadLogin.Close()

                End Using 'end using cmd
            End If 'end first password test
            
        Catch ex As Exception
            MessageString = "An Error Occured,Restart Application and Login Again"

        Finally
            connection.Close()
        End Try

        'if we set the messagestring variable to a value then we know we need to display a message
        ' the MsgBox method of displaying messages is outdated, also when you look behind the scenes it
        ' makes a call to the MessageBox class passing all it's information along, so skip the middle man
        ' and go right to the MessageBox.
        If Not String.IsNullOrEmpty(MessageString) Then
            MessageBox.Show(MessageString, "Login Failed", MessageBoxButtons.OK, MessageBoxIcon.Error)

            'Now there was one instance where you wanted to actually close the application, here is where we 
            ' decide that. Since each of the 3 error messages starts with a unique set of characters we can use those
            ' to determine it
            If MessageString.StartsWith("An") Then
                Application.Exit()
            End If

            '' Alternatively you could just disable the login button and textboxes and keep only the/a Exit button
            '' enabled - in otherwords give them no choice
            'Me.OK.Enabled = False
            'Me.bntExit.Enabled = True
            'Me.UserName.Enabled = False
            'Me.PasswordTextBox.Enabled = False

        End If

    End Sub

[raziel_]

how so exit sub indicate spaghetti code? dose return in C indicate the same thing then? i always think that spaghetti code use labels.

[demausdauth]

By C do you mean C#, or comparative language.

It's a void method -- equivalent of a Sub Procedure in VB.Net. You shouldn't be 'returning' anything from that type of method.

And perhaps I was a little aggressive of what I think of Exit Sub. Company I used to work for the standard method of processing in a sub was to just exit when you felt like it. So a particular method (literally 1500+ lines long) had 27 Exit Subs at various places. That one was hell to debug when there was a problem.

So I guess it is more of my aversion than indicative of spaghetti code, although I thought I once read an article that did allude to that as well.