Someone from accounting came to my cube, asking me to investigate why he could not void a check in our system. He says he's never had a problem before, and there was no indication of a problem this time either, except that when he looked at another report, just this one check was not voided.
OK, then. When debugging, I found the following code. Note that this is an ASP.NET application in C# with SQL Server 2005 backend. I'll provide what happens when he clicks the button in the UI to void the checks, then all relevant functions from there (mildly anonymized).
[code language="C#"]
// In the code-behind:
protected void btnVoidChecks_Click(object sender, EventArgs e)
{
try
{
lblMsg.Text = "";
SomeWcfService service = new SomeWcfService();
service.VoidChecks(Convert.ToInt32(cboSomeDropDownList.SelectedItem.Value), txtFromCheckNo.Text, txtToCheckNo.Text, this.UserProfile.UserId.Value);
lblMsg.Text = "Succeeded!";
}
catch (Exception ex)
{
lblMsg.Text = ex.Message.ToString();
}
}
// In SomeWcfService:
public int VoidChecks(int branchId, string checkNumbersFrom, string checkNumbersTo, int voidedBy)
{
return _repository.VoidChecks(branchId, checkNumbersFrom, checkNumbersTo, voidedBy);
}
// _repository is an interface type -- let's skip to the implementation it's using and the VoidChecks function there:
public int VoidChecks(int branchId, string checkNumbersFrom, string checkNumbersTo, int voidedBy)
{
// Need to make sure that the first 3 digits of checkNumbersFrom and checkNumbersTo match
// Example: Void checks from 103968 to 1032016 that have 103 as a prefix
if (checkNumbersFrom.Substring(0,3) != checkNumbersTo.Substring(0, 3))
{
throw new Exception("The first 3 digits of the Check numbers must match!");
}
else if (checkNumbersFrom.Length < 3)
{
throw new Exception("The Check number length must be 3 or greater!");
}
using (SqlConnection connection = new SqlConnection(this.ConnectionString))
{
using (var command = new SqlCommand("CheckVoid", connection))
{
command.CommandType = CommandType.StoredProcedure;
command.CommandTimeout = 500000;
command.Parameters.AddWithValue("BeginCheckNumber", checkNumbersFrom);
command.Parameters.AddWithValue("EndCheckNumber", checkNumbersTo);
command.Parameters.AddWithValue("BranchID", branchId);
command.Parameters.AddWithValue("UserID", voidedBy);
connection.Open();
object returnValue = command.ExecuteScalar();
if (returnValue == null)
{
return 0;
}
else
{
try
{
return (int)returnValue;
}
catch (Exception)
{
return -1;
}
}
}
}
}
[/code]
I won't bore you with the stored procedure, but let's say that it has the possibility of violating a primary key constraint, which is what caused the problem. That's a WTF in itself that I won't cover right here.
Now, the litany of WTFs:
- The same stupid anti-pattern shows up seemingly at random throughout the code, including here, in which instead of propagating an exception, a method returns a certain value. It's as though we didn't have Exceptions in C#.
- If that isn't bad enough, callers of these methods nearly always discard the value and treat the method as void. In this case, that means
- Why set the label to an empty string just to reset it again in all circumstances?
- That's a great command timeout for something that should be pretty much instant. How about a delay indicating a problem?
- ex.Message is already a string, so ToString() is worthless, not that we'll ever get to it anyway as long as the service is responding.
- Check numbers are compared in a range, using string comparison. There are some business rules in place that might make that not completely problematic here, but still...