2
\$\begingroup\$

I'm learning VB.NET for work reasons, and I'd like to ask your help to improve my coding and my programming skills. What can I do better?

The program will be used to store data in the DB, like how many pencil, rubber, etc. You have to store and update these values according to buying and selling orders.

This is my SQLTools class:

Imports System.Data.SqlClient
Imports System.Data

Public Class SQLTools
    Inherits System.Windows.Forms.Form
    Private Const SqlString As String = "Server=*****;Database=*****;User ID=*****;Password=*****;Integrated Security=SSPI;"
    Private myConn As SqlConnection
    Private myCmd As SqlCommand
    Private myReader As SqlDataReader

    Private Sub Conn()
        myConn = New SqlConnection(SqlString)
    End Sub

    Public Function Connect() As SqlConnection
        myConn = New SqlConnection(SqlString)
        Return myConn
    End Function

    Public Function GetValDouble(ByVal query As String) As String
        On Error GoTo Handler

        //Setting and opening connection
        Conn()
        myConn.Open()

        //Get any Double value from DB
        Dim Res As Integer
        Dim I = 0
        myCmd = myConn.CreateCommand
        myCmd.CommandText = Trim(query)
        myReader = myCmd.ExecuteReader()
        Do While myReader.Read()
            Res = Convert.ToString(myReader.GetValue(I))
            I += 1
        Loop
        myReader.Close()
        myConn.Close()
        Return Res

    Handler:
        MsgBox("Qualcosa è andato storto :(")
        Err.Clear()
    End Function

    Public Function GetValStr(ByVal query As String) As String
        On Error GoTo Handler

        //Setting and opening connection
        Conn()
        myConn.Open()

        //Get any string value from DB
        Dim Res As Integer
        Dim I = 0
        myCmd = myConn.CreateCommand
        myCmd.CommandText = Trim(query)
        myReader = myCmd.ExecuteReader()
        Do While myReader.Read()
            Res = Convert.ToString(myReader.GetValue(I))
            I += 1
        Loop
        myReader.Close()
        myConn.Close()
        Return Res

    Handler:
        MsgBox("Qualcosa è andato storto :(")
        Err.Clear()
End Function

Public Function GetValInt(ByVal query As String) As Integer
    On Error GoTo Handler

    //Setting and opening connection
    Conn()
    myConn.Open()

    //Get any int value from DB
    Dim Res As Integer
    Dim I = 0
    myCmd = myConn.CreateCommand
    myCmd.CommandText = Trim(query)
    myReader = myCmd.ExecuteReader()
    Do While myReader.Read()
        Res = Convert.ToInt16(myReader.GetValue(I))
        I += 1
    Loop
    myReader.Close()
    myConn.Close()
    Return Res

    Handler:
         MsgBox("Qualcosa è andato storto :(")
         Err.Clear()
End Function

Public Sub Delete(ByVal Table As String)
    //Opening connection
    Conn()
    myConn.Open()

    //Clearing transactions History
    Dim Sql As String = "Delete from " + Table
    myCmd = New SqlCommand(Sql, myConn)
    myCmd.ExecuteNonQuery()
    MsgBox("Cronologia ordini canellata.")
    myConn.Close()

End Sub

End Class

Insert Sell Order:

Imports System.Data.SqlClient
Imports System.Data

Public Class INS_Sell
    Inherits System.Windows.Forms.Form
    Private myConn As SqlConnection
    Private myCmd As SqlCommand
    Private myReader As SqlDataReader
    Dim Tool As New SQLTools()

Private Sub CLS_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles CLS.Click, CLS.Click
    Dim M As New Main
    M.Show()
    Me.Hide()
End Sub

