- 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.
- Posted by liammclennan on February 13, 2009
Uno. The semantic problem
Exceptions "mean" that something out of the ordinary has happened. A validation error is part of the normal program flow, it is not "exceptional". New developers joining an existing team will likely be confused when they see exceptions used inappropriately.
Duo. The performance problem
Throwing exceptions is slow. Validation errors will occur all the time so performance is a reasonable consideration.
Tre. The Try... Catch problem
If you use exceptions to return model validation errors then somewhere you are going to need a Try... Catch... construct, which makes your code ugly.
... Many code bases are completely dominated by error handling ... it is nearly impossible to see what the code does because of all of the scattered error handling. - Michael Feathers, Clean Code
- Posted by liammclennan on February 10, 2009
For a long time my developer's achilles heel has been the question of how best to perform validation, specifically in the Asp.Net MVC environment. I have finally arrived at something I am happy with, so now I present to you my solution and how I got there.
Influences
My first experience working with the MVC pattern was Ruby on Rails. Here is a controller method from one of my rails projects:
def invoiced
if @entry.save
flash[:notice] = 'Entry was successfully created.'
else
flash[:notice] = 'Entry creation failed.'
end
redirect_to :action => 'list'
end
Rails uses Active Record, an implementation of the Active Record pattern. Each model object has a save method that performs validation and returns a boolean indicating if the model is valid.
When Asp.Net MVC preview 5 was released Scott Guthrie provided a sample implementation of model validation. This has become the basis of my model validation strategy.
Goals
A validation strategy must:
- support complex domain rule validations
- support simple datatype and length validations
- be able to report validation errors to the UI, showing which property caused the error.
Implementation
Each model class is responsible for validation itself and storing any errors. Each error that is recorded has an error message, the name of the property most closely associated to the error, and the erroneous value of that property. Here is the interface that my model classes must implement to be validateable:
public interface IValidateable
{
bool IsValid { get; }
IList Errors { get; }
void AddValidationFailure(string propertyName, object propertyValue, string errorMessage);
void AddValidationFailure(IList violations);
}
A caller, usually a controller action, calls IsValid to check if the entity is valid. If not then it uses the Errors collection to access the individual validation failures and report them to the user.
I added a method to my controller base class to update the ModelState with any validation errors that have occurred:
protected void SetModelErrors(IValidateable entity)
{
foreach (ValidationFailure error in entity.Errors)
{
ModelState.AddModelError(error.PropertyName, error.ErrorMessage);
}
}
Because the errors are added to the model state they will be displayed in the view because I have:
<%= Html.ValidationSummary() %>
in my master page and
<%= Html.ValidationMessage("Name", "*")%>
beside each validatable property (replace "Name" with the name of the property). Finally, I can perform validation in my controller actions using the following pattern:
public ActionResult AddToCart(Entity entityToAdd)
{
if (!entityToAdd.IsValid)
{
SetModelErrors(entityToAdd);
return View("Add", entityToAdd);
}
saver.AddToContext(entityToAdd);
saver.SaveAll();
return RedirectToAction("Menu");
}
What's Missing
It is popular to have validation performed on the client side as well. I don't do this because:
- duplication. It means validating twice.
- purity. I think my solution is conceptually pure. The javascript ones are messy.
- complex validation. Client side validation is not conducive to complex business rule validation
- there is no need. If I really want validation without a postback I use AJAX to fire the normal server-side validation.
The End
That is the high level view of my current validation strategy. I expect to tweak it slightly as I change my mind and new versions of the Asp.Net MVC framework are released. I did not discuss at all how the actual validation is performed; that's a topic for a future post.