I agree with all the points you made.

It would be interesting to see an example of a routing function in ASP MVC that does not need a controller.

I also do not like the way the model is structures. There is all kinds of stuff in the Model folder, view model classes, data rules, repositories and even the DBML LINQ-to-SQL file.

Rule handling is not refined nor functionally separated from the model. The view model Dinner class throws a generic ApplicationException (the exception type that should not be used according to MS best practices) when there are rule violations. The validation happens inside the model. And they are using Count() LINQ method instead of the more efficient Any() when checking for broken rules.

The dinner repository does not dispose LINQ-to-SQL context which would be a much bigger problem in NHibernate or Entity Framework due to identity map caching, etc.

Wow, mad man's IoC....

What are one thinking?

AccountController() : this(null, null)

{}

Didn't they have time to refactor?

I might have to check the codebase, I haven't had time to read the book yet, got it personally from Scott (or argued to get it...lol)

Is there any way to get rid of methods without any sence or actually doing something? Like Index and About? Some good pattern could have been used in my eyes. This should a good framework give you an option on.

I haven't seen this pattern for several years (thank goodness), but I'll see your "Mad Man's DI" and raise you to "Sociopath's DI." Think how much worse that gets when you switch to setter injection -->

public MyDependency MyDependency

{

set{ _dep = value; }

get

{

   if (_dep == null){

       _dep = new SomethingOrOther();

   }


  return _dep;

}

}

ASP.NET MVC has been my first introduction to unit testing, IoC, and (useful) polymorphism.

I started writing tests like the examples commonly given in ASP.NET MVC samples but a lot of them were less than useful as you mention. I still have a hard time deciding on how to write tests though -- searching for what testing ideology defines me as a dev.

I started doing dependency injection by having a parameterless constructor that new'd things up, but now use StructureMap. Also finding out IoC can do a lot more than DI -- it is awesome magic.

I also was not comfortable enough at first to create my own base controller class, so I applied filters to each controller separately. Once I had it working a non-DRY way, I felt I could try it the 'right' way.

So all in all, I followed a lot of the samples (from their Blogs, not NerdDinner specifically), and I don't know how comfortable I would have been jumping in all at once. It probably would have been better to go all at once, but maybe that would have turned me away. I imagine it is a hard juggle for the MVC team.

Worse is that HomeController is the exact controller created by default in Visual Studio when you create a new MVC project. What a great way to start your shiny new MVC app eh!

Yea I really don't get why the ioc was compromized like that.. seems really lazy.

I thought you'd have more to say about the codebase to be honest, it doesn't really blow me away personally.

I don't think he set out to try and prove anything in his review, it was just fulfilling a review request. The fact that there wasn't much to say should be a good thing, not a failure on an attempt to call out the MVC team.

Hopefully they fix this and update the default MVC template. Pretty much everything you just mentioned comes with the default setup, including the bad unit tests.

I tend to agree with Eyston. It's not always easy to take everything in at once. At the same time, an out of the box solution with best practices would be nice. CodeCampServer is close, but uses an extra MVC lib (MvcContrib). I would still consider that a great example. I've learned a lot from it.

I am not sure I see the problem with the DI pattern used. Why exactly is it any worse than typical poor mans DI where your empty constructor has to new up a bunch of objects?

@NotMyself,

In poor man's DI pattern the empty constructor would be the special case of initializing an object. The DI constructor is exactly like it would have been if the application was designed for real DI.

The "bastard child" over-complicates the DI constructor and makes it more difficult/dirty to convert the application to a real IOC container.

I like your idea of empty actions should be dealt differently. So I'm suggesting it for Monorail - castle.uservoice.com/.../273910-deal-with-empty...

@Dmitry

Ahh I see what you are saying now. To go from this implementation of poor mans DI to a real DI framework would cause a mixing of concerns where you have all this cruff in your DI constructor that could possibly interfere. Where as with traditional PMDI you just delete the unused constructor when you implement...

Do I have that right?

@NotMyself,

I think you got it right. The DI constructor should not have any references to concrete classes because it breaks loose coupling between assemblies and separation of concerns.

Good points all!

Quick Q...you say " It should be call FlushToDatabase, or SubmitChanges."

But it is calling SubmitChanges? Did you mistype?

I don't think so. It is probably a better name to say "SubmitChanges" because it is what the repository (indirectly) does.

