Never code review angry. ;)

With most samples built on a framework there is a balancing act:

1. keep it simple enough to be used as promotional material to encourage developers onto the framework

2. demonstrate how good practices such as IoC, SRP and unit testing etc can be leveraged on the framework

Unfortunately samples for the ASP.Net MVC 1 and 2 frameworks seem to keep towards the promotional side.

LMAO it's groundhog day. Does anyone take these "samples" seriously?

Can't people just go and find a nice open source project to use as a sample, it's going to be by far better.

That "unit test" comment around IMembershipService looks like wizard generated code from Visual Studio's MVC template. Obviously not much effort was put into this.

I take my hat off to this visionary idea of a "cart per selected product".

You are seeing the birth of a new type of ecommerce applications, multi-carts applications.

First, thanks for taking the time to look at the code and give some concrete feedback. Very much appreciated.

I'll of course be making changes based on community input. Also, we switched the repo to Mercurial today to make it easy to fork if you (or anyone else) would like, to illustrate how you'd do this better.

One thing to point out - half your criticisms were of the AccountController code which ships with MVC 2, not the Music Store code itself.

Keep in mind that this is written to teach the basic mechanics of MVC to beginners, some of whom may have avoided MVC to date due to the fact that most MVC code requires them to come up to speed on a lot of concepts all at once - all of them good, responsible practices, but sometimes they conflate the issue to a beginner who is trying to see how "hello world" gets onto the screen. Having just come from a weekend teaching absolute beginners about MVC, I'm reminded how difficult that first step can be. As it is, the tutorial is 80 pages, which is enough to turn some beginners away.

Once they are on MVC, we can get them into more advanced software development practices... maybe the first step is for me to ammend the tutuorial so that the end says "Wait, you're not done!" and points them at places to go for further study.

I'm definitely not trying to argue here, and am really happy for your feedback. Fork at will, hit the discussion forums, ping me offline, etc. Or, write more scathing blog posts, it's all good. :-)

@Jose - A Cart can contain many CartItems to track the quantity for each item. There is only one cart per user. I wish I was visionary enough to invent the cart-per-selected-product, but I'm not.

To Jon's credit, this app is meant to be the by-product of a step by step tutorial instead of sample to show best practices. For me, I can understand this point and not to worry about the lack of unit testing or DI. Anyway, good job, Jon.

Oh the shame! I've made one of these mistakes in something that's public. It's current work, but I'm not gonna say which mistake or where. Just gonna quietly fix it soon as I get them time.

I'm just glad my code doesn't get reviewed by Ayende. :-)

Well I am not surprised that it doesn't implement full blown di for a start the description says:

"MVC Music Store is a tutorial application built on ASP.NET MVC 2. It's a lightweight sample store which sells albums online, demonstrating ASP.NET MVC 2's productivity features and data access via Entity Framework 4. See what you can do with MVC 2 in under 1000 lines of c# code!".

I think that explains your issues. If you were to put other things such as di using castle, autofac or whatever etc it would make it harder for a lot of beginners to understand and i think that is the intended audience.

What we see here is a return to Component Oriented Software and entities that are nothing but a data bags to be manipulated stateless business objects. I thought that we moved past this sort of architecture.

It seems I am still living in stone age. ;-) Can you elaborate little bit more? Really appreciate if you can write a full post on it.

Ayende thanks for pointing out where this app is going wrong, thats a great help to developers out there.

Would you mind showing us a sample that would correct the flaws that you mention?

Thanks,

Dirk

The error handling is a little poor yes.

what do you mean by:

'What we see here is a return to Component Oriented Software and entities that are nothing but a data bags to be manipulated stateless business objects. I thought that we moved past this sort of architecture.'

@Judah Himango

I wish my code was reviewed by Ayende. I could actually learn a bunch from that.

"If you want to make them the default, it would be better to put them in the default constructor."

I completely agree, but: what if the user wants to replace one and use the default for the other?

