General Feedback

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.

AccountControlle
May 17, 2010 at 5:22 PM

I've been working on a major .NET MVC project for the last six months but I didn't learn through MS examples (except for the very basics with NerdDinner.) This is the first elaborate MS example of an MVC project I've seen.   If this is the best MS has to offer, then I wonder what came before. 

Aside from the issues mentioned in the post above, why are models mixed with interface and database code?  Why are there explicit save to database calls?  Where are the unit-of-work references?  Most of all, if a heavy-weight ORM is being used, why isn't it in it's own layer (assembly or class library?)  What are these partial models? 

Maybe much of the mess is due to Entity Framework,.  I've never used it, but I've certainly heard things.  .NET MVC is truly a wonderful thing, but perhaps it only shines on a different stack.  Using it myself with an nHibernate ORM, it's truly some of the best web-app code I've seen or had the pleasure to write.

Coordinator
May 17, 2010 at 6:00 PM

@Karl, thanks so much for taking the time to respond. I'll look at what I can roll into an 0.9 release. Some specific responses:

1 - We'd decided not to talk about IoC right at the beginning to keep the focus completely on MVC. What do you think about a section at the end that introduces IoC once the mechanics are understood?

2 - Agree, will fix that.

4 - Yes. They don't have to return objects, but that's how you add metadata to EF partial classes so you can update the EF model. I agree, it's not at all beautiful.

5 - Yep

6 - Agree

7 - Both jQuery and ASP.NET Ajax are included in the ASP.NET MVC template, so this shows the two working together. jQuery is the recommended library for Javascript development on ASP.NET MVC, and the ASP.NET Ajax libraries generally add some ASP.NET MVC specific features on top. This will be more and more the case over time, based on the announcements at MIX10 stating that ASP.NET Ajax focus is being changed to work on top of jQuery at a more basic level, Microsoft has developers contributing to jQuery, etc.

Coordinator
May 17, 2010 at 6:15 PM

@theYipster - The focus of this tutorial is intentionally focused on the basic mechanics of ASP.NET MVC for people who are unfamiliar with the pattern. It's written at a more basic level than the NerdDinner sample which at least discusses testing, dependency injection, the repository pattern, etc. The idea here is to provide an introduction to ASP.NET MVC for developers for whom NerdDinner may have been offputting by focusing on exactly one concept throughout. When someone finishes it, they're of course not done learning, but they are up to speed enough with the mechanics of MVC to the point that they can then begin to apply some professional development practices.

It sounds like we need to work on two things:

1 - Notes throughout and a conclusion at the end of the tutorial that lets people know where to go next.

2 - A more advanced tutorial. However, I'm not entirely sure who this would be written for - people who already are up to speed with ASP.NET MVC, testing, architectural concerns, other ORM's, etc. don't need a tutorial. Maybe what you're after is a reference implementation showing best practices?

May 17, 2010 at 9:42 PM
Edited May 17, 2010 at 9:46 PM

If the Music Store is intended to be a first introduction to the pattern, then I feel the design flaws are even more alarming.  Isn't the whole raison-d'etre of MVC about architectural cleanliness with abstractions that work with the web's way of operation?  Isn't it about clean, minimalist code, and adherence to the well-lauded principles of "Don't Repeat Yourself" and "Separation of Concerns?" (Which, btw, the Music Store example violates in spades.)

I feel these are ideals that should be strongly shown in all examples--especially those for beginners.  The whole choice between MVC and another framework is one of architecture, and I think that understanding the architecture early on is a key to success with the framework.  Furthermore, any beginning example to a framework serves an important dual role:  not only is it a primer to a development method, it's an advertisement for the benefits of the framework.   Any good first example should show how MVC specifically, through its architecture, meets these ideals. It should clarify what the benefits (and costs) are to other options, and it should try as best as possible to demystify what return will be after making the effort to learn the framework properly.

