May 12, 2010 at 6:34 PM
Edited May 12, 2010 at 6:41 PM
We are finally seeing some cleaner MVC samples coming out of MS, congratulations. Good use of view models, relatively concise and well intentioned actions, and very clean and consistent code. I do have some suggestions:
1 - The IoC story could be improved. The storeDb field members in most controllers needs to get injected into the constructor. The default AccountController constructor is a smell. I know you want to focus on MVC, but MVC is largely about clean design
and separation, and DI frameworks are a great toolset to help developers achieve that.
2 - The try/catch spread over some of the controllers is avoidable exception swallowing
3 - Program against interfaces (there are a lot of cases where this isn't happening, but the List<T> in the ViewModels jumped out to me)
4 - Maybe this is a known pattern, but what's up with partial models, contained a nested "XYZMetaData" class which exposes a bunch of properties that return objects? Is that really how you do things with EF?
5 - Attribute overload (but without using 3rd party libraries, there really is no way around this, unfortunately).
6 - Some responsibility overlap. GetTopSellingAlbums doesn't belong in HomeController,
7 - 2 JS frameworks?
Again, its definitely the best Microsoft initiated MVC demo...most items on the list are pretty small (the IoC thing will cause you headaches in the long run, or when you wanna unit test though). The Model/EF story continues to be a disaster when compared
to other MVC frameworks for other stacks, but there isn't much you can do about that.