Private Sub Exec_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Exec.Click
    On Error GoTo Handler

    //Opening DB connection
    Tool.Connect()
    myConn.Open()

    //Extrapolating value from textbox
    Dim Quant As Integer = Convert.ToInt16(TextBox1.Text)

    //Setting Quant to -Quant in order to sell Articles
    Quant -= Quant * 2

    //Selecting Articles from ComboBox
    Dim Articolo As String = ComboBox1.SelectedItem
    Dim Art As Integer
    Select Case Articolo
        Case "Quaderno"
            Art = 1
        Case "Penna"
            Art = 2
        Case "Matita"
            Art = 3
        Case "Gomma"
            Art = 4
    End Select

    //Quantity Checking for the choosen article
    If Tool.GetValInt("Select Quant from Magazzino where ID_Art=" + Convert.ToString(Art)) + Quant <= 0 Then
        MsgBox("Quantità insufficente, anullo ordine.")
        Exit Sub
    End If

    //Updating transactions chronology
    Dim Sql As String = "insert into Movimento values(@Art,@Quant)"
    myCmd = New SqlCommand(Sql, myConn)
    myCmd.Parameters.Add("@Art", SqlDbType.Int).Value = Art
    myCmd.Parameters.Add("@Quant", SqlDbType.Int).Value = Quant
    myCmd.ExecuteNonQuery()

    //Quantity Check for the transaction
    Quant = Tool.GetValInt("Select Quant from Magazzino where ID_Art=" + Convert.ToString(Art)) + Quant
    If Quant <= 0 Then
        Quant = 0

    //Autobuy article        
    ElseIf Quant <= 10 Then
        Quant += 20
        Sql = "Insert into Movimento values(@Art, 20)"
        myCmd = New SqlCommand(Sql, myConn)
        myCmd.Parameters.Add("@Art", SqlDbType.Int).Value = Art
        myCmd.ExecuteNonQuery()
        MsgBox("Quantità articolo sotto la soglia minima, acquistati 20 pezzi.")
    End If

    //Warehouse updating
    Sql = "update Magazzino set Quant = @Quant Where ID_Art = @Art"
    myCmd = New SqlCommand(Sql, myConn)
    myCmd.Parameters.Add("@Art", SqlDbType.Int).Value = Art
    myCmd.Parameters.Add("@QUant", SqlDbType.Int).Value = Quant
    myCmd.ExecuteNonQuery()
    MsgBox("Modifica avvenuta con successo!")
    myConn.Close()
    Exit Sub

    Handler:
        MsgBox("Qualcosa è andato storto :(")
        TextBox1.Text = ""
    End Sub

    Private Sub QRY_FormClosing(ByVal sender As Object, ByVal e As System.Windows.Forms.FormClosingEventArgs) Handles Me.FormClosing
        Application.Exit()
    End Sub

    Private Sub Form2_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
        On Error GoTo Handler
        Dim Res As String = ""

     //Opening DB connection
        myConn = Tool.Connect()
        myConn.Open()

    //Populating Articles ComboBox
        myCmd = myConn.CreateCommand
        myCmd.CommandText = "Select Articolo From Magazzino"
        myReader = myCmd.ExecuteReader()
        Do While myReader.Read()
            For I = 0 To myReader.FieldCount() - 1
                Res = myReader.GetString(I)
                ComboBox1.Items.Add(Res)
            Next
        Loop
        myConn.Close()
        myReader.Close()
        Res = ""
        Exit Sub

    Handler:
        MsgBox("Impossibile recuperare i dati dal magazzino.")
        myConn.Close()
        Res = ""
        Err.Clear()

    End Sub
End Class

Insert Buy Order:

Imports System.Data.SqlClient
Imports System.Data

Public Class INS_Buy
    Inherits System.Windows.Forms.Form
    Private myConn As SqlConnection
    Private myCmd As SqlCommand
    Private myReader As SqlDataReader
    Dim Tool As New SQLTools()

Private Sub CLS_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles CLS.Click, CLS.Click
    Dim M As New Main
    M.Show()
    Me.Hide()
End Sub

