How can we better optimize this EpiServer breadcrumb navigation code?

I'm working on improving the performance of an MVC EpiServer website, and was looking for some guidance regarding code optimization for a breadcrumb control. We are trying to reduce the memory footprint in IIS of this site when it runs.

Currently this code is in a view, which I intend to amend by placing it into a helper class. But my core question revolves around the actual objects being used here. Is any of this overkill or unnecessary?

PageData sp = DataFactory.Instance.GetPage(PageReference.StartPage);
PageRouteHelper pageRouteHelper = EPiServer.ServiceLocation.ServiceLocator.Current.GetInstance<PageRouteHelper>();
PageData currentPage = pageRouteHelper.Page;
PageData pp = DataFactory.Instance.GetPage(currentPage.ParentLink);

List<PageData> navList = new List<PageData>();
while (pp.ContentLink != sp.ContentLink)
{
    if (pp.HasTemplate() && pp.VisibleInMenu && pp.IsVisibleOnSite())
    {
        navList.Add(pp);
    }
    pp = DataFactory.Instance.GetPage(pp.ParentLink);
}
navList.Add(pp);
navList.Reverse();

sp = null;
pageRouteHelper = null;
currentPage = null;
pp = null;

The above is in a code block in the view. Then a simple loop cycles through the navList, like so:

@foreach (PageData page in navList)
{
    <li>@Html.PageLink(page)</li>
}

I dislike that twice the code employs DataFactory.Instance.GetPage, and that 3 PageData objects (plus a PageData list) get instantiated to perform this task. With 50+ properties and about 40 methods, the PageData object looks weighty.

This feels like overkill to me, especially when what we want from each PageData object is its visibility, anchor URL, and anchor text. I'd really like to avoid code bloat, especially on this Breadcrumb control that gets rendered on most of this EpiServer site's pages.

Thanks for your help.

Solution

For your concerns about PageData:

PageData objects (and all derivatives you create in your own system ("ArticlePage", "ListPage" or others) are cached in memory. Compared to "FooBar-style" tutorial example classes, it's enourmous, but in real life, you can easily handle hundreds of them for a single page load on a modern server. Add on some output caching or other caching solutions, and it becomes a non-issue in practice. Doing a bunch of Get<T>(...) and GetChildren<T>(...) for every request is perfectly fine.

It is true that it has loads of properties, but it's probably better to have "one source of truth" for a given page object, than to have custom queries for every single usage in your entire site. Suddenly, you'll want some text property you have defined. Or access the Access Control List. Or inspect it's content type. Or edit it and save it.

For example: In a simple menu, you'll probably both want its name, possibly some form of "Intro", "Excerpt" or "MainIntro" property, it's ContentLink (to create the actual value for <a href=...>) Also, you'll often handle items of a specific type, and not the PageData base class, which opens up to even more properties that you define by yourself.

So no, it is not unnecessary nor overkill. It is common practice, and it works. If you simply don't want to see all the properties while editing, you could simply handle them as IContent. Be aware though, that the implementation behind it will still be PageData objects (or rather runtime generated proxy classes for the actual page type you're retrieving).

Apart from that:

You should not put loads of code like that in a view. Program logic belongs in controllers or other classes. This has nothing to do with Episerver, more with general best practices.
You should avoid accessing DataFactory, and rather retrieve IContentLoader via dependency injection (either as a constructor parameter, or via ServiceLocator.Current.GetInstance<IContentLoader>.

Here's a flexible and easy way to do breadcrumbs:

Create a file, like HtmlHelperExtensions.cs, which contains:

public static IHtmlString BreadCrumbs(
    this HtmlHelper helper,
    ContentReference currentPage,
    Func<MenuItemViewModel, HelperResult> itemTemplate = null,
    bool includeCurrentPage = true,
    bool requireVisibleInMenu = true,
    bool requirePageTemplate = true)
{
    itemTemplate = itemTemplate ?? GetDefaultItemTemplate(helper);
    Func<IEnumerable<PageData>, IEnumerable<PageData>> filter = GetFilter(requireVisibleInMenu, requirePageTemplate);
    var menuItems = new List<MenuItemViewModel>();
    var contentLoader = ServiceLocator.Current.GetInstance<IContentLoader>();

    var currentPageData = contentLoader.Get<PageData>(currentPage);
    ContentReference parentLink = currentPageData.ParentLink;

    if (includeCurrentPage)
        menuItems.Add(CreateBreadCrumb(currentPageData, currentPage, contentLoader, filter));

    var pages = new List<PageData>();

    do
    {
        var page = contentLoader.Get<PageData>(parentLink);
        pages.Add(page);
            parentLink = page.ParentLink;
        } while (!parentLink.Equals(ContentReference.RootPage));

    menuItems.AddRange(
            pages.FilterForDisplay(requirePageTemplate, requireVisibleInMenu)
                .Select(page => CreateBreadCrumb(page, currentPage, contentLoader, filter)));

    menuItems.Reverse();

    return menuItems.List(itemTemplate);
}

private static MenuItemViewModel CreateBreadCrumb(
    PageData page,
    ContentReference currentContentLink,
    IContentLoader contentLoader,
    Func<IEnumerable<PageData>, IEnumerable<PageData>> filter)
{
    var menuItem = new MenuItemViewModel(page)
    {
        Selected = page.ContentLink.CompareToIgnoreWorkID(currentContentLink),
        HasChildren = new Lazy<bool>(() => filter(contentLoader.GetChildren<PageData>(page.ContentLink)).Any())
    };
    return menuItem;
}

private static Func<IEnumerable<PageData>, IEnumerable<PageData>> GetFilter(bool requireVisibleInMenu, bool requirePageTemplate)
{
    return pages => pages.FilterForDisplay(requirePageTemplate, requireVisibleInMenu);
}

And, in your view;

@helper BreadCrumb(MenuItemViewModel breadCrumbItem)
{
    <li>
        @if (breadCrumbItem.Page.HasTemplate() && !breadCrumbItem.Page.ContentLink.CompareToIgnoreWorkID(Model.CurrentPage.ContentLink))
        {
            @Html.PageLink(breadCrumbItem.Page)
        }
        else
        {
            @breadCrumbItem.Page.Name
        }
    </li>
}

<nav>
    <ul>
        @Html.BreadCrumbs(Model.CurrentPage.ContentLink, BreadCrumb, requireVisibleInMenu: false, includeCurrentPage: false)
    </ul>
</nav>