This way he can call new AccountrController(new MyCrazyClass(), null);

I still think it's not the best way to do it.

@configurator

Con you do that with Named Parameters??

I like the review. The tome might be a little aggressive but I'm pleased to see this. I only have a year or so's experience with MVC and have only been a developer for 2/3 years so seeing a post like this is a great way for me to learn. I enjoy digging through apps such as music store and KIGG etc but to see them reviewed is nice. Please dont let this turn into a flame war as it could be a great topic for use newer developers to learn from.

Look forward to you post on

'What we see here is a return to Component Oriented Software and entities that are nothing but a data bags to be manipulated stateless business objects. I thought that we moved past this sort of architecture.'

Looking very forward to your upcoming analysis! I think criticizing code is great so long as you can explain what's wrong and what could be done to improve it.

For my part, having only just started to look at and get to grips with ASP.Net MVC, I found the tutorial a useful introductory help to get to grips with the various elements of the Microsoft MVC implementation, but I'd certainly never look on it as anything more than an introductory tutorial - certainly not a proposition of good coding practices.

I did also find it slightly disconcerting (from a tutorial aspect) that the latter part introduces the shopping cart, goes through how to build the basic cart presented, talks about its display, but never actually goes through how or where to add the demo products to the cart in the first place.

It also suddenly steps into JQuery and JSON with virtually no explanation, of what's going on - I was left with the impression that there were a whole stack of pages missing between 77 and 78 (in the 0.8 version PDF I was following).

Also, dropping the stylesheet and images into the fledgling project didn't product anything like the demo screenshots.

It was an interesting into, but left me thinking that the second half of the tutorial was all a little too frustrating.

@V: Optional arguments can't be used here, since default values must be constants.

It's always disappointing to see criticisms of other people's code without any specific suggestions for improvement. It would be nice to for readers to be able to see you replace the highlighted code with your better ideas.

@bz

You're comment is only valid when you take away all context from the conversation. How many times, and over how many years, is the onus on the critic to demonstrate improvements? Should we build a new demo app each time an MS demo comes out and fails to properly do DI?

Should we write out the correct way to handle exceptions even though we've been talking about it, blogging about it, writing open source applications that show it, for a decade?

Ayende can hardly be accused of not wearing his code on his sleeve.

Sorry Karl, I didn't mean to upset you. I was just suggesting that it would be more educational and of more benefit to the developer community to not just point out faults, but potential improvements as well. Personally, when I wish to see changes in something, I suggest what I feel would be the improvement. It's much more positive that way.

I wish my code was reviewed by Ayende. I could actually learn a

bunch from that.

That's surely true, if you are already at a stage far beyond the basics. For beginners such a code review is most likely confusing. Someone who is new to MVC want's to understand how it works and what could be done using it. At this stage one needs simple example projects and will not think about unit test, error handling, DDD design etc. All of this is surely important...but maybe before or after trying to understand MVC.

So, if this would be a prof. app I would totally agree wit Ayende, but for the reason of learning and understanding it is good enough to keep it simple.

+1 to bz and others

I am just starting to learn MVC and found this tutorial and code very useful.

Thanks for the effort Jon

I'm growing tired of the continued 'we have to make this easy to the dumb webform guys can figure it out' mentality.

Just do it right so we don't end up with the same ugly result of bad vb developers writing bad vb.net code when it came out...

Is this demo part of patterns and practices team ?

@Steve Dude - i'm so fraking with you on that :) I'm sooo sick and tired of all these 'newbie' demo sample apps that seriously are aimed at non-programmers IMO. I've delt with programmers before who are litterly fail-dumb .. and they do understand ASP.NET MVC after u teach them it .. slow and steady. Just like any other language and concept for the first time - slow and steady.

