Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

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:

Screenshot

Here's the codebehind file:

span.kwd{color:#22F;}
span.obj{color:#008080;}
span.str{color:#811;}

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;           
   }
}
This entry was posted in Grab a coffee before reading. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

25 Responses to How to hire a programmer – Part 2 – Improve this code

  1. This post gave us a major Brainstorm session of all the possibilities we can utilize on our blog.

  2. dude says:

    I think I see the problem. It’s written in some painful language chosen by oblivious middle-managers. What I’d do, in an interview like this, is politely excuse myself from the room.

  3. cashto says:

    Having not looked at the comments, and having no knowledge of ASP.Net codebehind (and hence unable to spot bugs / performance problems) … this is what I spotted:

    * try-except can be replaced by Int32.TryParse.
    * Use “using (SqlConnection connection = GetConnection())” rather than explicit .dispose.
    * Most duplication can be removed by introducing a “DataTable RunQuery(string)” static method.
    * The lines Results.DataSource = xxxx; Results.DataBind() are duplicated, could extract method.
    * I would negate the sense of the test Cache[“AllCategories”], populate the cache inside the conditional, and then unconditionally return Cache[“AllCategories”] afterwards.
    * string.Format of SELECT * FROM Categories is unnecessary (nothing to format).

  4. Dave Reed says:

    Yes yes… please do not fill ViewState with the data. You’re already caching it on the server, its more hurtful to bloat ViewState with it than it is to rebind it every time. Afterall, the DropDown rebuilds itself either way, may as well do it from the cached server-side data than from the posted ViewState. Everyone wins.

  5. Dave Reed says:

    (author of aboved linked article)

    True ViewState isn’t being tracked in OnInit, but it IS being tracked in child controls, so you’ll also have to disable viewstate on that there DropDown :) Selection of the initially selected index can/should remain in OnLoad on non postback only.

  6. Srin says:

    I’m not 100% sure but won’t binding the CategoryList in OnLoad put the items in the ViewState? Not a big deal here as there are only 5 items, but I’ve seen people do this with dropdown’s containing 100’s of items. Since the categories list is in the cache, override the OnInit and then bind the control everytime. You will not have any performance penalty and since ViewState is not tracked in the Init phase, you will reduce the page-size.

    Here’s a link which shows exactly what I’m talking about.

    http://weblogs.asp.net/infinitiesloop/archive/2006/08/03/Truly-Understanding-Viewstate.aspx

  7. Tim says:

    Couldn’t

    int defaultCategory;

    if (!int.TryParse(Request.QueryString[“CategoryId”].ToString(), out defaultCategory))

    {

    defaultCategory = -1;

    }

    be shortened to

    int defaultCmsFormId = -1;
    int.TryParse(Request.QueryString[“CmsFormId”], out defaultCmsFormId);

  8. karl says:

    Brian, just drop the .ToString()

  9. Brian says:

    Thanks for the pointers, Duncan. How would you write that segment of code?

  10. Duncan Godwin says:

    The problem with the TryParse code is two fold, if there is no CategoryId specified in the QueryString a NullReferenceException will be thrown on the call to ToString(), and as there’s no longer a catch all for Exception, a default won’t be set and the page will just error. Secondly the call to ToString is unnecessary because QueryString[] returns a string (null if value not found).

  11. Brian says:

    Error swallowing! Aside from the various other things I would change, the first thing I would to is to remove this

    int defaultCategory;
    try
    {
    defaultCategory = Int32.Parse(Request.QueryString[“CategoryId”]);
    }
    catch (Exception ex)
    {
    defaultCategory = -1;
    }

    in favor of something like this:

    int defaultCategory;

    if (!int.TryParse(Request.QueryString[“CategoryId”].ToString(), out defaultCategory))
    {
    defaultCategory = -1;
    }

  12. karl says:

    Mr Debug: I’ll agree with adding assertions, but not with throwing try/catch’s everywhere. You shouldn’t catch an exception unless you can actually handle it. Logging an “OutOfMemoryException” isn’t handling it…logged should be left to a global error handler. You can read more at:
    http://codebetter.com/blogs/karlseguin/archive/2006/04/05/142355.aspx

  13. At first I was bit hesitant to reply due to fear that I might write the code same as I write two years ago lols. But anyway, here’s my take.

    1. Naming. I am not a code purist but having a name casing and classname prefix saves me from collisions.

    2. BasePage. I think better if you inherit the class from a BasePage/SecuredBasePage this more flexible and maintainable in my experience.

    3. ??. I very fond of using this
    int defaultCategory = Int32.Parse(Request.QueryString[“CategoryId”] ?? “-1″);

    5. Connectionstring should be placed inside the web.config file, just not in code behind. In can be in other external configuration file.

    6. Use “using” when creating instances of connection, this is safer.

    7. For simple application that N-Tiering is an overkill, parameterized queries may suffice or stored procedures to avoid sql injection.

    8. Its my first time to see code tapped into overrriden Onload event, I think better if you have Page_Load event.

    10. Separation of concerns. As other said, if this is a member of a larger codebase, separating classes and designating responsibilities on each is smarter.

    11. Most likely you wont need it but also consider SqlCacheDependcy.

  14. Mr_Debug says:

    How about the most important thing – debug and tracing.

    About 90% of my code is written from a defensive posture. Everything is enclosed within Try/Catch and I trace into and out of all routines to a written log that can be as terse or verbose as you want.

    Anything barfs, no guessing. = bullet-proof code.

    Oh – how about adding a few asserts too?

  15. karl says:

    Nice Duncan :)

  16. Duncan Godwin says:

    Between checking if AllCategories is in the cache and getting it from the cache you could get a NullReferenceException. The simple solution would be to assign the DataTable to a variable first, and check if that is not null.

    Probably doesn’t matter too much for the size of products you have, though I’d still like to see explicit parameter binding on the SQL rather than string.Format, this will allow SQL Server to share query plans. From a security perspective I think seeing string.Format to create SQL parameters can lull another developer into a mindset where they think it’s safe extend that SQL in the same way, potentially introducing SQL Injection problems.

  17. karl says:

    Wow you guys got most of them pretty quickly.

    No one’s caught a very subtle issue that could cause a NullReferenceException under one-in-a-million timing :)

    DaRage: old habbits…I have a hard time hooking into Page_Load..I always override onInit and onLoad (I changed my default template)…good to see that you noticed it.

    And in an interview, I would be very interested/excited about someone discussion architectural changes..especially with comments such as “if this is part of a bigger…otherwise it’s probably fine…”. Very open minded :)

  18. Mark Brackett says:

    1. Catch (Exception ex) should be changed to catch a specific exception you’re guarding against (InvalidFomatException?), or check for the invalid condition first.

    2. Pull the databind of Results out into a seperate method. Call it from the OnLoad and the Search_Click.

    3. Pull the databind of Categories out into a seperate method.

    4. Connection string in web.config

    5. Don’t use SELECT *. Specify the columns. I occassionally will put the sql statement into web.config, so that it’s easy to see what is being run or to change it if needed.

    6. Use a paramater for the GetResaults sql statement. Consider making it a view.

    7. Use using on SqlConnections. Move the cache insert after the using block.

    8. As mentioned, if this is part of a larger codebase, it’d make sense to refactor to some classes and a DAL. Otherwise, I think it’s perfectly reasonable to keep the code in the codebehind, as it’s easy to understand and maintain a single page this way.

    9. I’d think base.OnLoad(e) should be called regardless of postback status….

    10. You’re missing a line break between GetResults and GetCategories…but now I’m just nit picking. 😉

  19. DaRage says:

    I forgot to say for the ones who suggesting creating a business lager and use mvp, if the page is the who application then the code behind is fine. I don’t think Karl meant architectural suggestions as much as finding code smells.

    another smell, create the connection in a using block.

  20. DaRage says:

    Yow-Hann, I wouldn’t even use Int32.TryParse or anything that rely on the exception being thrown not because of performance but if you expect an exception than it’s not an exception. I think the code should check if the query string parameter exists first and then get it’s value.

    Also, whey is it overriding the OnLoad on not using the Page_Load event handler?

    also, i think getting the results shouldn’t be done on post back.

  21. Yow-Hann says:

    If your page is part of a bigger application/project (and not just a one off project), then it may be a good approach to go with MVC architecture.

    Harry already pointed out some good ones. I will attempt to point out a couple (perhaps smaller) issues and leave code open to other community feedback:

    — Performance tip —
    Where it states:
    try
    {
    defaultCategory = Int32.Parse(Request.QueryString[“CategoryId”]);
    }
    catch (Exception ex)
    {
    defaultCategory = -1;
    }

    you can use Int32.TryParse instead to reduce the overhead of throwing an exception. So it would look like:
    defaultCategory = -1;
    Int32.TryParse(Request.QueryString[“CategoryId”], defaultCategory);

    — Exception Handling —
    The data access code, you do not have try..catch…finally wrapped around them. Thus, a perfect environment is assumed. Alternatively, you can use a ‘using’ statement instead. The using statement is pretty much the same as it will also ensure that your resource is properly disposed (it is actually a try…finally statement in IL). Also, by ‘using’ it, you will also reduce the risk of accidentally calling Close() instead of Dispose(). Dispose() actually calls Close() underneath and handles any releasing of unmanaged resources.

  22. Harry says:

    Just some ideas:
    1. All connection string must be configurable ouside of code
    2. All inline SQL must go
    3. No ADO.NET code in WebForm
    4. Use custom objects instead of DataTable
    5. If MVP is used, use a separate presenter to handle events like click

  23. karl says:

    hahah jayson, I can fess up to it too :)

  24. Very good idea, I think I’ll adopt it.

  25. I think I wrote that exact codebehind about 5 years ago! ;-).