Comments requested by experienced programmers

[spebola]

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.

 

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

[Nerseus]

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:

cmd = "SELECT * FROM Fac WHERE Fac_Code >= '" & sFac.Replace("'", "''") & "' ORDER BY Fac_Code"

 

-Ner

[spebola]

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.