Comments requested by experienced programmers

S

spebola

Regular
Joined
Mar 9, 2003
Messages
50
Location
Oklahoma City
I am a newcomer to the PC, Visual Basic and .Net but not to programming(AS/400). I would appreciate any constructive criticism on the following code before I go any further on this application. The code displays a form with a tool bar, menus, text box and list view. I read 25 rows from a table and display in the list view (25 is an arbitrary limit to reduce network traffic). The user can reposition list view contents is several ways, and select an entry for display, edit, copy or add a new entry. The GetData function is in a dll and returns a datareader based on the command passed.

Visual Basic: 
Dim sFac As String = ""
    Dim arrFac(500)
    Dim pagNo As Integer = 0
    Dim drFac As IDataReader
    Public Shared mMode As String
    Public Shared mFac As String
    Dim oErr As Exception

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

        tbrList.Buttons(1).Enabled = False
        tbrList.Buttons(3).Enabled = False
        tbrList.Buttons(5).Enabled = False
        tbrList.Buttons(6).Enabled = False
        mnuFileDisplay.Enabled = False
        mnuFileEdit.Enabled = False
        mnuFileCopy.Enabled = False
        mnuPopDisplay.Enabled = False
        mnuPopEdit.Enabled = False
        mnuPopCopy.Enabled = False
        arrFac(0) = sFac
        loadlistview()
    End Sub

    Private Sub loadlistview()
        Dim ix As Integer
        Dim cmd As String

        Try


            cmd = "SELECT * FROM Fac WHERE Fac_Code >= '" & sFac & "' ORDER BY Fac_Code"
            drFac = GetData(cmd)
        lvwFac.BeginUpdate()
        lvwFac.Items.Clear()
        ix = 0
        With drFac
            While .Read And ix < 25
                sFac = .GetString(0)
                Dim lvi As New ListViewItem(.GetString(0), 0)
                lvwFac.Items.Add(lvi)
                lvi.SubItems.Add(.GetString(1))
                lvi.SubItems.Add(.GetString(6))
                lvi.SubItems.Add(.GetString(7))
                ix = ix + 1
            End While
        End With
        lvwFac.EndUpdate()
        pagNo = pagNo + 1
        arrFac(pagNo) = sFac
            drFac.Close()

        Catch oErr As Exception
            MessageBox.Show("Error: FacList - " & oErr.Message)
            If Not drFac.IsClosed Then
                drFac.Close()
            End If
            Me.Close()
        End Try
    End Sub

    Private Sub lvwFac_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles lvwFac.Click
        mFac = lvwFac.FocusedItem.Text
        tbrList.Buttons(3).Enabled = True
        tbrList.Buttons(5).Enabled = True
        tbrList.Buttons(6).Enabled = True
        mnuFileDisplay.Enabled = True
        mnuFileEdit.Enabled = True
        mnuFileCopy.Enabled = True
        mnuPopDisplay.Enabled = True
        mnuPopEdit.Enabled = True
        mnuPopCopy.Enabled = True
    End Sub

    Private Sub lvwFac_Leave(ByVal sender As Object, ByVal e As System.EventArgs) Handles lvwFac.Leave
        tbrList.Buttons(3).Enabled = False
        tbrList.Buttons(5).Enabled = False
        tbrList.Buttons(6).Enabled = False
        mnuFileDisplay.Enabled = False
        mnuFileEdit.Enabled = False
        mnuFileCopy.Enabled = False
        mnuPopDisplay.Enabled = False
        mnuPopEdit.Enabled = False
        mnuPopCopy.Enabled = False
    End Sub

    Private Sub txtSFac_Leave(ByVal sender As Object, ByVal e As System.EventArgs) Handles txtSFac.Leave
        sFac = txtSFac.Text
        arrFac(0) = sFac
        pagNo = 0
        loadlistview()
    End Sub

    Public Sub PageDown()
        sFac = arrFac(pagNo)
        tbrList.Buttons(1).Enabled = True
        loadlistview()
    End Sub

    Public Sub PageUp()
        pagNo = pagNo - 2
        If pagNo <= 0 Then
            pagNo = 0
            tbrList.Buttons(1).Enabled = False
        End If
        sFac = arrFac(pagNo)
        loadlistview()
    End Sub

    Public Sub DisplayDetails()
        mMode = "Dsp "

    End Sub

    Public Sub NewDetails()
        mMode = "New "

    End Sub

    Public Sub EditDetails()
        mMode = "Edit"

    End Sub

    Public Sub CopyDetails()
        mMode = "Copy"

    End Sub

    Public Sub PrintDetails()

    End Sub
    Private Sub tbrList_ButtonClick(ByVal sender As System.Object, ByVal e As System.Windows.Forms.ToolBarButtonClickEventArgs) Handles tbrList.ButtonClick
        Select Case e.Button.Text
            Case "Down"
                PageDown()
            Case "Up"
                PageUp()
            Case "Display"
                DisplayDetails()
            Case "New"
                NewDetails()
            Case "Edit"
                EditDetails()
            Case "Copy"
                CopyDetails()
        End Select
    End Sub

    Private Sub mnuPopDown_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopDown.Click
        PageDown()
    End Sub

    Private Sub mnuPopUp_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopUp.Click
        PageUp()
    End Sub

    Private Sub mnuFileDown_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileDown.Click
        PageDown()
    End Sub

    Private Sub mnuFileUp_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileUp.Click
        PageUp()
    End Sub

    Private Sub cmdExit_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles cmdExit.Click
        Me.Close()
    End Sub

    Private Sub mnuFileDisplay_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileDisplay.Click
        DisplayDetails()
    End Sub

    Private Sub mnuPopDisplay_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopDisplay.Click
        DisplayDetails()
    End Sub

    Private Sub mnuFileNew_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileNew.Click
        NewDetails()
    End Sub

    Private Sub mnuPopNew_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopNew.Click
        NewDetails()
    End Sub

    Private Sub mnuFileEdit_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileEdit.Click
        EditDetails()
    End Sub

    Private Sub mnuPopEdit_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopEdit.Click
        EditDetails()
    End Sub

    Private Sub mnuFileCopy_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileCopy.Click
        CopyDetails()
    End Sub

    Private Sub mnuPopCopy_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopCopy.Click
        CopyDetails()
    End Sub

    Private Sub mnuFilePrint_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFilePrint.Click
        PrintDetails()
    End Sub

    Private Sub mnuPopPrint_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopPrint.Click
        PrintDetails()
    End Sub

    Private Sub mnuFilePrinterSetup_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFilePrinterSetup.Click
        PrintDialog1.ShowDialog()
    End Sub

    Private Sub mnuPopPrinterSetup_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopPrinterSetup.Click
        PrintDialog1.ShowDialog()
    End Sub

    Private Sub mnuFileExit_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuFileExit.Click
        Me.Close()
    End Sub

    Private Sub mnuPopExit_Click(ByVal sender As Object, ByVal e As System.EventArgs) Handles mnuPopExit.Click
        Me.Close()
    End Sub

    Private Sub FacList_KeyDown(ByVal sender As Object, ByVal e As System.Windows.Forms.KeyEventArgs) Handles MyBase.KeyDown
        If e.KeyCode = 33 Then
            PageUp()
        Else
            If e.KeyCode = 34 Then
                PageDown()
            End If
        End If
    End Sub
