Ayende @ Rahien

It's a girl

Reviewing NerdDinner

Scott Hanselman has asked me to see if I can port NerdDinner to NHibernate, now that Linq for NHibernate has made RTM. I thought that I might as well take the time to look at the code and do a code review.

I would like to say a few things before actually even starting to look at the code. I am not going to review the code as a sample application, I am going to review it as if it was a standard production project. I am assuming that (remember, haven’t seen the code yet) there are things there that are done that way explicitly because they are easier to explain as a sample app than a more complex project would be.

The first thing that I opened was HomeController, which I am going to show you in full.

image

There are two things that I don’t like here.

First, the empty actions (return View() is empty as far as I am concerned) resulting in what is, for all intents and purposes, an empty controller (one that contains empty actions). In other words, the only reason that this controller exists is because it is the way ASP.Net MVC want it to. A brief overview of the codebase show me that there are more than a few of method like that.

I would deal with something like that completely on the view side, having a routing action that would check the view directly and just render the view, rather than creating empty actions and controllers.

Second, [HandleErrorWithELMAH], such things should not be on controllers (and that attribute appears on all controllers). It should be on a base controller, because right now, it is a violation of DRY, and it is easy to forget.

Then there are the tests for HomeController, which are… less than useful, shall we say?

image

Next, let us take a look at the following interface:

image

I have a serious problem with the Save() method. I had to look at the implementation to figure out what it is doing. It should be called FlushToDatabase, or SubmitChanges. Save() is a totally unexpected name for the method who implementation is:

image

You are not, actually, saving anything.

As an aside, I don’t like seeing things like this one:

image

Linq to SQL actually have a way of handling cascading deletes, and it should have been used.

For that matter, coming back to the tests, there aren’t any persistence tests. Since there are some complexity involved in the queries (geo located queries), I would have liked to see at least those queries being covered.

I don’t want to rehash the poor man’s IoC story, but this is really pissing me off:

image

I mean, if you want to do poor man’s IoC, go ahead. But please don’t create this bastard child.

This is more about code style than hygiene, but I don’t like it:

image

Those classes should be in separated files, they should not be in AccountController.

Overall, I don’t see too many issues with the codebase. The only thing that pissed me off was the Mad Man IoC.

Comments

Dmitry
07/30/2009 08:43 PM by
Dmitry

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.

Benny Thomas
07/30/2009 08:45 PM by
Benny Thomas

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.

Jeremy D. Miller
07/30/2009 08:53 PM by
Jeremy D. Miller

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;

}

}

Eyston
07/30/2009 08:57 PM by
Eyston

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.

Neil Mosafi
07/30/2009 08:58 PM by
Neil Mosafi

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!

Stephen
07/30/2009 09:02 PM by
Stephen

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.

Eyston
07/30/2009 09:04 PM by
Eyston

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.

Tyler
07/30/2009 09:08 PM by
Tyler

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.

Chris Stewart
07/30/2009 09:08 PM by
Chris Stewart

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.

NotMyself
07/30/2009 09:14 PM by
NotMyself

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?

Dmitry
07/30/2009 09:18 PM by
Dmitry

@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.

NotMyself
07/30/2009 09:29 PM by
NotMyself

@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?

Dmitry
07/30/2009 09:46 PM by
Dmitry

@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.

Scott Hanselman
07/30/2009 10:09 PM by
Scott Hanselman

Good points all!

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

But it is calling SubmitChanges? Did you mistype?

Tuna Toksoz
07/30/2009 10:15 PM by
Tuna Toksoz

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

Thomas Eyde
07/30/2009 10:21 PM by
Thomas Eyde

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?

Victor Kornov
07/30/2009 10:43 PM by
Victor Kornov

@Scott Hanselman

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

Haacked
07/30/2009 10:45 PM by
Haacked

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?

Victor Kornov
07/30/2009 10:48 PM by
Victor Kornov

@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.

Haacked
07/30/2009 10:50 PM by
Haacked

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

Victor Kornov
07/30/2009 10:52 PM by
Victor Kornov

@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".

Buddy Stein
07/30/2009 11:31 PM by
Buddy Stein

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.

alwin
07/31/2009 01:29 AM by
alwin

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.

Ayende Rahien
07/31/2009 01:56 AM by
Ayende Rahien

Stephen,

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

There are several levels of code reviews that I can make. From a Find The WTF?! (1) to We Are Good, But Let's Do Better (10)

Depending on what level I am using, I would comment on more things. For example, commenting on things that offend my sense of style, or suggestion for reconstruction.

This is a review of: What is wrong?, when I am trying to be unbiased.

Just to give you an idea, I am in 1,200 lines for a simple ToDo list app so far, because I am trying to write it as if I was writing a big application. This means that before you can do something with the app, you have to understand it.

That is obviously not something that you want in a demo app.

Ayende Rahien
07/31/2009 01:57 AM by
Ayende Rahien

Scott,

I meant, it should be called. The problem is that Save is a misleading name

Ayende Rahien
07/31/2009 02:04 AM by
Ayende Rahien

Haacked,

What was good?

Everything that I didn't comment upon :-)

