5 Replies - 287 Views - Last Post: 30 January 2019 - 09:30 AM Rate Topic: -----

#1 nyt1972   User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 98
  • Joined: 04-February 10

More simplified and improved code

Posted 29 January 2019 - 06:39 AM

Hello,

I want you to check my code and improve or suggest how can I make this code more simplified and improved if possible?

The Entry Form is to Insert new Session, or Update/Existing existing, but if there is already a Session marked as current, do Check for Current before insert, delete or update.

My table tblSession:
ID   SessionName   is_current
 1   2014-2015        0
 2   2015-2016        0
 3   2016-2017        0
 4   2017-2018        1


Connection Class is:
Imports System.Data.SqlClient
Imports System.IO

Module SqlConn
    Public sqL As String
    Public ds As New DataSet()
    Public cmd As SqlCommand = Nothing
    Public dr As SqlDataReader = Nothing
    Public drlogin As SqlDataReader = Nothing
    Public da As New SqlDataAdapter
    Public dt As DataTable
    Public dt2 As DataTable
    Public result As Integer
    Public conn As SqlConnection

    Public Sub dbConnection()
        Try
            conn = New SqlConnection
            conn.ConnectionString = "Data Source=TAREEN-PC\SQLEXPRESS;Initial Catalog=AnsiSchoolDB2;Integrated Security=True;Connect Timeout=15;Encrypt=False;TrustServerCertificate=True;ApplicationIntent=ReadWrite;MultiSubnetFailover=False;"
            conn.Open()
        Catch ex As SqlException
            MsgBox(ex.Message)
        End Try
    End Sub
End Module



And the Insert, Update, Delete Form's Code is below:

Imports System.Data.SqlClient

