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