Private Sub Exec_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Exec.Click
    On Error GoTo Handler

    //Opening DB connection
    myConn = Tool.Connect()
    myConn.Open()

    //Extrapolating value from textbox
    Dim Quant As Integer = Convert.ToInt16(TextBox1.Text)

    //Selecting Articles from ComboBox
    Dim Articolo As String = ComboBox1.SelectedItem
    Dim Art As Integer
    Select Case Articolo
        Case "Quaderno"
            Art = 1
        Case "Penna"
            Art = 2
        Case "Matita"
            Art = 3
        Case "Gomma"
            Art = 4
    End Select

    //Updating transactions chronology
    Dim Sql As String = "insert into Movimento values(@Art,@Quant)"
    myCmd = New SqlCommand(Sql, myConn)
    myCmd.Parameters.Add("@Art", SqlDbType.Int).Value = Art
    myCmd.Parameters.Add("@Quant", SqlDbType.Int).Value = Quant
    myCmd.ExecuteNonQuery()

    //Warehouse updating
    Sql = "update Magazzino set Quant = @Quant Where ID_Art = @Art"
    myCmd = New SqlCommand(Sql, myConn)
    myCmd.Parameters.Add("@Art", SqlDbType.Int).Value = Art
    myCmd.Parameters.Add("@QUant", SqlDbType.Int).Value = Tool.GetValInt("Select Quant from Magazzino where ID_Art=" + Convert.ToString(Art)) + Quant
    myCmd.ExecuteNonQuery()
    MsgBox("Modifica avvenuta con successo!")
    myConn.Close()
    Exit Sub

    Handler:
        MsgBox("Qualcosa è andato storto :(")
        TextBox1.Text = ""
    End Sub

Private Sub QRY_FormClosing(ByVal sender As Object, ByVal e As System.Windows.Forms.FormClosingEventArgs) Handles Me.FormClosing
    Application.Exit()
End Sub

Private Sub Form2_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
    On Error GoTo Handler
    Dim Res As String = ""

    //Opening DB connection
    myConn = Tool.Connect()
    myConn.Open()

    //Populating Articles ComboBox
    myCmd = myConn.CreateCommand
    myCmd.CommandText = "Select Articolo From Magazzino"
    myReader = myCmd.ExecuteReader()
    Do While myReader.Read()
        For I = 0 To myReader.FieldCount() - 1
            Res = myReader.GetString(I)
            ComboBox1.Items.Add(Res)
        Next
    Loop
    myConn.Close()
    myReader.Close()
    Res = ""
    Exit Sub

    Handler:
        MsgBox("Impossibile recuperare i dati dal magazzino.")
        myConn.Close()
        Res = ""
        Err.Clear()
    End Sub
End Class

SQLTools

Insert Buy Order

Insert Sell Order

\$\endgroup\$
4
\$\begingroup\$

so first thing I would say to you is

  1. Don't use Goto's

Second thing

  1. Don't swallow Exceptions

You also are using C# comment indicators when you should be using an apostrophe


to illustrate some of what I am saying I grabbed your Form2 Load Sub from the Insert and got rid of the goto exception handling. I also put in 2 using statements that will automatically dispose of the Connection and the Reader upon leaving scope of the Using block.

Check it out:

Yours

Private Sub Form2_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
    On Error GoTo Handler
    Dim Res As String = ""

 //Opening DB connection
    myConn = Tool.Connect()
    myConn.Open()

//Populating Articles ComboBox
    myCmd = myConn.CreateCommand
    myCmd.CommandText = "Select Articolo From Magazzino"
    myReader = myCmd.ExecuteReader()
    Do While myReader.Read()
        For I = 0 To myReader.FieldCount() - 1
            Res = myReader.GetString(I)
            ComboBox1.Items.Add(Res)
        Next
    Loop
    myConn.Close()
    myReader.Close()
    Res = ""
    Exit Sub

Handler:
    MsgBox("Impossibile recuperare i dati dal magazzino.")
    myConn.Close()
    Res = ""
    Err.Clear()

End Sub

Mine

Private Sub From1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
    Dim res As String = ""
    Using myConn As SqlConnection = Tool.Connect()
        myConn.Open()
        myCmd = myConn.CreateCommand
        myCmd.CommandText = "Select Articolo From Magazzino"
        Using myReader As SqlDataReader = myCmd.ExecuteReader()
            Do While myReader.Read()
                For I = 0 To myReader.FieldCount() - 1
                    res = myReader.GetString(I)
                    ComboBox1.Items.Add(res)
                Next
            Loop
        End Using
    End Using
End Sub

