D.AddTwoNumbers



  • 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



  • 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.



  •  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.

     



  • @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?)



  • @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


  • 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.



  • 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.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.