Public Class frmSession
    Private Sub btnNew_Click(sender As Object, e As EventArgs) Handles btnNew.Click
        Reset()
    End Sub

    Sub Reset()
        txtSession.ReadOnly = False
        txtSession.Text = ""
        btnSave.Enabled = True
        btnDelete.Enabled = False
        btnUpdate.Enabled = False
        Chk_iscurrent.CheckState = False
        txtSession.Focus()
    End Sub

    Private Sub btnSave_Click(sender As Object, e As EventArgs) Handles btnSave.Click
        If Chk_iscurrent.Checked = True Then
            If checkCurrentSession() = True Then
                Dim chk As Integer = MessageBox.Show("Already Current Session exists, Want to make this current?", "Error", MessageBoxButtons.YesNo, MessageBoxIcon.Warning)
                If chk = DialogResult.Yes Then
                    UnCurrentSession()
                    savesession()
                Else
                    Chk_iscurrent.Checked = False
                    savesession()
                End If
            Else
                savesession()
            End If
        Else
            savesession()
        End If

    End Sub

    Private Sub UnCurrentSession()
        Try
            dbConnection()
            Dim id As Int32
            sqL = "SELECT SessionID from tblsession WHERE is_current=1;"
            cmd = New SqlCommand(sqL, conn)
            Using dr = cmd.ExecuteReader()
                While dr.Read
                    If dr.HasRows() Then
                        id = dr.GetInt32(0)
                        MsgBox(id.ToString)
                    End If
                End While
            End Using
            cmd = New SqlCommand
            sqL = "UPDATE tblsession SET is_current=0 WHERE [email protected];"
            With cmd
                .Connection = conn
                .CommandText = sqL
                .Parameters.Clear()
                .Parameters.AddWithValue("@d1", id)
                result = .ExecuteNonQuery()
                If result = 0 Then
                    MsgBox("Error in updating current session!")
                Else
                    ' Do nothing
                End If
            End With


        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            da.Dispose()
            cmd.Dispose()
        End Try
    End Sub

    Private Sub savesession()
        Try
            If txtSession.Text = "" Then
                MessageBox.Show("Please enter Session", "", MessageBoxButtons.OK, MessageBoxIcon.Warning)
                txtSession.Focus()
                Return
            End If
            If CheckifExists() = False Then
                dbConnection()
                sqL = "insert into tblSession(SessionName,is_current) VALUES (@d1,@d2);"
                cmd = New SqlCommand
                With cmd
                    .Connection = conn
                    .CommandText = sqL
                    .Parameters.Clear()
                    .Parameters.AddWithValue("@d1", txtSession.Text)
                    .Parameters.AddWithValue("@d2", Chk_iscurrent.CheckState)
                    result = .ExecuteNonQuery()
                    If result = 0 Then
                        MsgBox("Error in adding new product!")
                    Else
                        MessageBox.Show("Session added Successfully", "Added", MessageBoxButtons.OK, MessageBoxIcon.[Information])
                    End If
                End With
            Else
                MessageBox.Show("Session Already Exists", "Error", MessageBoxButtons.OK, MessageBoxIcon.[Error])
            End If
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
            txtSession.Text = ""
            txtSession.Focus()
            Reset()
            getData()
        End Try
    End Sub

    Private Function CheckifExists() As Boolean
        Try
            dbConnection()
            sqL = "select SessionName from tblSession where [email protected]"
            cmd = New SqlCommand(sqL, conn)
            cmd.Parameters.AddWithValue("@d1", txtSession.Text)
            Using dr = cmd.ExecuteReader()
                While dr.Read()
                    If dr.HasRows Then
                        Return True
                        ' Current Found
                    Else
                        Return False
                        ' Current Not Found
                    End If
                End While
            End Using
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
        End Try
    End Function

    Private Sub getData()
        Try
            dbConnection()
            sqL = "SELECT SessionID,RTRIM(SessionName) as SessionName,is_current from tblSession order by SessionName;"
            cmd = New SqlCommand
            With cmd
                .Connection = conn
                .CommandText = sqL
                .Parameters.Clear()
                .ExecuteNonQuery()
            End With
            dt = New DataTable
            da = New SqlDataAdapter
            da.SelectCommand = cmd
            da.Fill(dt)
            dgw.DataSource = dt
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
        End Try
    End Sub

    Private Sub frmSession_Load(sender As Object, e As EventArgs) Handles MyBase.Load
        getData()
    End Sub

    Private Sub btnClose_Click(sender As Object, e As EventArgs) Handles btnClose.Click
        Me.Close()
    End Sub

    Private Sub btnUpdate_Click(sender As Object, e As EventArgs) Handles btnUpdate.Click
        If Chk_iscurrent.Checked = True Then
            If checkCurrentSession() = True Then
                Dim chk As Integer = MessageBox.Show("Already Current Session exists, Want to make this current?", "Error", MessageBoxButtons.YesNo, MessageBoxIcon.Warning)
                If chk = DialogResult.Yes Then
                    UnCurrentSession()
                    doUpdate()
                Else
                    Chk_iscurrent.Checked = False
                    doUpdate()
                End If
            Else
                doUpdate()
            End If
        Else
            doUpdate()
        End If

    End Sub
    Private Function checkCurrentSession() As Boolean
        Try
            dbConnection()
            sqL = "SELECT * from tblsession WHERE is_current=1;"
            cmd = New SqlCommand(sqL, conn)
            Using dr = cmd.ExecuteReader()
                While dr.Read()
                    If dr.HasRows Then
                        Return True
                        ' Current Found
                    Else
                        Return False
                        ' Current Not Found
                    End If
                End While
            End Using
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
        End Try
    End Function

    Private Sub doUpdate()
        Try
            dbConnection()
            sqL = "UPDATE tblsession SET [email protected], [email protected] WHERE [email protected];"
            cmd = New SqlCommand
            With cmd
                .Connection = conn
                .CommandText = sqL
                .Parameters.Clear()
                cmd.Parameters.AddWithValue("@d0", txtSessionID.Text)
                cmd.Parameters.AddWithValue("@d1", txtSession.Text)
                cmd.Parameters.AddWithValue("@d2", Convert.ToBoolean(Chk_iscurrent.CheckState))
                result = .ExecuteNonQuery()
                If result = 0 Then
                    MsgBox("Error in updating product!")
                Else
                    MsgBox("Successfully updated the selected product!")
                End If
            End With
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
            btnUpdate.Enabled = False
            txtSession.ReadOnly = True
            getData()
        End Try
    End Sub

    Private Sub dgw_MouseClick(sender As Object, e As MouseEventArgs) Handles dgw.MouseClick
        If dgw.Rows.Count > 0 Then
            Dim dr As DataGridViewRow = dgw.SelectedRows(0)
            txtSession.ReadOnly = False
            txtSessionID.Text = dr.Cells(0).Value.ToString()
            txtSession.Text = dr.Cells(1).Value.ToString()
            Chk_iscurrent.Checked = Convert.ToBoolean(dr.Cells(2).Value)
            btnUpdate.Enabled = True
            btnDelete.Enabled = True
            btnSave.Enabled = False
        End If
    End Sub

    Private Sub btnDelete_Click(sender As Object, e As EventArgs) Handles btnDelete.Click
        Try
            dbConnection()
            sqL = "SELECT * from tblsession WHERE [email protected] and is_current=1;"
            cmd = New SqlCommand
            With cmd
                .Connection = conn
                .CommandText = sqL
                .Parameters.Clear()
                .Parameters.AddWithValue("@d0", txtSessionID.Text)
                dr = .ExecuteReader()
                If dr.HasRows() Then
                    MessageBox.Show("Cannot delete Current Session?", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error)
                    Reset()
                    Return
                Else
                    Dim chk As Integer = MessageBox.Show("Are you sure to delete Session?", "Delete", MessageBoxButtons.YesNo, MessageBoxIcon.Warning)
                    If chk = DialogResult.Yes Then
                        DeleteSession()
                        Reset()
                    Else
                        Exit Sub
                    End If
                End If
            End With
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
        End Try

    End Sub

    Private Sub DeleteSession()
        Try
            dbConnection()
            cmd = New SqlCommand
            sqL = "Delete from tblsession WHERE [email protected];"
            With cmd
                .Connection = conn
                .CommandText = sqL
                .Parameters.Clear()
                result = .ExecuteNonQuery()
                If result = 0 Then
                    MsgBox("Error in deleting session!")
                Else
                    MsgBox("Successfully deleted the selected session!")
                End If
            End With
        Catch ex As SqlException
            MsgBox(ex.Message)
        Finally
            conn.Close()
            cmd.Dispose()
            btnUpdate.Enabled = False
            btnDelete.Enabled = False
            txtSession.ReadOnly = True
            getData()
        End Try

    End Sub
