- Posted by liammclennan on March 11, 2009
Nerddinner is the brand new asp.net MVC example application with the tagline, "Organizing the world's nerds and helping them eat in packs". I downloaded the code to see how it was put together and compare it to my own style. Overall I quite like it, given the context that it is a very small application. It has a lightweight architecture which is excellent for the small example application that it is, but obviously would not be appropriate for a project of any size. What follows is my 2c.
Javascript is all global
The javascript, most of which is in the Map.js file, is all written in the global namespace. According to Javascript guru Douglas Crockford, "Javascript's biggest problem is its dependence on global variables". Code in the global namespace is visible from any javascript running in the page, including 3rd party scripts.
It would be nice if this code was moved into a namespace.
Too much commenting
It is now widely accepted that too much commenting is as bad, or worse, than too little. Uncle Bob goes so far as to say that, "comments are always failures" and, "nothing can clutter up a module more than frivolous dogmatic comments".
//
// ViewModel Classes
public class DinnerFormViewModel {
// Properties
public Dinner Dinner { get; private set; }
public SelectList Countries { get; private set; }
// Constructor
public DinnerFormViewModel(Dinner dinner) {
Dinner = dinner;
Countries = new SelectList(PhoneValidator.Countries, Dinner.Country);
}
}
I rest my case.
Good use of View Models
View models are projections of domain model classes that are designed to simplify binding to the UI. I like the way that they are implemented in Nerddinner. They have chosen to use a mixture of domain model and view model classes as needed, which seems like a pragmatic approach.
Parameterless Controller Constructors
The controllers have two constructors: one that accepts the controllers dependencies (great), and one that has no parameters. Im just not sure what the point of the parameterless constructor is. It seems like it could potentially hide a dependency so Im not sure if it is a good idea.
Use of Controller.User Property
The controller actions make use of the Controller.User property. In my applications I have found that this makes testing difficult and it is better to have this as an explicit dependency.
Use of == in Javascript
Geekdinner includes usages of both the == and === forms of equality testing in javascript. Because == attempts to coerce the types of its operands it is usually not a good idea to use it.
Validation
Mostly I like the way that they have implemented validation. Validation is done by the domain objects, any errors are stored on the domain object, and validation is linked to the UI via ModelState which is all great. The one thing I don't like is that they have used exceptions to indicate a validation error. I don't like this because a validation error is not exceptional. Using exceptions is both slow and semantically incorrect.
Overall I think the implementation is excellent. I wish every project I worked on was this well designed. The concerns I have pointed out are all minor things and quite possibly the result of my own misunderstanding or ignorance. I went through this exercise because I believe that a lot can be learnt by reviewing code. Why not download the source and make up your own mind?
UPDATE:
I found what I think is a problem in Nerd Dinner. Have a look at the following repository code.
public class DinnerRepository : NerdDinner.Models.IDinnerRepository {
NerdDinnerDataContext db = new NerdDinnerDataContext();
}
The trouble is that the DataContext is designed to be used as a unit of work, yet by declaring it inside the repository it becomes possible, even likely, that a single unit of work may have multiple repositories.