I would like to find out if there's a cleaner way to go about validating input before it goes into a function. What I'm doing seems like a lot of code, I'm sure there's a better way. I would also really appreciate some constructive criticism as my goal is to grow as a developer so anything you may see wrong with it or something you would've done better please mention it.

The code below does get the job done. I have tested it and it works, however I would like to improve its beauty and efficiency.

    Private Sub dgvproformas_CellContentClick(sender As Object, e As DataGridViewCellEventArgs) Handles dgvproformas.CellContentClick

    Dim colname As String = ""

    colname = dgvproformas.Columns(e.ColumnIndex).Name

    Select Case colname

        Case "btnsave"

            Dim jobid As Integer = 0
            Dim proformano As String = ""
            Dim paymentreceiveddate As DateTime = Nothing
            Dim paymentreceived As Boolean = False
            Dim proformarequired As Boolean = False
            Dim proformaraised As Boolean = False

            jobid = dgvproformas.Item("job id", e.RowIndex).Value

            If Not IsDBNull(dgvproformas.Item("proformano", e.RowIndex).Value) Then
                proformano = dgvproformas.Item("proformano", e.RowIndex).Value
            End If

            If Not IsDBNull(dgvproformas.Item("proformapaymentreceived", e.RowIndex)) Then
                paymentreceived = dgvproformas.Item("proformapaymentreceived", e.RowIndex).Value
            End If

            If Not IsDBNull(dgvproformas.Item("proformarequired", e.RowIndex).Value) Then
                proformarequired = dgvproformas.Item("proformarequired", e.RowIndex).Value
            End If

            If Not IsDBNull(dgvproformas.Item("proformaraised", e.RowIndex).Value) Then
                proformaraised = dgvproformas.Item("proformaraised", e.RowIndex).Value
            End If

            If Not IsDBNull(dgvproformas.Item("proformapaymentreceiveddate", e.RowIndex).Value) Then
                paymentreceiveddate = dgvproformas.Item("proformapaymentreceiveddate", e.RowIndex).Value
            End If

            If MsgBox("Are you sure you'd like to save this record?", vbYesNo) = vbYes Then
                updateProforma(jobid, proformano, paymentreceived, proformaraised, proformarequired, paymentreceiveddate)
            End If

        Case Else

    End Select

End Sub

The problem with your code is that you keep repeating the same code over and over again.

If Not IsDBNull(dgvproformas.Item("columnName", e.RowIndex).Value) Then
  columnVar = dgvproformas.Item("columnName", e.RowIndex).Value
End If

What you need is function that does the same thing, so that you have code that looks more like this:

columnVar= fn(dgvproformas, "columnName", e.RowIndex)

Except we want to keep type safety so, a bit more like:

columnVar = fn(Of Integer)(dgvproformas, "columnName", e.RowIndex)

Personally, I like extension methods for something like this, and would create an extension method for it. I would almost certainly name it GetValueOrDefault.

I believe you can eliminate the rowindex if you get the row, so combining the two...

Dim row = dgvproformas.Rows(e.RowIndex)
columnVar = row.GetValueOrDefault(Of Integer)("columnName")

Finally, I like meaningful names and dislike magic strings, so I like Nameof...

Dim row = dgvproformas.Rows(e.RowIndex)
columnName = row.GetValueOrDefault(Of Integer)(NameOf(columnName))
Private Sub dgvproformas_CellContentClick(sender As Object, 
                                          e As DataGridViewCellEventArgs) Handles dgvproformas.CellContentClick

    ' I think you don't need colname variable at all
    ' You can just check for proper column name and exit if it is not
    ' And get rid of Select Case block
    If dgvproformas.Columns(e.ColumnIndex).Name.Equals("btnSave") = False Then
        Exit Sub
    End If

    ' Now you can get instance of DataGridViewRow 
    ' With which you can get column values without everytime providing RowIndex
    Dim row As DataGridViewRow = dgvproformas.Rows(e.RowIndex)

    ' Then instead of using six parameters for updateProforma method
    ' Create a class which represent those values
    ' If you change type of the proerties to be Nullable, then
    ' you don't need to validate against null values
    Dim data As New ProformaData With
        .JobId = row.Cells("job id").Value,
        .Proformano = row.Cells("proformapaymentreceiveddate").Value,
        .PaymentReceivedDate = row.Cells("proformano").Value,
        .PaymentReceived = row.Cells("proformapaymentreceived").Value,
        .ProformaRequired = row.Cells("proformarequired").Value,
        .ProformaRaised = row.Cells("proformaraised").Value

    ' Then you can change update updateProforma method
    ' to accept instance of ProformaData instead of six parameters

    If MsgBox("Are you sure you'd like to save this record?", vbYesNo) = vbYes Then
        ' Values of Nullable you can get by GetValueOrDefault method
        ' which will return default value if Nullable has no value
        ' False for Boolean, 0 for Integer, null for String
        ' Only for string you will use If statement to get empty string instead of null
        Dim proformano As String = If(data.Proformano.GetValueOrDefault, String.Empty)
    End If       
End Sub

Public Class ProformaData
    Public Property JobId As Integer?
    Public Property Proformano  As String
    Public Property PaymentReceivedDate As DateTime?
    Public Property PaymentReceived  As Boolean?
    Public Property ProformaRequired As Boolean?
    Public Property ProformaRaised As Boolean?
End Class

Then I suggest you set "Option Strict On" in your project or in the one file at the time. "Option Strict On" will give fast feedback (during compile time) about possible type casting/converting errors.

  • 1
    \$\begingroup\$ Aside from mentioning Option Strict On, which is the reason you don't get a downvote from me, you only have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Heslacher Dec 28 '17 at 6:36

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.