Writing a thin navigation controller

Question

I'm trying to achieve a thin controller, and I started out with a thin controller but over time as the project has progressed the controller has got more complex. Could you please review the following code and let me know if the controller is still considered a thin controller or whether any of the code should be moved out into another class?

public class NavigationController : WebReportControllerBase
{
    private readonly INavigationService _service;

    public NavigationController(INavigationService service)
    {
        _service = service;
    }

    internal INavigationService Service
    {
        get
        {
            return _service;
        }
    }

    public ActionResult Browse(NavigationPanelInputModel inputModel)
    {
        var item = inputModel.Item ?? "Category";

        var navigationPanelViewModel = new NavigationPanelViewModel();
        navigationPanelViewModel.Title = "Browse";
        navigationPanelViewModel.NavigationLevel = Service.GetChildLevel<NavigationPanel>(item, "Browse");

        if (item != "Category")
        {
            navigationPanelViewModel.NavigationLevel.AllowFiltering = true;
        }

        var viewModel = new NavigationBarOverlayViewModel
        {
            ViewModel = navigationPanelViewModel,
            View = "NavigationPanelContainer",
            CssClass = "browse"
        };

        if (Request.IsAjaxRequest()) {
            if (inputModel.Item == null)
            {
                return PartialView("NavigationBarOverlay", viewModel);
            }
            else
            {
                return PartialView("~/Areas/WebReport/Views/Shared/DisplayTemplates/NavigationPanel.cshtml", navigationPanelViewModel.NavigationLevel);
            }
        }
        else
        {
            return View("PageOverlay", viewModel);
        }
    }

    public ActionResult Reports(NavigationPanelInputModel inputModel)
    {
        var item = inputModel.Item ?? "InitialList";

        var navigationPanelViewModel = new NavigationPanelViewModel();
        navigationPanelViewModel.Title = "Reports";
        navigationPanelViewModel.NavigationLevel = Service.GetChildLevel<NavigationPanel>(item, "Reports");

        var viewModel = new NavigationBarOverlayViewModel
        {
            ViewModel = navigationPanelViewModel,
            View = "NavigationPanelContainer",
            CssClass = "reports"
        };

        return View("PageOverlay", viewModel);
    }

    public ActionResult PeerGroups(NavigationPanelInputModel inputModel)
    {
        var item = inputModel.Item ?? "InitialList";

        var navigationPanelViewModel = new NavigationPanelViewModel();
        navigationPanelViewModel.Title = "Peer Groups";
        navigationPanelViewModel.NavigationLevel = Service.GetChildLevel<NavigationPanel>(item, "PeerGroups");

        var viewModel = new NavigationBarOverlayViewModel
        {
            ViewModel = navigationPanelViewModel,
            View = "NavigationPanelContainer",
            CssClass = "peer-groups"
        };

        if (Request.IsAjaxRequest())
        {
            if (inputModel.Item == null)
            {
                return PartialView("NavigationBarOverlay", viewModel);
            }
            else
            {
                return PartialView("~/Areas/WebReport/Views/Shared/DisplayTemplates/NavigationPanel.cshtml", navigationPanelViewModel.NavigationLevel);
            }
        }
        else
        {
            return View("PageOverlay", viewModel);
        }
    }

    public ActionResult MyProfile(NavigationPanelInputModel inputModel)
    {
        var item = inputModel.Item ?? "InitialList";

        var navigationPanelViewModel = new NavigationPanelViewModel();
        navigationPanelViewModel.Title = "My Profile";
        navigationPanelViewModel.NavigationLevel = Service.GetChildLevel<NavigationPanel>(item, "MyProfile");

        var viewModel = new NavigationBarOverlayViewModel
        {
            ViewModel = navigationPanelViewModel,
            View = "NavigationPanelContainer",
            CssClass = "my-profile"
        };

        if (Request.IsAjaxRequest())
        {
            return PartialView("NavigationBarOverlay", viewModel);
        }
        else
        {
            return View("PageOverlay", viewModel);
        }
    }
}

Answer 1

The responsability of a controller is to create ViewModels and return them. This usually imply some boilerplate code, creating new instances, setting parameters, etc.. Then, you need to return the good view, which also leads to some boilerplate code.