for doing your Error Handling. if you have to catch the exceptions and handle them the way you were, you should use a try-catch-finally block. like this

    Try
        Dim res As String = ""
        Using myConn As SqlConnection = Tool.Connect()
            myConn.Open()
            myCmd = myConn.CreateCommand
            myCmd.CommandText = "Select Articolo From Magazzino"
            Using myReader As SqlDataReader = myCmd.ExecuteReader()
                Do While myReader.Read()
                    For I = 0 To myReader.FieldCount() - 1
                        res = myReader.GetString(I)
                        ComboBox1.Items.Add(res)
                    Next
                Loop
            End Using
        End Using
    Catch ex As Exception
        MsgBox("Impossibile recuperare i dati dal magazzino.")
    Finally
        Res = ""
        Err.Clear()
    End Try
End Sub

not sure why you would need to set that Res variable to an empty string, it will be Garbage Collected after the scope leaves this Sub regardless.

Notice how I didn't close the connection or the reader, this is because the Using blocks are actually Try Finally statements where in the Finally statement they call the IDispose's dispose method for the resource that was being used. so the connection is disposed once the scope leaves that block, same with the reader.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for your advise, I will update my code asap. I used C# comment indicator because on codereview apostrophe isn't detected as comments indicator. I did it just for an easier reading of the code, i'm actually using apostrophe in my program. \$\endgroup\$ – Simo Mar 21 '18 at 8:00
  • \$\begingroup\$ SE uses a third party formatting library, and for some reason the VB comments have to be closed, so you have to have an apostrophe at the end of the comment line as well, so it acts as a multi-line comment. \$\endgroup\$ – Malachi Mar 21 '18 at 13:17
1
\$\begingroup\$

Error Handling and Using blocks have already been covered by @Malachi. My comments are inline.

Option Strict On
'Always and forever set Option Strict On
'This can be set for all your VB projects
Imports System.Data.SqlClient
    Imports System.Data

    Public Class SQLTools
    'Inherits System.Windows.Forms.Form
    'Don't inherit form Windows.Forms - this is Not a Form, it is a helper class
    Private Const SqlString As String = "Server=*****;Database=*****;User ID=*****;Password=*****;Integrated Security=SSPI;"
        Private myConn As SqlConnection
        Private myCmd As SqlCommand
        Private myReader As SqlDataReader

    Private Sub Conn()
            myConn = New SqlConnection(SqlString)
        End Sub

    Public Function GetConnectionObject() As SqlConnection
        myConn = New SqlConnection(SqlString)
        Return myConn
    End Function
    'Your Function is called GetValDouble but it returns a String
    'this is very misleading to anyone trying to use this class
    Public Function GetValDouble(ByVal query As String) As String
        'Your use of GoTo has already been covered
        On Error GoTo Handler

        'Setting And opening connection
        Conn() 'bad name for a Sub - What does it do?
        'How about calling it CreateNewConnection
        myConn.Open()

        'Get any Double value from DB
        Dim Res As Integer 'Your function is defined to return an String 
        'but you are trying to return an Integer - won't compile
        Dim I = 0
        myCmd = myConn.CreateCommand
        myCmd.CommandText = Trim(query)
        myReader = myCmd.ExecuteReader()
        'Suppose the passed in query returns 100 records
        'I is being incremented on each iteration
        'the .GetValue(I) is trying to read the column(I) in the return
        'Are you returning 100 columns?
        Do While myReader.Read()
            'Now you are trying to assign a string to the variable
            'that you just declared as an integer, yet your Function
            'title is promising a Double!
            Res = Convert.ToString(myReader.GetValue(I))
            'Assuming the assignment could be made, you are overwritting the
            'the value of Res on each iteration
            I += 1
        Loop
        myReader.Close()
        myConn.Close()
        Return Res

Handler:
        'Don't communicate with the user from a helper class
        'Let the error go up the call stack until you handle it in a UI class (a Form)
        MsgBox("Qualcosa è andato storto :(")
        Err.Clear()
    End Function
\$\endgroup\$
  • \$\begingroup\$ Thanks Mary! My way of coding is changed in the last months. I took in consideration alot of advices you all gave me here and I'm pretty amazed how i changed from March to August. Thanks you all! \$\endgroup\$ – Simo Aug 3 '18 at 9:12

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.