How to hire a programmer - Part 2 - Improve this code

In Part 1, Christopher pointed out that since we all spend a lot of time maintaining and fixing code, a good interview test is to have people fix up some smelly code.

Since I couldn't possibly agree more, I wrote a smelly codebehind page as an example of the type of test you might ask a potential candidate. I wouldn't expect someone to catch ALL the issues, or even agree with me that something actually IS an issue...but there are certainly some key items which should be fixed, and others which should atleast we discussed.

You could ask them to rewrite the code. You could ask them to talk about what they'd do, or number issues and provide a corresponding numbered list of suggestion/comments

I'll post my own "solution" to this "problem" next week :)

The test...

We have built a very simple page where clients can search our list of products by category. There are two ways to change category, either via a dropdown on the page, or by passing a categoryId in the querystring (so other pages can link directly to a category). There isn't anything functionally wrong with the site and we love the look (i.e, don't waste time changing how it looks), but the code is a little messy and we'd like to clean it up. There are no tricks or secrets. Here's what the page looks like:

Here's the codebehind file:

public partial class search : Page
{
   protected override void OnLoad(EventArgs e)
   {
      int defaultCategory;
      try
      {
         defaultCategory = Int32.Parse(Request.QueryString["CategoryId"]);
      }
      catch (Exception ex)
      {
         defaultCategory = -1;
      }

      Results.DataSource = GetResults(defaultCategory);
      Results.DataBind();

      if (!Page.IsPostBack)
      {
         CategoryList.DataSource = GetCategories();
         CategoryList.DataTextField = "Name";
         CategoryList.DataValueField = "Id";
         CategoryList.DataBind();
         CategoryList.Items.Insert(0, new ListItem("All", "-1"));
         CategoryList.SelectedIndex = CategoryList.Items.IndexOf(CategoryList.Items.FindByValue(defaultCategory.ToString()));
         base.OnLoad(e);
      }         
      
      
   }

   private void Search_Click(object sender, EventArgs e)
   {
      Results.DataSource = GetResults(Convert.ToInt32(CategoryList.SelectedValue));
      Results.DataBind();      
   }
   
   private DataTable GetCategories()
   {      
      if (Cache["AllCategories"] != null)
      {
         return (DataTable) Cache["AllCategories"];
      }

      SqlConnection connection = new SqlConnection("Data Source=DB;Initial Catalog=Store;User Id=User;Password=PW;");
      string sql = string.Format("SELECT * From Categories");
      SqlCommand command = new SqlCommand(sql, connection);

      SqlDataAdapter da = new SqlDataAdapter(command);
      DataTable dt = new DataTable();

      da.Fill(dt);
      Cache.Insert("AllCategories", dt, null, DateTime.Now.AddHours(1), System.Web.Caching.Cache.NoSlidingExpiration);

      connection.Dispose();
      return dt;
      
   }
   private DataTable GetResults(int categoryId)
   {
      SqlConnection connection = new SqlConnection("Data Source=DB;Initial Catalog=Store;User Id=User;Password=PW;");
      string sql = string.Format("SELECT * FROM Products P INNER JOIN Categories C on P.CategoryId = C.Id WHERE C.Id = {0} OR {0} = -1", categoryId);
      SqlCommand command = new SqlCommand(sql, connection);
      
      SqlDataAdapter da = new SqlDataAdapter(command);
      DataTable dt = new DataTable();
      
      da.Fill(dt);
            
      connection.Dispose();      
      return dt;           
   }
}

 

via http://codebetter.com/blogs/karlseguin/archive/2006/12/01/How-to-hire-a-programmer-_2D00_-Part-2-_2D00_-Improve-this-code.aspx