Your controllers are thin. Very thin I'd say. I think you're searching too far to refactor perfectly fine code. I know, that's a boring answer, so I'll explain why I think it's the best I can do.

What are our options? Extract a method to return the good kind of view (PartialView or View) according to the type of the request?

But this method would need 2, sometimes 4 parameters to work correctly. At least 2 parameters to define the view to return (The string name of the view) and 2 other parameters to verify if the Item is null and what view to return in that case. That sucks, right? An "helper" method with 4 parameters where 2 are optionals? Nah, let's forget that.

Extract the creation of the ViewModels perhaps? Ok, so I need a Factory Method, but what are the parameters? The Title, the Item, that other string beside the item in your service call, sometimes you'd need to pass the parameters to create NavigationBarOverlayViewModel too, which is 3 more parameters. That leads to a Factory Method that has 3 to 6 parameters, sometimes optionals, always looking alike.

What's common between those refactoring experiments? No reusability, low cohesion, high coupling. So, it's bad.

Plus, those two "helper" methods would serve what purpose? To create ViewModels and to return the good view? If we re-read the first sentence of my answer, that's exactly what the controller is supposed to do! Why would you take their jobs (South Park pun intended)?

So, all in all, that code is really good code. Keep it up.

As a little side note that might decrease code's length, here's a tip:

if (Request.IsAjaxRequest())
{
    return PartialView("NavigationBarOverlay", viewModel);
}
else
{
    return View("PageOverlay", viewModel);
}

Could be replaced with :

return Request.IsAjaxRequest() ? PartialView("NavigationBarOverlay", viewModel) : View("PageOverlay", viewModel);

Is it better? That, I don't know, it's a matter of opinion in that case.

PS: You might want to add [HttpGet] attribute above your controller's actions. You don't want people to reach these actions with a Post, Delete or Put, it'd be weird.

Thanks to @Heslacher, there's a way you can cut on some duplicated code by delegating the creation of NavigationPanelViewModel somewhere else. I would recommend creating a second constructor to this ViewModel that would look like this :

public NavigationPanelViewModel(string title, ??? level, bool allowFiltering)
{
    Title = title;
    NavigationLevel = level;
    AllowFiltering = allowFiltering;
}

(Consider verifying if parameters are null)

and use it like this :

public ActionResult Browse(NavigationPanelInputModel inputModel)
{
    var item = inputModel.Item ?? "Category";
    var childLevel = Service.GetChildLevel<NavigationPanel>(item, "Browse");
    var navigationPanelViewModel = new NavigationPanelViewModel("Browse", childLevel, item != Category);

    var viewModel = new NavigationBarOverlayViewModel
    {
        ViewModel = navigationPanelViewModel,
        View = "NavigationPanelContainer",
        CssClass = "browse"
    };

    if (Request.IsAjaxRequest()) {
        if (inputModel.Item == null)
        {
            return PartialView("NavigationBarOverlay", viewModel);
        }
        else
        {
            return PartialView("~/Areas/WebReport/Views/Shared/DisplayTemplates/NavigationPanel.cshtml", navigationPanelViewModel.NavigationLevel);
        }
    }
    else
    {
        return View("PageOverlay", viewModel);
    }
}

public ActionResult Reports(NavigationPanelInputModel inputModel)
{
    var item = inputModel.Item ?? "InitialList";
    var childLevel = Service.GetChildLevel<NavigationPanel>(item, "Reports");
    var navigationPanelViewModel = new NavigationPanelViewModel("Reports",childLevel, false);

    var viewModel = new NavigationBarOverlayViewModel
    {
        ViewModel = navigationPanelViewModel,
        View = "NavigationPanelContainer",
        CssClass = "reports"
    };

    return View("PageOverlay", viewModel);
}

Note also that the recommended use of var is when you can figure out what is the type just by looking at the code, so :

var childLevel = Service.GetChildLevel<NavigationPanel>(item, "Reports");

isn't explicit. What is returned? We don't know, which is why I'd recommend not using var!

Answer 2

I'm sorry that I can't comment on whether code needs moving out, but what I did notice is that there is a lot of code that could be refactored into a separate function which would help the functions look slimmer and might help with the decisions you have to make.

In MyProfile(), PeerGroups(), Reports() and possibly Browse() the first 10 or so lines look like the could be refactored. See what you think.