The other day I was searching the net on how to do cascaded delete in Linq to SQL. What I basicly found was to let the database handle it, which is not an option, or that this is not supported.

Searching againg this evening, I found some custom code claiming to do it.

So, to paraphrase the Discovery Channel: How do they do it?

@Scott Hanselman

He means that IDinnerRepository.Save() should be renamed to IDinnerRepository. SubmitChanges()

To be fair to NerdDinner, the AccountController was included by the default project template for ASP.NET MVC. So it isn't nerd dinner's fault. That's also why you see Poor Man's IOC there in that case.

Usually I agree 100% that each class should have its own file. However, we debated this long and hard (that's what she said) in this case because we didn't want to clutter the default project template with a bunch of files.

One reason we didn't want to do that was we figured that enterprises and very experiencedevelopers would simply delete AccountController and implement their own. So we thought we'd make it easy by having just one file to delete there.

For those new to ASP.NET MVC, we wanted to keep the AccountController there as an example of how to integrate with MembershipProvider.

Do you think we should split it out in the default template anyways? Or maybe put it in a ASPNetMvcTemplate.dll assemly that's not part of the runtime but included with the project template somehow?

@Thomas Eyde

Officially L2S does not supoort cascading deletes.

You have 3 options if you need that:

1) DB referential integrity, FK

2) Custom code a-la what they have in NerdDinner

3) What Oren probably meant - 'extend' L2S data context with some methods which follow naming convention. They allow you to hook into processing of entities. As well as plain override some methods on data context.

P.S. Was there anything you liked or thought was good in there at all? :)

@Haacked

Why add something knowing peple will delete it and make efforts (hard thinking ;) ) to make it easily deletable? That should have warned you.

Anyway, I think it would be better to have 2 mvc templaes in VS, e.g. one is "Empty MVC site" and another "Example MVC site".

Scott and Phil, can I have refund on that MVC Book I bought? Only kidding, everybody can use a code review now and then.

Ayende, can't wait to see the nHibernate implementation.

I actually like the empty actions. To know which webpages there are, you just go to the controller to find out. It also helps with strongly typed links.

For me the default Home and Account controller are the least of my problems. My primary audience is Dutch. That means we want dutch urls, seems logical right. But in order to have Dutch url's (SEO) I also have to use Dutch controller names in classes and I really don't like that.

MVC should be all about separation between code and presentation. So why do I have to rename AccountController to ProfielController and rename all it's methods?!

We've spent 3 months rewriting major parts of MVC to support localization.

Back on topic: the screenshot of the AccountController class show interfaces of IFormAuthentications and IMembershipService. First IFormsAuthentication should have been named IAuthentication as I can also use OpenID or (MSIE) integrated authentication. Second, ASP.net already provides authentication en membership providers, why aren't those reused by the Microsoft developers? Are they substandard?

Dave - can't you just do that with routing??

Dave, have you ever heard of routing rules in ASP.NET MVC?? :) I seriously hope you did something other for three months than changing controller names and rewriting MVC, otherwise your time spent was utter waste.

@Dmitry,

There's no problem having concrete classes in a DI constructor when you don't need the interface. I know people differ on the subject, but coding against an abstraction doesn't necessarily mean abstract classes or interfaces.

The real power is you state your dependencies in the constructor and let the DI container do the dirty work.

@Hacked,

You could have split the file and moved them to a subfolder. Deleting one controller and one folder isn't that hard.

@Dave,

I think you can use routing to create your Dutch urls. I wish I thought of that earlier. No wait, I opted to code in Norwegian. That helps too, but the Norwegian code upset some developers, the Dutch included I guess :-)

When I was reading the part of "Cascading Deletes" I wondered if I missed something. Did quick research and Linq2SQL doesnt support this. I know in NHibernate its pretty simple, but for now seems ok to delete as they did.

Unfortunately routing rules don't really work. Or I have to use about 100 hardcoded paths in each supported language. But without heavy modifcations you can't map the url '/profiel/registratie' to AccountController with action 'Register'. I can work around that with a ProfielController class, but we also have the same url's in English (/Profile/Register), German (/Profil/Registrierung), Spanish (/Perfil/registro) and French (/Profil/enregistrement). They all have to map to the Register action. But what about input differences (cultures) like date and number formats. What about metric and unit measurements (Feet vs Meter, Celsius vs Fahrenheit)? Should you display a french-only content page to a visitor who has selected the English language? System pages (like login, forgot password, etc) are automatically redirected to the url associated with the current language.

