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
- 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.
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