These things just re-iterate poor programming -> no DI, TDD (or even BDD), etc. etc. That's what's hamstringing this industry. I should know -> i was one of those dumb asses for so long (and only now i'm 3/4 dumb ass as i strive to learn how to do some of this stuff, right).

But please -> no more 'i'm a VB programmer / i've never programmed before' tutes.

Besides that, nice stuff Jon :) I'm a fan of your passion and i hope Ayende can fork and teach. Keep it up guys :)

@NC

What logic are you seeing in the view in Phil's example? The only mehtod in there is called "ShouldShow" which is clearly Presentation Logic.

I must say I haven't checked the sample, but I'm definitely looking forward to the proposed alternative on "Component Oriented Software and entities that are nothing but a data bags to be manipulated stateless business objects".

Specially if its a fork of the project and involves all the scenarios. I certainly see problems with the snippet of code and plenty of room of improvement, but after such an statement I'm expecting to have a major Aha! moment with the proposed alternative.

Wow. Some of these comments are indicative of the elitist mentality of the alt.net community at large and the same kind of mentality that puts off those developers who are trying to learn to do the right thing.

If someone puts up a tutorial for the beginner and gets flamed for his efforts, why would he continue to try to be helpful? If the beginner sees this sort of attitude, we risk killing his confidence before he even starts down the right road because from his perspective it's not filled with people who are helpful, it's filled with sharks looking for any trace of blood in the water.

Nobody has even bothered to suggest a way to fix the "issues", instead you guys have just bashed everything from default code in the account controller to using the webforms engine which, in my opinion, is a retarded thing to complain about.

I'd love to see all of you armchair critics submit something better to the community instead of just sitting atop your pedestals and spiting at those you feel are beneath you.

And before everyone rushes to Ayende's defense, my comments weren't aimed at him (he has clearly contributed much to the community) - it's aimed at all the people who felt the need to pile on the negativity.

I'm kind of with you @Eric, on whenever you create an application and put yourself out there you start to become vulnerable to purists. It's more a problem when criticisms are delivered without alternate solutions.

At the end of the day, what should matter is that you use the tools to the best of your abilities to meet (and hopefully exceed) the needs of the client. In this case the simplest solution and the path of least resistance is usually best. I do however prefer the use IOC's / Repository pattern as I believe it tends to lead to a more maintainable project.

I think the reason why it's generating a lot of criticisms is that its a 'best practices example' on how to design an MVC application and MS has been notoriously poor or slow at adopting 'best-practice' technologies and patterns. Which is a real mystery because they produce the best-of-class languages, frameworks and developer tools. I'm guessing this is more of a result in MS sponsoring PHD's on many R&D projects to test whether language features will work in the real world before actually implementing them in there more mainstream languages, so far almost everything that was introduced has been a hit.

Anyway when I'm not tied up with the commercial interests of my day job, I like to open source all my work and I will definitely welcome any code review s from anyone as I think we can all learn from constructive criticisms. I currently maintain an high-performance, cross-platform web services framework here:

http://code.google.com/p/servicestack/

And for anyone that's interested I have developed an Ajax framework that automatically compresses all your html, css and javascript in static javascript files (ala gmail) so you can deliver the fastest web experience from any static web server here:

http://code.google.com/p/ajaxstack/

I think he took some of the ideas from the Nerd Dinner tutorial. I never understood this whole poor man's DI idea.

The ShoppingCart is distributing for having data access and httpcontext dependencies. What is it supposed to be, a business object or a service/dao?

@Bradley Landis

The View should deal with nothing more than presenting the View. It shouldn't have to work out what it needs to present.

The example has helper method code IN the view. The example has filtering of the view information, in the view, not once, but TWICE.

I just refuse to understand why people create "tutorial application built on ASP.NET MVC 2" using the worst practices when they have much better tools in hands.

STOP DOING TUTORIALS! START DOING QUALITY SOFTWARE!

Or go back to the Uni...

Sorry, but it really is annoying to have billions of tutorials that show how to do thing the worst possible way.