We haven't spent 3 months only on writing a new routing module, we spent 3 months making MVC be able to cope with localization.. and than we spend 6 months on building a website upon our modified MVC framework.

And even when you have an English only MVC website, it's bad practice to let presentation determine how to name your classes. You can't just change the url /Account/Create to /Account/Register. You actually have to refactor your code (rename the Create methods to Register) just because you want to make a presentation change.. where's the separation between presentation and code here?

I hope this clarifies things a bit..

Just to follow up on what Phil said, I would personally like to see bits of code that you found to be well designed. Sometimes I get the feeling that the term "code review" ends up becoming "pointing out everything I don't like".

And as a side note, I agreed with everything you said, especially about the unit test and the attribute saturation. The unit test seems to support the notion of "100% code coverage is the goal of testing" which makes me cry :(

Thanks to NerdDinner folks for the sample app and to Oren for the feedback.

I don't understand what the problem is with the "Mad Man's DI" there.

Dimitri, you obviously have an answer, but it goes whoosh over my head...could you expand a bit on what you are saying for the thicker of us?

Jeremy, I don't see what's wrong with the Sociopath's DI either...guess that makes me a Mad Sociopath, huh? :-) Could you too please expand a bit on what you find wrong with your example?

I'm not sure what part of the context I am missing....are these patterns you would always consider bad, or is it just in the context of wanting to use some tool/framework that doesn't jive well with these setups?

/Mats

Ayende,

Thank you for your reply,

I'm still not sure I'm with you/agree, though I think I understand what you are saying now.

I think it may come down to different perspectives and preferences...concretely, I start out by disagreeing with:

"Poor Man's IoC isn't great, but it is a good solution if you can't use real IoC"

I don't want to paint you into some corner where you have to defend a lot that you may not have meant based on a quick comment in trying to explain something to me, but I think I really have a different perspective here.

Where I stand, the app should be able to stand on its own, working with useful default configurations, without involving any DI container/configuration framework. Then it should preferably be written so that it allows further configuration via such frameworks, but it should not require them, not for custom configuration and absolutely not for standard setup/initialization.

For a concrete class to have built-in dependencies to its default concrete collaborators I see as no problem at all - I see that in fact as good practice, based on what I said above that you should be able to start the app up without configuring it and have it run in a standard/default mode.

A DI framework should be able to override the use of the default collaborators, of course, but you should be able to override via "manual" configuring code as well of course (easily, not via reflection or weird special methods...just passing what you like to the constructors or setting properties should be just as easy to do for the developer as for a DI framework).

In this perspective, having a class know of its default collaborators is thus not to scatter responsibilities around - it is placing them where they belong.

"The pattern shown in ND is of a ctor passing null to another ctor to rely on a default implementation in that ctor."

Yup.

"It is ugly."

On this we disagree. I think it is pretty.

"It means that there is more code, conditionals and that changing something isn't as easy as it should."

This of course would make me agree it was ugly if you could make me see that this were true. But why would this be the case when we are talking about specifying default implementations for the collaborators an object depends on? And isn't the alternative if you don't want to specify default collaborators in the code to totally rely on a DI framework to get the app up and running?

I think perhaps our main point of disagreement is that I don't think it is neat for an object to have a "dependency" on an outside configuring process to be able to do any work at all - to me that's much worse than a default implementation of something having built-in dependencies to other default implementations of its collaborators (which should probably reside in the same assembly) because, again, I see this as no real problem at all but almost verging on a feature of OO (in essence, it is the abstract factory pattern!).

To try to become a bit more concrete - do you have any actual examples of how passing null to the ctor which knows how to then use a default implementation makes it so that "changing something isn't as easy as it should."? Remember, I want to change which default collaborator to use inside the class, not in a config file because in my perspective that is the right place to do it. And it is just one change in one place, regardless of if it is the class or in a config file. Yes it requires a recompile, but remember that's (almost) how I would want to have it for changing default collaborators (at least I see it as no cost at all, and I might even get a compilation error out of it if I did something stupid).

"More code" - Yes (very little) but fewer lines of configuration.

"More conditionals" - Why? Wouldn't that conditionality just be moved somewhere else (out of the class where at least I think it belongs)?

As always, Thank You for your utterly entertaining, thought-provoking and enlightening blog!

/Mats

@Mats,

The DI (biggest) constructor should only set the member variables of the initialized object to the provided values. The additional logic that handles "nulls" makes the constructor ugly because now it tries to do more than necessary. This logic would be totally useless if a DI container was used.

Dmitry

@Thomas Eyde,

You are absolutely correct. But I was talking about this example where IFormsAuthentication and IMembershipService are interfaces.

"This logic would be totally useless if a DI container was used."

Right, but for the case when the DI container is not used...? (But again, it can be, for custom configuration)

If that is a completely unrealistic scenario for you, then I buy what you are saying in that context, but for me that's a very realistic scenario.

/Mats

@Mats I don't think anyone is saying that, given the constraints you want to apply, it's wrong to provide default implementations of the dependencies on the same class.

What you don't want to do however, is tie the same constructor you could use for injection with those implementations... the common approach is to use another constructor that creates the dependecies and call the DI constructor.

At least to me, the null parameters meaning 'default' is what strikes me as odd, Am I wrong?

@Mats,

I still do not understand why do you need this additional logic, even if you are not planning to extend/refactor the application. Why not make the code easier, such as:

public AccountController : this(new FormsAuthentication(), new MembershipService())

{

}

• I know it is not how you instantiate the objects but it's just to make a point.

You can always use if/switch/?/?? operators instead of using polymorphism or method overloading as well but why? The point is to have code that is well separated and easy to understand.

Ayende,

You are standing in the wrong place :-)

