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 3 – Solutions

Part 2 featured a piece of smell code and asked for ways to improve it. The goal isn't necessarily for someone to catch all the issues but at least get the big ones and be able to talk about the code. A couple small things did get past everyone :)

 The stuff most people caught:

  • Some objects aren't being properly disposed of. If it implements IDisposable, dispose of it! I really should be using the "using" or try/finally block in my data access functions.
  • There's exception swallowing when parsing an int (I'll talk more about this a little further down since some people had different views on how to do it right)
  • Connection string is hard-coded
  • SELECT * instead of specific columns
  • I'm using string concatenation for my SQL command…should use Parameters

On that last point, some people suggested avoiding inline Stored Procedures. I tend to agree, but either way, use parameterized queries! Sometimes I think people don't realize they work just fine for inline-commands as well. Although my use of string.Format with an integer data-type is probably safe, it's just asking for trouble (and as someone else said, telling all the other developers on the team that that's the way to do things).

Most people suggested a better architecture to the code, namely separating my data layer, using custom objects, a base page and applying a MVC pattern. At the least, I'd expect an interviewee to suggest moving out the data layer. Some of the comments were really open minded though and suggested improving the architecture only if this was part of a bigger project, otherwise the sloppy codebehind was probably ok. That shows a lot of maturity and understanding :)

Some of the items that got by most people:

  • When checking the cache for a value, always assign it a hard reference, else it might be gone by the time you actually try to get it. Duncan managed to get it. So instead of the code I have, you really have to do:

DataTable dt = (DataTable)Cache["AllCategories"]
if (dt == null)
{
   …get your dt
}
return dt;

  • Mark was the only one who caught that base.OnLoad(e) should be called regardless of the Postback status!

About that…it seems like at least a couple of you didn't particularly like me overriding the OnLoad and OnInit methods. I'll do a bit more research on it this week, but it's actually how I do it (I've changed my page template). Personally, I find the AutoEventWireUp to be a completely unnecessary abstraction of what's actually happening – overriding the base classes function.

As for the minor issues that seem to have gotten past everyone's radar:

  • Although there's no harm, it doesn't make any sense to worry about Request.QueryString["categoryId'] during a postback. The parameter will only be used during the initial hit to the page. Everything in the onLoad function, except base.OnLoad  should be in a If Page.IsPostback
  • My Search_Click function blindly accepts user-input and converts it to an int. The worse that'll happen is that it'll crash for that user (which I can normally live with), but it's probably something we can handle.

So the last issue is how to properly parse/deal with the categoryId input from the querystring.

Rondel suggested:
int defaultCategory = Int32.Parse(Request.QueryString["CategoryId"] ?? "-1");

but that'll crash out if CategoryId isn't null but isn't an integer either.

The consensus seemed to be to use the new Int32.TryParse (maybe short-circuiting a null check in before), which I agree with. DaRage points out that TryParse uses exceptions to do it's thing (damnit, I reformatted and don't have reflector on here yet). I think that's a good point that's worth keeping in mind,but I still think it's the right way to do it. Yes, you should only rely on exception handling for exceptional situations (crappy user-input on the querystring isn't exceptional in my mind), but that's something inside a .NET library…it's the .NET team's fault for relying on exceptions inside a function that was specifically meant to be used in this kinda case

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

14 Responses to How to hire a programmer – Part 3 – Solutions

  1. karl says:

    Ian,
    TryParse works a little differently because it won’t throw an exception if the input can’t be turned into an int:

    int userCode;
    if (!Int32.TryParse(Request.QueryString[“code”], out userCode)
    {
    userCode = -1;
    }

    as an example.

    Convert has multiple overloads, Int32.Parse/TryParse only accept strings. Many of the Convert functions just call other functions. For example, the Convert.ToINt32() overloads which expects a decimal simply calls decimal.ToInt32(). Similarly, the Convert.ToInt32() which expects a string, simply calls Int32.Parse (after doing some basic checks). In some cases though, some different logic is used.

    Karl

  2. Ian says:

    I was just wondering why you would use Int32.TryParse(…) over Convert.ToInt32(…)?

  3. karl says:

    Owen, it won’t throw an exception if it doesn’t exist in the cache. And personally, i WANT an exception if it isn’t a datatable…that means that key is being used somewhere else who knows what negative side effect that could have (at the very least it’ll hurt performance).

  4. owen says:

    Typos are your friend:
    DataTable dt = Cache[“AllCategories”] as DataTable;

  5. owen says:

    DataTable dt = (DataTable)Cache[“AllCategories”];
    Wouldn’t it be better to use the as operator here:
    DataTable dt = Cache[“AllCategories”];

    The explicit cast will throw an exception if the object is not a datatable or does not exist in the cache. The as operator on the other hand will just return null in those cases, and since you’re checking dt == null anyways….

  6. dylan says:

    “When checking the cache for a value, always assign it a hard reference, else it might be gone by the time you actually try to get it.”

    This is a good one. I’d been getting extremely random and rare errors from an application that retrieved from the cache, and it was this problem exactly: the cache was being cleared in the exact microsecond between when I checked the cache object and when I actually retrieved it.

    Thanks for the good post!

  7. Matt says:

    Scott Guthrie himself told me to override OnInit for attaching events, as event handling code is only auto-generated by VS2005 when they are defined in the codebehind and not in HTML mode. I still use the default Page_Load method, though.

  8. karl says:

    harry, thanks for the feedback:

    1 – Possibly, but it’s likely a business decision not a programmers decision. That said, I imagine most people didn’t suggest throwing an exception because the original exception defaulted to -1, suggesting that was the expected behaviour. Your point is valid though.

    2 – The cache is shared accross all users of the site. I didn’t explicitly state the usage expectation of this site, but yes, the assumption is that it’ll be used by multiple people at once. The purpose of the cache is to increase the site’s performance and decrease the load on the database. It’s a fairly common practice – although it can make scalability and testability a little harder.

  9. i does this all allways with dependency injection.
    in my pages is only an public property
    [QueryString(“myIntParam”,IfConvertionFails=-2, IfValidationFails =-3, IfSourceIsNull=-1)]
    [RangeValidator(1,100)]
    public int IntQuery
    {
    get { return intQuery; }
    set { intQuery = value; }
    }

    so i have defined the contract for data needed in my apps… thats clean code for me. under the hood it uses int.TryParse.

  10. Harry says:

    I have two questions to the solutions.

    1. What are the goals in handling CategoryId? Shouldn’t invalid query string variables (null and non-integer) be treated as invalid method parameters and cause an exception?

    2. Why is Cache even necessary here? What’s the purpose? Is there a risk that the DataTable will be retrieved more than once? Where can I find other references to this practice?

    Can someone help me out there?

  11. bill xie says:

    The solution does not look good either. Overally lots of code are written in this way. As project side and business demands evolve the code definitely ends up like a crap.

  12. Yow-Hann says:

    I don’t think TryParse throws an exception. As Eric pointed out, if perf tests are done between the two, TryParse is faster.

    In Part 2, I had attempted to explain that TryParse does not actually throw an exception and thus, you reduce overhead/performance hit from this. “you can use Int32.TryParse instead to reduce the overhead of throwing an exception” – perhaps my wording was not concise enough. In any case, my snippet was incorrect for defaulting to -1, as it should’ve been:

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

    {

    defaultCategory = -1;

    }

  13. Eric Wise says:

    *.TryParse is orders of magnitude faster than any other type of casting. Test it out.

  14. karl says:

    Int32.TryParse is pretty involved..but it doesn’t look like it relies on exceptions at all…