views:

287

answers:

3

There has been many discussion on ASP.NET MVC and Codebehind-files, mostly where it has been pointed out that these Codebehind-files are evil.

So my question is, how do you handle page-specific logic?

What we don't want here is spaghetti-code in the inline-code and we don't want page-specific code scattered throughout helper-classes or on top of the HTML-helper-class.

An example would be:

<% for(int i = 0; i < companyList.Count; i++) { %>
    RenderCompanyNameWithRightCapsIfNotEmpty(company, i)
<% } %>

With accompanying codebehind:

private string RenderCompanyNameWithRightCapsIfNotEmpty(string company, index)
{
    if (index == 0) {
        return string.Format("<div class=\"first\">{0}</div>", company);
    }
    // Add more conditional code here
    // - page specific HTML, like render a certain icon
    string divClass = (index % 2 == 0) ? "normal" : "alternate";
    return string.Format("<div class=\"{1}\">{0}</div>", company, divClass);
}

This will only be used on one page and is most likely subject to change.

Update: A couple approaches I thought about where these:

1) Inline codebehind on page - with simple methods that returns strings.

<script runat="server">
    private string RenderCompanyHtml(string companyName) ...
<script>

2) Putting a method which returns a string in the Controller. But that would be putting View-logic into the Controller.

public class SomeController : Controller 
{
    [NonAction] 
    private static string RenderCompanyHtml(string companyName) ...

    public ActionResult Index() ...
}
+3  A: 

You should put that code in the controlleraction where you prepare the viewdata.

I usually make a region "helper methods" in my controller class with a few [NonAction] methods to keep things clean.

So my (simplified) controller would look like this:

public class SomeController : Controller 
{
  #region Helper methods
  [NonAction] 
  private static string CompanyNameWithRightCapsIfNotEmpty(string company)
  {
    if (string.IsNullOrEmpty(company)) {
        return company;
    }
    return UpperCaseSpecificWords(company);
  }

  #endregion

  public ActionResult Companies()
  {
    var companies = GetCompanies();
    var companyNames = companies.Select(c =>  CompanyNameWithRightCapsIfNotEmpty(c.Name));
    ViewData["companyNames"] = companyNames;
    return view();
  }
}
Thomas Stock
I agree with Thomas. The whole point of using MVC is to centralize your logic and gain greater separation of concerns between presentation logic and "view". As much behavior as possible should reside in your controller, leaving only basic loops and conditionals in your view. Formatting is something you are likely going to want to unit test along with anything else in your controllers...pulling it out into a view or codebehind makes it untestable behavior.
jrista
This is putting View-logic in the Controller. Is that really OK in the MVC-pattern?
Seb Nilsson
I call it "preparing the data to be viewed". I'd call it "view-logic" if I would create a string "<ul><li>" + Directory.Name + "</li><li>..." in the controller.
Thomas Stock
Don't know if this is a good example, but if you would want to show the birth year in a view, you could retrieve the full birthdate from the DB or you could just retrieve the year part. Is that putting view-logic in the data access? You could spend a lot more time argueing about it than you would spend implementing the functionality.. :-) Sometimes it's ok to break some rules if it means you have a clean, readable application. I find my solution clean and easily understandable, and I can't come up with a situation where you would be punished for doing it this way.
Thomas Stock
+1  A: 

Use Html Helpers.

Like so create the helper methods in a static class:

    public static string Label(this HtmlHelper helper, string target, string text)
    {
        return String.Format("<label for='{0}'>{1}</label>", target, text);
    }

.. then use in your view:

<span><% =Html.Label("FinishDateTime.LocalDatetime", "Finish Time:")%><br />

You could create a helper method called maybe RenderCompanyName(string[] companies) that checked for nulls, did the caps manipulation and rendered the html in between - all in the same helper if you like.

Also: controller action methods should be light - ie. only getting the data and returning views. You should delegate things like manipulation of data for presentation to views and Html helpers.

EDIT: Here is a helper that you might be after:

This helper renders an IList<> to html in the form of an unordered list <ul>...</ul>. The useful thing about it is that it gives you control over how the list is rendered thru css AND it allows you to render additional html/content for each item. Take a look - this is the helper:

    public static string UnorderedList<TItem>(this HtmlHelper helper,
        IList<TItem> items, Func<TItem, string> renderItemHtml,
        string ulID, string ulClass, string liClass)
    {
        StringBuilder sb = new StringBuilder();

        // header
        if (!ulID.IsNullOrTrimEmpty()) sb.AppendFormat("<ul id='{0}'", helper.Encode(ulID.Trim()));
        else sb.AppendFormat("<ul");
        if (!ulClass.IsNullOrTrimEmpty()) sb.AppendFormat(" class='{0}'>", helper.Encode(ulClass.Trim()));
        else sb.AppendFormat(">");

        // items
        foreach (TItem i in items)
        {
            if (!liClass.IsNullOrTrimEmpty())
                sb.AppendFormat("<li class='{0}'>{1}</li>", helper.Encode(liClass.Trim()),
                    renderItemHtml(i));
            else
                sb.AppendFormat("<li>{0}</li>", renderItemHtml(i));
        }

        // footer
        sb.AppendFormat("</ul>");

        return sb.ToString();
    }

..using it is easy. here is a simple example to render a list of tags:

    <div id="tags">
        <h2>Tags</h2>
        <%=Html.UnorderedList<Tag>(Model.Tags.Tags,tag=>
            {
                return tag.Name;
            },null,null,null) %>
    </div>

..you can see in my usage example that i have chosen not to specify any css or id attribute and i simply return the name of the Tag item thru the use of the anonymous delegate. Anonymous delegates are way easy to use.. in your case maybe something like this would work:

    <div id="tags">
        <h2>Tags</h2>
        <%=Html.UnorderedList<string>(ViewData["companies"],company=>
            {
                if (someCondition) return company.ToUpper();
                else return company;
            },null,null,null) %>
    </div>

.. ViewData["companies"] is an IList<string> for simplicity.

cottsak
Yeah but if you're going to make helper methods, you have the same problem of where to put them. It would just feel weird to me to put some page specific display logic in a /MyWebsiteHelpers/ folder together with reuseable helper methods. (I usually keep a /MyWebsiteHelpers/ folder for website specific helpers, and a seperate framework project for helpers I'll reuse for other projects as well.)
Thomas Stock
its ok to have one or two 'very specific' helpers. but like Mark Dickinson said, if you can make the helper's application a little more generic then the likelihood of it being reusable is greater. see my example above in the Edit.
cottsak
cottsak: I see your edit and it's nice, but that helper is irrelevant to the question. Your answer still has the page specific logic in the view. The helper just makes a dropdownlist out of a List<string>, it doesn't handle the companynames..
Thomas Stock
You said before that you should keep your controlleractions light, but here you are already adding a List<string> in your viewdata. If you want to use a helper, I would prefer a combination of our idea's where you do in the controlleraction: var companynames = companies.Select(c => c.Name.WithRightCapsIfNotEmpty);
Thomas Stock
cattsak, thanks for the answer, but you are totally disregarding the main points of the question:- No HTML-helpers- No inline spaghetti-code
Seb Nilsson
@Seb, a helper would still be useful to you as you need to render items from some sort of collection.. and foreach constructs are not pretty inside views - that's why there are helpers. So it turns out that if you hate spag. code so much then you still want a helper of some sort to reduce your mess in the view.
cottsak
@Seb, your example (after edit) i suspect is still not your your concrete application. why not tell us the conditions for the 'RenderCompanyNameWithRightCapsIfNotEmpty' method? If your conditions turn out to be too specific that they cant be used in a helper then the correctly-rendered strings should be coming from a Service Layer (or somewhere else that fetches/formats the data according to business rules). The formatting should not be done in the controller - that's my point.
cottsak
+2  A: 

Helper methods are one good way of handling page specific code, but I think it is a;ways preferable to get your model to show the data you need.

If you're going to go for the helper option, you would be better served by making the operation it perfroms a bit less page specific. If your method RenderCompanyNameWithRightCapsIfNotEmpty has to be so specific it would be better if your model provided it. One way would be to have the model provide a list with the text already formatted, and expose it as a public property (say an IEnumerable of formatted company names).

Mark Dickinson