Seriously, it is pretty hard to comment on an application that size, because it is literally possible to get away with anything that you want at this stage.

Ayende Rahien
07/31/2009 02:06 AM by
Ayende Rahien

Buddy,

The NHibernate impl is nothing to write home about, mind you.

It is just a brute force approach to port that.

Dave
07/31/2009 06:47 AM by
Dave

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?

Jamie
07/31/2009 09:08 AM by
Jamie

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

Bunter
07/31/2009 09:13 AM by
Bunter

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.

Thomas Eyde
07/31/2009 09:36 AM by
Thomas Eyde

@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.

Thomas Eyde
07/31/2009 09:37 AM by
Thomas Eyde

@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 :-)

Marcel
07/31/2009 10:16 AM by
Marcel

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.

Dave
07/31/2009 11:06 AM by
Dave

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..

Patrick
07/31/2009 02:00 PM by
Patrick

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.

Mats Helander
07/31/2009 02:02 PM by
Mats Helander

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 Rahien
07/31/2009 02:10 PM by
Ayende Rahien

Mats,

The problem is that there are well known conventions for doing things like that.

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

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

It is ugly.

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

Same thing for the Sochipath's IoC.

It is scattering responsibilities everywhere.

The biggest ctor should be concerned with just getting the parameters to fields, and initalizing the class.

It should not make decisions about default implementations.

Mats Helander
07/31/2009 03:01 PM by
Mats Helander

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

Dmitry
07/31/2009 04:53 PM by
Dmitry

@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

Dmitry
07/31/2009 04:55 PM by
Dmitry

@Thomas Eyde,

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

Mats Helander
07/31/2009 05:05 PM by
Mats Helander

"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

Fredy Treboux
07/31/2009 06:34 PM by
Fredy Treboux

@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?

Dmitry
07/31/2009 07:11 PM by
Dmitry

@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 Rahien
07/31/2009 09:39 PM by
Ayende Rahien

Mats,

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

You are standing in the wrong place :-)

More seriously, containers take away a LOT of the comlexities inherit in composing applications.

Trying to do it by hand is just waste at this point in time.

Take a look at some apps / frameworks that were explicitly written with an IoC in mind. Rhino Service Bus as a good example.

Mats Helander
07/31/2009 11:30 PM by
Mats Helander

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

Dmitry
08/01/2009 12:11 AM by
Dmitry

@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.

Ayende Rahien
08/01/2009 06:34 AM by
Ayende Rahien

writing and using mixins etc without using any actual AOP framework for the weaving

How are you doing this?

Then I need to have designed for this to work

Mats,

The issue here is not whatever it can be made to work or not, the issue is one of overall design. In recent years, all my apps has been using a container, to the point where I consider a container to be as necessary as System.String.

Once you accept a container as a given, your entire approach for designing a system goes through a fairly rapid transformation, because a container make things such as complex compositions so much easier. Again, I can only point out to RSB for an example of such a system.

Mats Helander
08/01/2009 08:56 AM by
Mats Helander

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...

Mats Helander
08/01/2009 09:06 AM by
Mats Helander

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

}

}

Ayende Rahien
08/01/2009 10:10 AM by
Ayende Rahien

Mats,

But why would it not be the proper concern of the object to know its null substitution values

Because this create a very hard to change object graphs.

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.

That is even leaving aside issues like life cycle management.

Creation of a dependant object is often a complex issue, and not something that should be the responsability of a class.

I'm not sure that the overall design of my apps looks so different from yours

I am pretty sure that there is. Did you review the RSB code base?

It is not a slur, it is just different methods of approaching the problem.

And no, having DI and baking the assumption of DI into the architecture are two different things.

re: Mixins

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

Mats Helander
08/01/2009 10:24 AM by
Mats Helander

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.<

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

Ayende Rahien
08/01/2009 10:32 AM by
Ayende Rahien

Mats,

I don't want to pay the framework overhead (reflection, basically)

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

Nope, you just go into FormAuth and change its default collaborators! :-)

Really?

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

Mats Helander
08/01/2009 10:40 AM by
Mats Helander

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

Mats Helander
08/01/2009 10:44 AM by
Mats Helander

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

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.<

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

Ayende Rahien
08/01/2009 10:47 AM by
Ayende Rahien

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.

re: con str

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

public Logger(ILoggingDest[] destinations)

Now you need to find all the destinations in the application. This is a fixed list, but I don't want to make this logic in a collaborator.

But I don't think that we can agree on that.

Mats Helander
08/01/2009 11:45 AM by
Mats Helander

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<

Even for around interceptors/advice?

/Mats

Mats Helander
08/01/2009 11:49 AM by
Mats Helander

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

public Logger(ILoggingDest[] destinations)<

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

Ayende Rahien
08/01/2009 01:55 PM by
Ayende Rahien

Mats,

Yes, even around advices & interceptors.

so the sensible default for it is an empty list

Which render it useless

So a single class requiring something that you cannot provide by default render the whole issue mote. At that point, why even bother?

Mats Helander
08/07/2009 03:22 PM by
Mats Helander

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

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

Comments have been closed on this topic.