X

Aggregator

CodeSOD: Copy of a Copy of a

The Daily WTF
2 months 3 weeks ago

Jessica recently started at a company still using Windows Forms.

Well, that was a short article. Oh, you want more WTF than that? Sure, we can do that.

As you might imagine, a company that's still using Windows Forms isn't going to upgrade any time soon; they've been using an API that's been in maintenance mode for a decade, clearly they're happy with it.

But they're not too happy- Jessica was asked to track down a badly performing report. This of course meant wading through a thicket of spaghetti code, pointless singletons, and the general sloppiness that is the code base. Some of the code was written using Entity Framework for database access, much of it is not.

While it wasn't the report that Jessica was sent to debug, this method caught her eye:

private Dictionary<long, decimal> GetReportDiscounts(ReportCriteria criteria) { Dictionary<long, decimal> rows = new Dictionary<long, decimal>(); string query = @"select ii.IID, SUM(CASE WHEN ii.AdjustedTotal IS NULL THEN (ii.UnitPrice * ii.Units) ELSE ii.AdjustedTotal END) as 'Costs' from ii where ItemType = 3 group by ii.IID "; string connectionString = string.Empty; using (DataContext db = DataContextFactory.GetInstance<DataContext>()) { connectionString = db.Database.Connection.ConnectionString; } using (SqlConnection connection = new SqlConnection(connectionString)) { using (SqlCommand command = new SqlCommand(query, connection)) { command.Parameters.AddWithValue("@DateStart", criteria.Period.Value.Min.Value.Date); command.Parameters.AddWithValue("@DateEnd", criteria.Period.Value.Max.Value.Date.AddDays(1)); command.Connection.Open(); using (SqlDataReader reader = command.ExecuteReader()) { while (reader.Read()) { decimal discount = (decimal)reader["Costs"]; long IID = (long)reader["IID"]; if (rows.ContainsKey(IID)) { rows[IID] += discount; } else { rows.Add(IID, discount); } } } } } return rows; }

This code constructs a query, opens a connection, runs the query, and iterates across the results, building a dictionary as its result set. The first thing which leaps out is that, in code, they're doing a summary (iterating across the results and grouping by IID), which is also what they did in the query.

It's also notable that the table they're querying is called ii, which is not a result of anonymization, and actually what they called it. Then there's the fact that they set parameters on the query, for DateStart and DateEnd, but the query doesn't use those. And then there's that magic number 3 in the query, which is its own set of questions.

Then, right beneath that method was one called GetReportTotals. I won't share it, because it's identical to what's above, with one difference:

string query = @" select ii.IID, SUM(CASE WHEN ii.AdjustedTotal IS NULL THEN (ii.UnitPrice * ii.Units) ELSE ii.AdjustedTotal END) as 'Costs' from ii where itemtype = 0 group by iid ";

The magic number is now zero.

So, clearly we're in the world of copy/paste programming, but this raises the question: which came first, the 0 or the 3? The answer is neither. GetCancelledInvoices came first.

private List<ReportDataRow> GetCancelledInvoices(ReportCriteria criteria, Dictionary<long, string> dictOfInfo) { List<ReportDataRow> rows = new List<ReportDataRow>(); string fCriteriaName = "All"; string query = @"select A long query that could easily be done in EF, or at worst a stored procedure or view. Does actually use the associated parameters"; string connectionString = string.Empty; using (DataContext db = DataContextFactory.GetInstance<DataContext>()) { connectionString = db.Database.Connection.ConnectionString; } using (SqlConnection connection = new SqlConnection(connectionString)) { using (SqlCommand command = new SqlCommand(query, connection)) { command.Parameters.AddWithValue("@DateStart", criteria.Period.Value.Min.Value.Date); command.Parameters.AddWithValue("@DateEnd", criteria.Period.Value.Max.Value.Date.AddDays(1)); command.Connection.Open(); using (SqlDataReader reader = command.ExecuteReader()) { while (reader.Read()) { long ID = (long)reader["ID"]; decimal costs = (decimal)reader["Costs"]; string mNumber = (string)reader["MNumber"]; string mName = (string)reader["MName"]; DateTime idate = (DateTime)reader["IDate"]; DateTime lastUpdatedOn = (DateTime)reader["LastUpdatedOn"]; string iNumber = reader["INumber"] is DBNull ? string.Empty : (string)reader["INumber"]; long fId = (long)reader["FID"]; string empName = (string)reader["EmpName"]; string empNumber = reader["EmpNumber"] is DBNull ? string.Empty : (string)reader["empNumber"]; long mId = (long)reader["MID"]; string cName = dictOfInfo[matterId]; if (criteria.EmployeeID.HasValue && fId != criteria.EmployeeID.Value) { continue; } rows.Add(new ReportDataRow() { CName = cName, IID = ID, Costs = costs * -1, //Cancelled i - minus PC TimedValue = 0, MNumber = mNumber, MName = mName, BillDate = lastUpdatedOn, BillNumber = iNumber + "A", FID = fId, EmployeeName = empName, EmployeeNumber = empNumber }); } } } } return rows; }

This is the original version of the method. We can infer this because it actually uses the parameters of DateStart and DateEnd. Everything else just copy/pasted this method and stripped out bits until it worked. There are more children of this method, each an ugly baby of its own, but all alike in their ugliness.

It's also worth noting, the original version is doing filtering after getting data from the database, instead of putting that criteria in the WHERE clause.

As for Jessica's poor performing report, it wasn't one of these methods. It was, however, another variation on "run a query, then filter, sort, and summarize in C#". By simply rewriting it as a SQL query in a stored procedure that leveraged indexes, performance improved significantly.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.
Remy Porter