Do not put presentation concerns in the controllers!
I thought it would be obvious, but I am currently cleaning up an application that has mixed them in a very annoying way.
I decided to track the reason for this coupling and I found this on the sample controller in ASP.Net MVC:
I am pretty sure that setting the page title is a presentation concern, as such, I don't want to see this in the controller, and I certainly don't want to see it in the base sample from which everyone is going to start.
Comments
Thats exactly what I thought. I read the the title and message in the view and thought. What the hell is this doing here? :)
Amen to that. I liked this and your other post on ViewData. What garbage. :)
I somewhat disagree.
To me, the presentation layer is in charge of how something is presented rather than what is being presented.
For example, I could have an XML view and an HTML view that determine how to present the view data, as determined by the controller.
The controller is then responsible for passing the information to be rendered to a view. If your application has the requirement for changing page titles, shouldn't the controller be in charge of passing this information around?
Maybe I am wrong, so feel free to argue back and help me see your reasoning.
Say I was using one layout for the site with a default page title and keywords. If the user was looking at a product, the title should be a brief summary of the product, and the keywords would be generated from the full description of the product, to make it google friendly. How would I accomplish this without setting the title in the controller?
You pass the product to the view for presentation, and you set it in the view
I agree with Brian. Storing the title in view data is not a presentation concern, it's part of the presentation model (which some people call viewmodel). It's the data the view needs to display. The view decides where and how to display that title.
It seems to me what you're really against is not putting the title in ViewData, it's using ViewData at all. This is a separate issue and I think there are definitely two schools of thought on this.
What a lot of people do is create a specific view model type for each view specific to that view. So rather than put Title in ViewData, you'd send a ProductDisplayModel instance to the view, and that instance would have a title property.
I think it's a matter of preference.
It would be a ProductDisplayModel, but it wouldn't be a Title property.
It would be the view that make the decision to put the Name in the title, or something like that.
Make a distinction between being able to push the data to the view, and the decision what to do with it is in the view.
Title is a view concern. Pushing the data to view is the controller concern
"It would be the view that make the decision to put the Name in the title, or something like that."
Why does my view have to make decisions? Doesn't the Controller make the decisions? So basically I'm back to writing inline logic like the old legacy asp days in my views.
This is all messed up, I'm going back to regular old WebForms, you guys are just confusing the hell out of this issue.
And if the title of the page doesn't actually come from the Product?
MVC Learner,
The view is the place to make presentation decisions.
Justin,
I am not dealing with abstract questions, give me a concrete example.
Brain,
What is changing page title?
Page title is inherently a presentation concern. I am pushing the data that the view is requiring to do its job, and it can decide how to make the presentation any way it likes
It's no more abstract than your explanation - "It would be a ProductDisplayModel, but it wouldn't be a Title property.
It would be the view that make the decision to put the Name in the title, or something like that."
You are assuming that the title of the page comes from the "Name" of the product. I simply asked what if the title of the page doesn't come from the Product?
I think this discussion really boils down to 2 different styles of thought, the passive view vs. the supervising controller.
You are vouching for a supervising controller, which receives user inputs and the view pulls information from the controller. This still has the web form issue of view logic not being testable.
My thinking revolved around a passive view, where you push all information to the view so that everything is easily testable. By minimizing the decisions in the view, you can test the UI behavior without complex functional testing frameworks like Selenium.
Justin,
In that case I guess you would use a helper.
A quick search of the castle users group says I should use CaptureFor to push properties from the view up to the layout. Right now I have ISearchEngineOptimized on my controllers but I am not happy with it at all.
Brian (great name, BTW) makes a good point when he says...
The flip side... If the controller can give the HTML title to a view, why not also an HTML table? The controller can quickly become somewhat of a rendering engine itself, which it should not be. (Slippery slope that isn't easy to go down, but you can.)
If you have complex titles derived from data, logic could be added to a view component or helper of some sort, the likes of which can know how to transform input data into output data (i.e. the html title).
Take this blog. The HTML title is "Do not put presentation concerns in the controllers!" Should it be this:
Controller: ViewData["Title"] = "Do not put presentation concerns in the controllers!"
View: <%= Title %>
or should it be this:
Controller: ViewData["Post"] = post; // the blog post
View: <%= Post.Title %>
I'll take the latter.
Justin,
I am assuming that the title of the page comes from the information that the controller passed to the view.
This may be something like "{Name} - {Price}$", or whatever the view decided. If it needs this information for presentation purposes, the controller will give it. But what it display and how it display it is the responsibilityof the view, not the controller
Brian,
Passive view is not an applicable way of working in the Web MVC world.
Brian DeMarzo,
That makes sense. Using testable helpers, while still allowing the view to be entirely responsible for presentation seems to be the elegant solution that makes the most sense.
Ayende,
Fowler's splitting of MVP has confused me I guess :) What I mean is MVP vs MVC really ( msdn.microsoft.com/en-us/magazine/cc188690.aspx)
Ok, I see the point. If the title is going to be something static then just set it as static in the view. If it's going to be dynamic, presumably that is based on something else in the model and the view can extract the requisite parts from the model to display as the title.
The title element is definitely a presentation concern.
Controller: ViewData["Title"] just makes your controller bloated. Use a helper or something inside the view itself if the title requires formatting and other logic.
How would:
ViewDate["Title"] = "The UI considers this a title";
be different from:
ViewData.Title = "The UI considers this a title";
I hardly see ViewData[string] as a Presentation concern, unless you're explicitly wrapping it witin h1 tags or something.
At some point the Controller needs to define what data will be available on the Presenter, and whether you use a Dictionary collection or a class it isn't really making a difference
I mostly agree with Ayende in that the code that default code that ships in MVC makes no sense. There is no need to set those static strings in the controller.
However, there are some cases where sending the title from the controller/model makes sense. If you are implementing a CMS where you want to give the editors full control over the title, then you have to pass it to the view somehow.
Aaron,
ViewData.Title is also a smell.
The view should make this decision, not the controller.
Ayende,
your example is not obvious for those, who don't know Microsoft implementation of MVC
I agree that setting view.title = "Hello" in controller may looks bad. But setting context["title"] = "Hello" and pushing the context as dictionary to a view is normal
Does asp.net mvc use some context keys like "Title" as special keys which are assigned to page title? Why setting "Message" is incorrect, if it is not a special key?
BTW, could you rewrite that examples in monorail?
I can definitely imagine cases where Title doesn't come directly from the concrete domain object passed to the view by controller i.e. there is some sort of "page model" dealing with page hierarchies, navigation paths etc.
It's definitely not concrete controller problem to deal with such problems as it would bloat the code to the thousand and one little scattered calls but it's not also a view problem.
I don't see a big difference in using
In Controller: ViewData["Title"]
vs.
In View: Helper.FormatTitle(Product.Name)
This is waste of time and simply just some kind of a purism, although purists don't allow helpers in their views.
More at here:
www.artima.com/lejava/articles/stringtemplate.html
This is nonsense. There's no more presentation concerns in using a ViewData property named "title" than it is in using one named "invoice". You might be arguing against the pedagogical value of the example, but as it is your criticism is predicated on nominalism, and as such invalid. There's no more meaning to "title" than there is to "invoice"; the fact that it evokes in you a certain indignation has no bearing on this matter.
As I've argued elsewhere, dogmatism opens you up to a counterargument by infinite regress: how is not a "helper object" the controller of the original controller? If the impedance mismatch gets high enough, the "helper object" would also be needed to marshal the view data out into the model, so that the fact that it doesn't actually respond to events don't make it any more passive.
It's stern dogmatism like this that promotes architecture astronautics. Just remember: neither GoF's patterns nor OO "principles" are generative, much less prescriptive (they sometimes are logically unsound, as Liskov's Substitution); they only describe "a way of doing things".
Bungle,
You are missing a very important concept. The single responsibility principle is observed using the second option.
And no, this is not purism. This is an important concept in creating maintainable applications
How would you internationali[sz]e the page HomeController is responsible for? The title is hard-wired.
Eric,
In the view:
<%= Resources.HomePageTitle %>
The single responsibility principle is key here.
The point Ayende is making is a subtle one but an architecturely sound one.
Using Matias's example and assuming that "invoice" is an invoice DTO in the view data bag. If the requirement was to display the title as a combination of the product name and invoice number then the view should look something like this:
InvoiceDto inv = (InvoiceDto)ViewData["invoice"];
-
<div<div>
Then requirements change and the invoice status is required to be displayed in addition. This is a presentation concern and should only be changed in the view (assuming of course their is a status property already). The change is easily made in the view job done...
-
<div<div - <%=inv.Status%>
If this logic was in the controller you would also have to start playing with css and html in the controller and this is a big smell. How do you change the styling of the title fragments when the view is simply given ViewData["title"]????
Give the view the data and let it do what ever it wants with it...otherwise to hell with the view lets just pass ViewData["page"] and be done????
Appologies...the code was not escaped properly, here it is in respective order:
InvoiceDto inv = (InvoiceDto)ViewData["invoice"];
<h1><%=inv.ProductName%> - <div class=somestyle><%=inv.No%><div></h1>And...
<h1><%=inv.ProductName%> - <div class=somestyle><%=inv.No%><div> - <%=inv.Status%></h1>Yes, I see this single responsibility principle. But then again who's gonna build these helpers and who's gonna learn all those helpers?
Your example:
inv.ProductName -> I need this in ASCII encoded, ProperCase and HTML escaped
inv.No -> I need this formatted with zeros removed
inv.Status -> I need this converted to image only if Status is Completed.
Then your view might look something like this
<= HTMLHelper.Escape(ProperCaseHelper.Convert(ASCIIHelper.EncodeFromUTF8(inv.ProductName))) %>
<= NumberHelper.RemoveZeros(inv.No); %>
<= (Status == Completed) ? @"
As you can see, this goes quick and easily very dirty (and I didn't even try to be nasty). In this case it would be a hell lot of easier to just say:
<%= Title %>
Or you could build whapper around your model and end with something like this:
<%= inv.ProductNameForAView %> ...
Sometimes it's just easier to put them in controller and change it there whenever it needs a change. Sometimes keeping single responsibility works better.
Helpers are more or less like server side controls. Web Designers need to learn tons of these lame helpers and their quirks as they had to learn server controls.
You can also combine several views:
In controller:
View["Title"] = RenderTitleFromStringTemplate(Product);
and then in that main view:
<%= Title %>
There are lot's of ways to do this and the lines are a little bit blurry (and they will always be if the rules aren't strictly enforced). I don't know a single framework or a view engine that does force you to single responsibility principle. And when someone tries to do something like it, people will call it impractical.
Why is passive view inappropriate for web MVC? It seems like most of the guidance I've seen, and tend to agree with, suggests that the View should be as dumb as possible, such that the primary logic of the page is in the controller, and therefore easily tested. If I change how I want to render the titles of my pages in some global fashion, it would be great if I could test that easily. But no, that doesn't mean I think I should be sending fully rendered <tables to my View - let the view do what it's good at and let the controller do everything else.
Bungle,
You are missing the most important part, the fact that it is not about the helpers, it is about the responsability.
Passive view requires communication with the controller. In Web MVC world, the controller hand off control of the request to the view and die.
It is also not much use when you are talking about views that are mostly string rendering than anything else.
But doesn't there need to be some point in which the controller determines what data will be available on the view? Isn't that the point of the controller to provide an interface to say a DAL, which isn't available at a view level?
Or am I mixing MVP too much with MVC?
"You are missing the most important part, the fact that it is not about the helpers, it is about the responsability"
Do you mean responsibility of a programmer? Or responsibility of a layer? Can you then tell me how would you have implemented the pseudo code I provided above (the one with helpers)? The helper hell isn't an answer. But without helpers you get almost a passive view (well you might have some construct depending on your templating language).
Let's make this simple with this PHP example:
controller is PHP code
view is PHP code
model is PHP code
In every layer you have the full power of PHP language. There is no strict separation of things. In this case it is responsibility of a programmer or a web designer to do the proper things in these layers. And this is what you are suggesting here. But we do not live in a perfect world, nor is MVC or anything else a silver bullet.
This is direct quote from the string template discussion I linked above:
"""
Bill Venners: So it comes down to what you think is realistic. If programmers do believe that view and model should be separated, why can't they do it with JSP or Velocity? Why can't they discipline themselves?
Terence Parr: Oh, they could, but it never happens. That is the problem.
Bill Venners: Why?
Terence Parr: It is some psychological thing. Everybody knows we shouldn't speed: it's bad for gas mileage, it's dangerous. But not everyone agrees that speeding is always a bad thing.
"""
It is the responsibility of the class / method.
Aaron,
Your description is accurate. The difference is in what you pass.
If you pass presentation elements, like Title, then you are tainting the controller with presentation concerns.
If you pass data that the view then use to present itself, this is another issue.
@Matías
Really? Exactly what HTML element does invoice refer to? None.
"responsibility of the class / method"
Classes, methods etc. don't have responsibility. They are just code. Programmer who writes them might have. But that depends on programmer and other aspects that might lead to for example quick hacks.Your one-liner comments don't really help as you don't even try to answer the questions. I ask you again, how would you write the example I provided above and adhering single responsibility?
I didn't answer your question because it is focusing on the wrong place.
The issue is not the amount of code being written, the issue is what you are doing in each place.
The controller should perform some action and hand the view the information it needs to render itself. This is usually either the application model or a view model.
The view make use of the model that the controller passed it to render itself. Making any decision that is required.
Again, the distinction here is separation of concerns, having each bit only have a single responsibility.
But my example clearly demonstrates that the amount of code can easily become issue in sense that it's not readable (maybe not even maintainable) and feels like bloat. So you either live with that kind of code or you loose single responsibility. The right choice depends on situation. I agree with you what you said about ASP.NET MVC samples regarding this conversation. It would not make samples more difficult to understand while still maintaining single responsibility aspects.
I am sorry, but I don't see this. Can you give a more expanded example?
You don't see problem here:
<title><%= HTMLHelper.Escape(ProperCaseHelper.Convert(ASCIIHelper.EncodeFromUTF8(inv.ProductName))) %></title>
Yes, you can write Helper like this:
<title><%= TitleHelper.GiveMeFormattedTitleByUsingProduct(Product) %></title>
Or do you have better ideas to implement the above and still maintaining the single responsibility? For me the above looks like shit. Helpers are shit because web designers don't usually write them. So whenever web designer needs something to be done to these model objects he/she has to ask programmer to write a new helper or modify the model to meet web designers requirements. Or he/she could write his own helpers using javascript hacks. If web designer agrees that Title (or part of it) is provided from the controller to make things easier for web designer, he/she probably says that it's ok that the Title is preformatted in controller. He probably never wants to make any changes on how it's rendered. And if you need to change it once a year, it's not a big deal to change it in controller code or some utility library the programmer has written for this.
Bungle,
Helpers are 100% part of the view logic. I don't see the distinction that you make here.
TitleHelper.GiveMeFormattedTitleByUsingProduct is one option of what I think you should use.
Another option, which is more friendly to designers, it to use a presentation model. Which take care of this.
Presentation model is again, 100% view logic.
I am sorry, but there isn't really a good way of isolating designers when you move beyond the simple databinding scenarios.
"Helpers are 100% part of the view logic."
You can have anything in your helpers. They can erase your hard drive etc. The thing is that if there is possibility to abuse single responsibility. It's abused sooner or later. Helpers are just another place to escape single responsibility (generally more places means more ways to do wrong things). That's the whole point of StringTemplate. It tries to minimize the possibilities of this happening. It cannot protect if you write models that have ToString methods that do shitty things or if controller implementer writes things like view.title = " ****Title ";
But as general principle and advice, your blog entry is right on target! In real world, people will abuse these instructions whatever you say to them. It's more than possible that people that understand this clearly, do make shortcuts. Some by mistake, some by not thinking about it and some when they are overloaded and near deadlines. If adhering single responsibility makes you do more work than it is needed without adhering to single responsibility, it's more than likely that these "mistakes" happen. This is a relatively small mistake on scale of mistakes that you can do in your programs (security, performance, usability, etc.).
"I am sorry, but there isn't really a good way of isolating designers when you move beyond the simple databinding scenarios."
Or we have not yet found one. The problem with helpers and presentation models is that these are usually programmed by the same programmer that has written the models and controllers. They are not manageable by web designer who needs them or needs to modify them. If web designer knows (as one should) Javascript, then there is one possibility: provide web designers easy way to write their own server side helpers using javascript (without compilers etc.).
No, you can't. If your helpers do things that are not directly related to presentation, that is an issue.
I don't believe is putting developers in a straight jacket just to "protect" them.
In a task oriented system the same view may be used for many purposes, especially when selecting a value from a short list. In this case you would want the view to display either
"Select a day of the week to continue"
or
"Select a site to visit"
Just because there is no useful reason for the example doesn't mean the approach has no use. Sure the example is commercially pointless, but it's not completely pointless because its point is to show how you can set something in the controller and have it displayed in whatever way the view decides is appropriate.
"Another option, which is more friendly to designers, it to use a presentation model. Which take care of this.
Presentation model is again, 100% view logic."
Now I am lost. In order to make a presentation model available for the designer to use, it has to be created somewhere. And since the presentation model was suggested as a better approach than helpers, aren't we left with the controller as the best place?
The quality of an implementation should be based on how we do it, not what we call it. However, the I feel the latter is what we are doing here.
What if you want to reuse the same set of controllers for a different set of views, as in a multi-tenant app?
Unless you are working with a set of administration screens, your HTML views will need to be search engine optimized. This is often outsourced to 3rd parties. Are you going to give them the right to modify your controller code? Are you going to make the controller responsible for meta tags too? Speaking from personal experience, I've done this already and it was a mistake.
What if the views weren't even HTML? If you wanted to use your controllers in a desktop app, and you had WPF as your view engine, you are now making you controller do extra work for stuff the view won't need.
We're making a change to Site.master in the default template to include a content section for the head section along the lines of what Ayende describes.
By default, its default content will still display <%= ViewData["Title"] %>, but any given view can simply override it.
Whoops.
I meant the default content of the placeholder is ViewData["Title"] but anyone who defines that content in their specific view will override that.
A quick question:
Who (Controller or View) should be responsible for ordering Products displayed on the page?
It seems that Controller should do it because of it requires some manipulations over the collection.
On the other side a page designer might want to order products the way he/she wants, thus View is responsible for giving the order.
Cheers.
Dmitriy,
The controller is responsible for that. Ordering should happen in the DB, not in the view.
Yes. I agree.
Just curious how would you deal if the designer (a person who develops views only) would like to have some control over things that controller is responsible for (ordering, displaying some hierarchical data with lazzy loading when controller did not loaded the data etc)?
Dmitriy,
At that point you need to negotiate how to do things.
Usually you build some sort of an interface that you use.
Ok. Would you mind to give a quick sample of such interface? Should we use DTOs (or view models) instead of Business Objects?
How would you compromise between flexibility and being protective, design practices guided?
View Model is my favorite, to tell you the truth.
For web app, I would define some Json web services that you can use.
Basically, it would all go into the same notion of making the view talk to the controller for the action
Ok. That's the key: " making the view talk to the controller for the action". But it is so huge attraction to write code like the one below instead of introducing additional layers.
and there's no way to prohibit doing it so sometimes I can find such kind of code after myself :(
It just works. Yes. But I don't sleep well with it and hope 10 seconds I spent for writing OrderBy won't become 10 days of debugging later.
Upps. My ugly code has been stripped. Trying again.
<% var products = ViewData.AsEnumerable <product("products"); %>
Sorry. It should be:
<% var products = ViewData.AsEnumerable <product("products").OrderBy(p=>p.Name); %>
Dimitriy,
If I find anyone doing something like that on the view, I am going to hit them on the head with a clue stick.
There will be bad things, we have to deal with that with code reviews, mentoring, etc.
Yep...
Oren,
Can you think about situations where it would be acceptable to do some sorting on the view?
Comment preview