D.AddTwoNumbers

b_redeker1

So I agreed to create a couple webservices for a website that is being redesigned. Nice change from my usual work, back to developing, good. They don't have all the code for the old site, but they managed to give me some stuff that should help me on my way. Most of it wasn't really necessary because it was pretty clear cut, but now I'm at the part where a web user can request a new "PIN" and this is the old code.

The bit with "AddTwoNumbers" triggered me as it references code I don't have, but the whole sub is a nice example of a less than ideal coding style. Enjoy.

    Private Sub pbsubmit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles pbsubmit.Click
        Dim newpin As String
        Dim testnewpin As String
        Dim ds As DataSet
        Dim DSPC As DataSet
        Dim datestr As String
        Dim PC(1) As SqlParameter
        Dim ok As Boolean = False

        'Check the date cardnumber combination
        If (txtdatebirth.Text.Length > 0) And Replace(txtdatebirth.Text, "/", "").Length = 8 Then
            datestr = Replace(txtdatebirth.Text, "/", "")
            datestr = Mid(datestr, 5, 2) & "/" & Right(datestr, 2) & "/" & Left(datestr, 4)
            If IsDate(datestr) Then
                PC(0) = New SqlParameter("@cardnr", txtcardnumber.Text)
                PC(1) = New SqlParameter("@datebirth", CDate(datestr))
                DSPC = sqlh.ExecuteDataset(dsn, CommandType.StoredProcedure, "CHECKDATEBIRTH", PC)
                If DSPC.Tables(0).Rows.Count > 0 Then
                    ok = True
                End If
            End If
        End If

        If ok = True Then
            Dim P(2) As SqlParameter
            Dim objrandom As New Random
            newpin = CStr(objrandom.Next(1000, 9999))
            testnewpin = newpin
            d.AddTwoNumbers(txtcardnumber.Text, newpin)
            newpin = d.a & d.b & d.c & d.d
            P(0) = New SqlParameter("@cardnumber", txtcardnumber.Text)
            P(1) = New SqlParameter("@newpin", newpin)
            P(2) = New SqlParameter("@email", txtemail.Text)
            ds = sqlh.ExecuteDataset(dsn, CommandType.StoredProcedure, "UpdateCardPinNumber_firsttime", P)
            If ds.Tables(0).Rows.Count > 0 Then
                SendEmail(testnewpin)
                Response.Redirect("Forgot_Pin_Success.aspx")
            Else
                MsgBox2.Execute(BWare.UI.Web.WebControls.MsgBox.MsgBoxType.Alert, "Incorrect e-mail cardnumber combination entered. Please contact customer service.", "", sender)
            End If
        Else
            MsgBox2.Execute(BWare.UI.Web.WebControls.MsgBox.MsgBoxType.Alert, "Invalid date or invalid cardnumber and or date of birth combination.", "", sender)
        End If
    End Sub

Who_the_Fuck

I agree completely. Using four spaces for each indent level like that quickly causes one to either run out of room on each line, or have a funny virtual double-spaced effect, because the indent takes up more than one full line.

b_redeker1

I was more talking about:

- meaningful variable names like PC

- meaningful variable names like .AddTwoNumbers which in fact turned out to create a hash code of sorts

- reformatting the datestring without any kind of comment

- testnewpin which doesn't test anything but contains a cleartext pin

etc.

sh_code

@b_redeker said:

- meaningful variable names like PC

Parameter Collection / Parameter Couple? (not really a collection, i know, but if you use the word in its casual meaning...)

therefore DSPC = DataSet from Parameter Couple?

(why, oh why people don't like abbreviations?)

bjolling

@b_redeker said:

I was more talking about:
- meaningful variable names like PC
- meaningful variable names like .AddTwoNumbers which in fact turned out to create a hash code of sorts
- reformatting the datestring without any kind of comment
- testnewpin which doesn't test anything but contains a cleartext pin
etc.

What is far worse is that there is so much business logic in a UI function called "pbsubmit_Click". How can you write a unit test for this?The function should have offloaded the work to a controller object that takes care of all business logic. It should be something like this:

Private Sub pbsubmit_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles pbsubmit.Click
  Dim success as Boolean
  Dim errorMessage as String

  success = accountController.RequestNewPin( txtdatebirth.Text, txtcardnumber.Text, byref errorMessage )

  if( Not success ) Then
    MsgBox2.Execute(BWare.UI.Web.WebControls.MsgBox.MsgBoxType.Alert, errorMessage)
  Endif
End Sub

b_redeker1

Or split into 2 actions, first the date check, then the pinrequest, but at least follow MVC. Which, not surprisingly, means the offending code is repeated in a number of places right now.

Also, does anyone know what the advantage of that BWare tool is? According to their website,

MsgBox is an ASP.NET control which provides an easy way to show a message box on the browser. This implementation uses the JScript alert(), confirm() and prompt() message box functions to provide the required functionality.

Which sounds a bit thin.

b_redeker1

Another one from the same project.

They created a class DBinfo which in turn calls stored procedures from the database; so they were on the right track there. But then they add this little nugget:

    Public Sub FixCheckboxColor(ByVal c As CheckBox)
        c.Style("Color") = IIf(c.Checked = True, "Black", "Silver")
    End Sub

Nice.