End Class



Please help.
Thanks in advance.

This post has been edited by nyt1972: 29 January 2019 - 07:25 AM


Is This A Good Question/Topic? 0
  • +

Replies To: More simplified and improved code

#2 modi123_1   User is online

  • Suitor #2
  • member icon



Reputation: 14854
  • View blog
  • Posts: 59,272
  • Joined: 12-June 08

Re: More simplified and improved code

Posted 29 January 2019 - 07:54 AM

Kudos for using parameters.

Not a fan of that.. what ever sql connection class is... the random readers, adapters, tables, etc. If you want to separate out your connection information then do so and have it return a collection of data as needed.

Not a fan of using GUI objects as any given form's data holders. GUI should represent only.

Your 'btnSave_Click' has 'save session' too many times to be efficient.

I am not understanding why 'school session' is this big of a deal? I would think a simple date check would be fine.

Use 'with no locks' on your selects.

Your parameter names are poorly done as d0, d1, etc.
Was This Post Helpful? 1
  • +
  • -

#3 nyt1972   User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 98
  • Joined: 04-February 10

Re: More simplified and improved code

Posted 29 January 2019 - 09:52 AM

Thanks modi123_1
Was This Post Helpful? 0
  • +
  • -

#4 Sheepings   User is offline

  • Senior Programmer
  • member icon

Reputation: 201
  • View blog
  • Posts: 1,125
  • Joined: 05-December 13

Re: More simplified and improved code

Posted 30 January 2019 - 06:31 AM

Not a good idea putting getData() in your load event, if this check takes a long time to process, for any reason, it will slow your GUI down in your MyBase.Load. Instead use form shown. Also, not a good idea to run stuff like this in the load event, as some errors may even get swallowed without you knowing something has gone wrong. Instead of using using blocks on ::
dr = cmd.ExecuteReader()
You should be putting them on your connection string
Using connection As New SqlConnection(cString)
and then enclose your dr inside those blocks. You can also do the same for commands as well, as there are multiple approaches you can use to write these out. ::
Using cmd As SqlCommand = New SqlCommand(Statement, Con)
Your dbConnection method could do with some work... Maybe do something like this instead ::
        Public Shared Function SetConnection() As Boolean
            Select Case Con.State = ConnectionState.Closed
                Case True
                    Con.Open()
                    Return True
                Case False
                    Con.Close()
                    Return False
            End Select
            Return False
        End Function
Then you won't need to call these all the time in your finally section, after you make the alterations. ::
            Finally
            conn.Close()
            cmd.Dispose()
I'd also recommend checking that your connection is not nothing :: if Con IsNot Nothing Continue. I believe if you can question the way you done things, then you haven't done them well enough. Improve improve improve...

Well played though on using parameters, and using blocks, keep plugging away at it, there is always room for improvement.
Was This Post Helpful? 1
  • +
  • -

#5 Sheepings   User is offline

  • Senior Programmer
  • member icon

Reputation: 201
  • View blog
  • Posts: 1,125
  • Joined: 05-December 13

Re: More simplified and improved code

Posted 30 January 2019 - 06:39 AM

Just to elaborate a little further... to be honest, even putting getData anywhere in your GUI related methods is actually bad practice, but if you want simplistic, then what I advised will work fine, providing they don't lock up your UI. But appropriately you'd be best kicking that getData call off on a new thread. Do you know anything about multithreading? Because that would actually be more appropriate, as your GUI should only be used to reflect the data it has received, and that's best done on non-UI threads.

This post has been edited by Sheepings: 30 January 2019 - 06:40 AM

Was This Post Helpful? 1
  • +
  • -

#6 nyt1972   User is offline

  • D.I.C Head

Reputation: 0
  • View blog
  • Posts: 98
  • Joined: 04-February 10

Re: More simplified and improved code

Posted 30 January 2019 - 09:30 AM

Thanks very much Sheepings.
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1