End Class
 
I'm not really sure what you're looking for... optimization, coding technique, or...?

In general, it looks pretty good. There are a few shortcuts you could take for coding, but they're just preferences (such as "var++" instead of "var = var + 1").

I would probably use Column names instead of indexes (in your GetString() calls). If you have to use indexes, add a comment so you know what those columns are later.

Same thing for the button indexes. I'd make a list of constants or an Enum so that you don't have to guess what those numbers mean one day.

Also, you could probably use an ArrayList for arrFac instead of a hard-coded array.

Since mMode and mFac are Public Shared, do you really want the "m" prefix (usually for private member variables)?

One last thing. I'd change the command string to check for embedded single quotes, as in:
Visual Basic: 
cmd = "SELECT * FROM Fac WHERE Fac_Code >= '" & sFac.Replace("'", "''") & "' ORDER BY Fac_Code"

-Ner
 
Thanks Nerseus, I appreciate your time in reviewing the code and the comments.

The ArrayList and the embedded single quote (hadn't thought of that) are great ideas. I will research the ArrayList and learn to use it.

I would like to use column names instead of indexes, but haven't figured that one out yet. I tried changing .GetString(0) to .Item("Fac_Code") but it creates another error that I haven't solved.

Same thing on the toolbar buttons. It appears the index is the only thing that works, I have tried tag. The Enum list would work and I can probably put that in the base forms class.

Again, Thanks for the comments and your time.
 
Back
Top