Sure, the first example may not be that practical or concrete -- but that's probably okay and often expected (for instance, what utility is there to "Hello World" beyond introducing a language or framework?) Since MVC is all about clean architecture, I think that providing an example that is inherently unclean -- where concerns are strewn between boundaries and logic is repeated ad-nauseum--violates core principals.  Yes, the Music Store may be a concrete example of how to build something of significance without much prior knowledge of the framework, but as an education to the benefits of MVC, I feel it misses the mark, and as an advertisement to the framework, I think it has a long, long way to go.

Jun 1, 2010 at 8:28 AM

@theYipster: I see your point, but I beg to differ...

When my team started to use MVC on a new project, there were a lot of new things to learn: MVC, EF, JQuery to name just a few. And given that it was refactoring of an existing WinForms application (terribly written - but working) the clients were not very enthusiastic about us taking several months to put enhancements on hold.

So we made a conscious decision not to use unit tests, IoC, or repository pattern. We studied Nerd Dinner, and excellent Sanderson's book, so we made this decision with our eyes open. But if somebody joins our team today, this tutorial will be an excellent jump-start for WinForms developer.

In summary, ideally you should study MVC along with all the patterns that you mention; but sometimes the reality stands in the way! :)

BTW, we also decided not to use ASP.NET Ajax - jQuery seems to do everything we need. I am glad to see that ASP.NET Ajax's future is to be complementary to jQuery!

 

Jul 3, 2010 at 4:06 PM

I cannot get by pages 28 – 33.

In Browse.aspx I’m getting errors on “Model.Genre.Name” and “Model.Albums”

I’m getting the error (are you missing a using directive or an assembly reference?)

 <h2>Browsing Genre: <%: Model.Genre.Name %></h2>

    <ul>

       <% foreach (var album in Model.Albums)

           { %>

               <li><%: album.Title %></li>

       <% } %>

   </ul>

 Similar problem with Index.aspx

<h2>Album: <%: Model.Title %></h2>

 It doesn’t seem to like Model.

 Is there an issue between Models and Model? I created Models -> Album.cs and Genre.cs. Is this correct?

Coordinator
Jul 4, 2010 at 9:22 PM

@JMPlex - This should really be a new discussion or issue. I need more information on the error message, but it looks like you might not have created a strongly typed view. Can you copy in the first line of your Browse.aspx view?

Jul 9, 2010 at 6:02 PM
Jon, I started over because I had some other problems. Maybe some caused by me. I'm now on page 29 again. It says, "then modify the Browse and Details medthods to appear as follows:" Do I copy all the code described for // GET: /Store/Browse/ to // GET: /Store/Details/ and just change "public ActionResult Browse()" to "public ActionResult Details()". I assume that is what you want us to do. Then at the bottom it says, "Right-click on Browse and add a strongly typed Browse view, then a strongly typed Details view. This seems to work ok for Browse when I right click on Browse(). I get the Add View the same as on the top of page 30. When I right click on Details() and try to create a strongly typed Details view, in View data class: I have the following: MvcMusicStore.Models.Album MvcMusicStore.Models.Genre MvcMusicStore.ViewModels.StoreBrowseViewModel MvcMusicStore.ViewModels.StoreIndexViewModel Under ViewModels I only have StoreBrowseViewModel.cs StoreIndexViewModel.cs I assume I need MvcMusicStore.ViewModels.StoreDetailsViewModel in the drop down for View data class. Is that correct? I tried to enter everything very carefully this time. Did I miss something again? I never have been able to get the display on the top of page 28. with . Disco Album1 and .Disco Album 2 displayed as circled. Thanks
Coordinator
Jul 9, 2010 at 6:17 PM

@JMPlex - That looks like a typo on page 29 - it should just say "modify the Browse method to appear as follows". The instructions for the Details action method and view start on the bottom of page 30. We don't use a StoreDetailsViewModel in that case because we're just displaying the details of one Album, so the Album class is sufficient.

 

Jul 9, 2010 at 7:39 PM

Thank you. That was helpful.