Continuing the refactoring from last week, I spotted another great example this morning that I ‘almost’ didn’t improve at all.
My ‘entry point’ into this refactoring was trying to eradicate dynamic SQL and also the dreaded SELECT *, so this snippet popped up in my search.
My immediate reaction was to refactor the dynamic SQL to a Stored Procedure, but then I took a step back and had a further look at the code.
The following has been changed slightly (naming) to protect the innocent.
foreach (Organisation organisation in dataObject.DistributionList.Organisations.Values)
{
listOfIDs += “‘” + organisation.OrganisationID + “‘,”;
}
string sql = “”;
if (listOfIDs.Length > 0)
{
listOfIDs = listOfIDs.Substring(0, listOfIDs.Length – 1);
sql = “SELECT * FROM tblOrganisations WHERE OrganisationID IN (” + listOfIDs + “) ORDER BY [Name]”;
}
else
{
sql = “SELECT * FROM tblOrganisations WHERE OrganisationID = ” ORDER BY [Name]”;
}
Database database = Database.GetDefaultConnection();
DataSet dsOrganisations = database.Execute(sql, Database.CommandExecutionType.TSQLDS);
dgrConfirmDistributionList.DataSource = dsOrganisations;
dgrConfirmDistributionList.DataBind();
int rowCount = 0;
foreach (Organisation organisation in dataObject.DistributionList.Organisations.Values)
{
// Get the datagrid row we are working on
DataGridItem dataGridItem = dgrConfirmDistributionList.Items[rowCount];
// Bind Target Divisions & Select Targeted Divisions
Divisions divisions = new Divisions();
divisions.Load(dataGridItem.Cells[0].Text);
DataGrid dgrConfirmTargetDivisions =
(DataGrid) dataGridItem.Cells[2].FindControl(“dgrDivisions”);
//more…..
What this code is doing is actually using an ‘already populated’ collection of ‘Organisation’s and performing a nasty dynamic SQL call (using an constructed IN clause) back to the database to retrieve the same data again so it can bind against a datagrid.
It then goes back to using the original collection further on and does some more binding through the object hierarchy by linking to child controls in the grid.
It’s clear here that we’ve got more going on than the need to just refactor the SQL code (dodgy table names etc). It turns out that I’ve touched this code quite recently by refactoring what was a ‘pseudo-not-quite’ collection into a generic dictionary. The original poor collection implementation was probably confusing enough to put the developer off trying to use that for data binding. The collection now can of course be bound against the original collection with no need to go back to the database.
We end up with the following:
dgrConfirmDistributionList.DataBind();
int rowCount = 0;
foreach (Organisation organisation in dataObject.DistributionList.Organisations.Values)
{
// Get the datagrid row we are working on
DataGridItem dataGridItem = dgrConfirmDistributionList.Items[rowCount];
// Bind Target Divisions & Select Targeted Divisions
Divisions divisions = new Divisions();
divisions.Load(dataGridItem.Cells[0].Text);
DataGrid dgrConfirmTargetDivisions =
(DataGrid) dataGridItem.Cells[2].FindControl(“dgrDivisions”);
//more…..
This not only removed bad and unnecessary code that could have hurt performance, it also made me focus on ‘real’ refactoring opportunities. It turned out that 90% of the remaining dynamic sql calls we actually duplicates of this code in other ASP.NET pages, and so that could be removed too.
Refactoring can be quite like peeling an onion. I’d been in this code a few days ago, and due to a small improvement made then (with the collection), I was able to come back and do some more. The moral here is ‘patience’. If you’re serious about improving all code you touch then you learn to make small, manageable changes that you can test before moving on to the next. There’s still work to do on this application but the code smells are starting to fade.