Haha :-) I enjoy knowing that I stand in a different ring corner from you sometimes, since usually I find myself agreeing with almost everything you say.

Now, if we had been talking about, say, an O/R Mapper, I would agree that it would be mad to have your app working fine both with and without it....when it comes to an AOP framework...I'm not so totally sure anymore - in fact I tend to do more and more "manual AOP" these days (writing and using mixins etc without using any actual AOP framework for the weaving). When it comes to default configurations, this I find so easy to do manually that the DI frameworks, frankly, mostly seem to add weight and come in my way. But then, that is until I start wanting to support custom configurations, at which point specifying those with the help of a DI framework makes all the sense in the world. Then I need to have designed for this to work, but it seems to me that the ND ctor should work fine in such scenarios.

But then, that's just me I guess :-)

@Fredy

At least to me, the null parameters meaning 'default' is what strikes me as odd, Am I wrong?

Ok, I buy that all magic values are inherently bad, including having null mean "use default"...only that this is so common I hardly see it as a magic value anymore. But in principle I do buy your point.

@Dmitry

I'm not sure why creating the objects there is better...with the ND approach you get a comfy way of passing only the collaborators you want to customize and pass null to the rest to get default collaborators for them. To me that's a win.

And in no way do I see this as similar to using switch statements instead of proper OO polymorphism ;-)

Happy hacking!

/Mats

@Mats,

Would you write ADO code like this.

var value = reader["field"] != DBNull.Value ? reader.GetString("field") : "Some default value";

Or would you rather have a DB trigger/ORM interceptor/etc replacing NULLs with "Some default value", assuming the requirement makes sense?

There are 2 issues here: separation of concerns and magic values.

Dmitry,

But why would it not be the proper concern of the object to know its null substitution values (which is what I think you use in your example, rather than a default value)? The same question, of course, goes for default values.

Ayende,

I'm not sure that the overall design of my apps looks so different from yours, just that I don't often see the need to use a DI container for specifying the default configurations. And most certainly there are cases when no sensible default collaborators present themselves (for example, why would the NHibernatePersistenceService be the default IPersistenceService....there it would probably be better to rely on a DI Framework or a Service Locator to use a configuration to inject the appropriate persistence service) but in cases where obvious such defaults exist - such as in the ND example - I just don't see the drawback...

Ayende,

How are you doing this?

The obvious (cumbersome :-P) way :-)

public interface IMyFace {

string ShowIt(double input);

int CalcIt(double input);

}

public class MyClass : IMyFace {

public MyClass() {

myFaceMixin = new MyFaceMixin(this);

}

private IMyFace myFaceMixin;

public string ShowIt(double input)

{

   return this.myFaceMixin.ShowIt(input);

}

public int CalcIt(double input)

{

   return this.myFaceMixin.CalcIt(input);

}

}

public class MyFaceMixin : IMyFace

