Scott Hanselman has asked me to see if I can port NerdDinner to NHibernate, now that Linq for NHibernate has made RTM. I thought that I might as well take the time to look at the code and do a code review.
I would like to say a few things before actually even starting to look at the code. I am not going to review the code as a sample application, I am going to review it as if it was a standard production project. I am assuming that (remember, haven’t seen the code yet) there are things there that are done that way explicitly because they are easier to explain as a sample app than a more complex project would be.
The first thing that I opened was HomeController, which I am going to show you in full.
There are two things that I don’t like here.
First, the empty actions (return View() is empty as far as I am concerned) resulting in what is, for all intents and purposes, an empty controller (one that contains empty actions). In other words, the only reason that this controller exists is because it is the way ASP.Net MVC want it to. A brief overview of the codebase show me that there are more than a few of method like that.
I would deal with something like that completely on the view side, having a routing action that would check the view directly and just render the view, rather than creating empty actions and controllers.
Second, [HandleErrorWithELMAH], such things should not be on controllers (and that attribute appears on all controllers). It should be on a base controller, because right now, it is a violation of DRY, and it is easy to forget.
Then there are the tests for HomeController, which are… less than useful, shall we say?
Next, let us take a look at the following interface:
I have a serious problem with the Save() method. I had to look at the implementation to figure out what it is doing. It should be called FlushToDatabase, or SubmitChanges. Save() is a totally unexpected name for the method who implementation is:
You are not, actually, saving anything.
As an aside, I don’t like seeing things like this one:
Linq to SQL actually have a way of handling cascading deletes, and it should have been used.
For that matter, coming back to the tests, there aren’t any persistence tests. Since there are some complexity involved in the queries (geo located queries), I would have liked to see at least those queries being covered.
I don’t want to rehash the poor man’s IoC story, but this is really pissing me off:
I mean, if you want to do poor man’s IoC, go ahead. But please don’t create this bastard child.
This is more about code style than hygiene, but I don’t like it:
Those classes should be in separated files, they should not be in AccountController.
Overall, I don’t see too many issues with the codebase. The only thing that pissed me off was the Mad Man IoC.