{

public MyFaceMixin(IMyFace target)

{

    this.target = target;

}


private IMyFace target;

public string ShowIt(double input)

{

   //Note that we must call target.CalcIt(), not this.CalcIt().

   //This is so the target gets a chance to 

   //override the implementation and intercept the call

   return "Calculation Result: " + target.CalcIt(input).ToString();

}

public int CalcIt(double input)

{

   return Convert.ToInt32( input ) * 42;  // << Teh Usefulness

}

}

Yuck, isn't that way way way too much repetition to write?

Yes, compared to having built-in support for mixins in the language (when will C# get this??) but there is no repetition of _logic_, which is the important part.

I'm doing it manually rather than using an AOP framework because it is performance critical code and I don't want to pay the framework overhead (reflection, basically) and I'm doing it at all because I absolutely don't want to repeat any logic...so I'm using this whenever I have already used up the inheritance option. Yes this would be a perfect case for code generation ;-)

I will review the RSB code to see what I find, but,

A good example would be when I need to change FormAuth to use something else that it requires.

Now I need to go and change it everywhere that create a FormAuth, and that is created everywhere that has a dependency on that.<<br />

Nope, you just go into FormAuth and change its default collaborators! :-) That's the beauty. What's that, FormAuth not your class? ;-) Well that's why we should always encapsulate our external dependencies, right? Adapter time.

/Mats

Yuck, isn't that way way way too much repetition to write?

Hehe, btw, you should see the scope of the bloat I'm actually writing...:-P

I adhere strictly to the following rule:

1) I always bind to interfaces.

2) Each interface has an abstract Base implementation (with the common reusables in it)

3) Then there is the (concrete) Default implementation, which inherits from the base class (and has sensible defaults for the members that were left abstract in the Base)

4) Specializations can inherit from either the Default or the Base (if the defaults are not at all useful)

5) Often there will be a mixin as well. This will then contain the actual implementation from the Base class, and the Base class will only delegate to the mixin. A class that already inherits another Base class can then also use the mixin to get the behavior.

I normally wouldn't write in this bloated way for a client, mind you. This is the way I code my own projects for _fun_! ;-) To me, the fun is in getting it right, even if it often takes lots more code and it may not be cost effective :-P But whenever I keep extending a code base that has grown beyond some complexity, and it follows this formula, I have to say I love it - and whenever I cheat on the formula I seem to come to regret it...

/Mats

Most AOP systems in .Net are working using dynamic code generation. In other words, they are as fast as anything that you can write.<<br />

Sure they generate code, but they generate glue code that delegates to the weaver that passes the packed up method to the interceptors that will use reflection to call them.

Really?

My logger impl. needs a connection string, not everything that uses FormAuth needs a connection string because a collaborator of FormAuth needs that.<<br />

So that connection string is then not a default value but a customization, and thus a great case for a DI framework! No argument there! But that doesn't mean that a sensible default value (in this case probably null/empty string) couldn't be encoded into the class.

/Mats

Take a look at Dynamic Proxy, for example. There is no reflection involved in generating a method call. Including calling interceptors or calling the actual method<<br />

Even for around interceptors/advice?

/Mats

Not really, I can think of other examples as well.

public Logger(ILoggingDest[] destinations)<<br />

But the Logger doesn't depend on having any destinations at all, so the sensible default for it is an empty list. I definitely agree that this is an example of something that could well be configured by the DI Framework. I'm talking about when an object needs some collaborators to function and when there are sensible default such collaborators to be had.

/Mats

So a single class requiring something that you cannot provide by default render the whole issue mote. <<br />

No, I don't agree. If I have just the logger, I probably would consider dragging a new (DI) framework into the mix as a bit overkill/unnecessary

overhead. In fact, in your recent post ayende.com/.../do-you-need-a-framework.aspx you excellently sum up my view here, so for reference, I refer you to yourself ;-)

Now, as you have more and more stuff like the logger, at some point the DI framework will start paying for itself. It seems you consider yourself to be at that point at a somewhat earlier stage than I do, but I think that's all we're arguing about in that respect. (Please note that I don't see any logical disconnect in you still agreeing with yourself in that post but also thinking the overhead for a DI framework is small enough to warrant its inclusion more often than I do. We view some costs differently, is all)

The original argument, however, was about whether it was for some reason bad to provide some sensible default collaborators as part of the default implementation of an interface. And here I remain unconvinced, even though Dmitry's latest comment managed to plant a nagging seed of doubt in me there...

Yes, even around advices & interceptors.

:-O I'll have to go check how the heck they pull off that magic then...cool! :-)

/Mats