3
\$\begingroup\$

Today I do something like this, where I get, select and sort some data and pass it on to the view (which loads a partial view) as a list of custom objects

Page model

public PagesSettings PageSettings { get; set; }
public List<Reportage> itemListReportages { get; set; }

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    var fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);

    if (fileContent?.Length > 0)
    {
        itemListReportages = (JsonConvert.DeserializeObject<List<Reportage>>(fileContent))
            .Where(l => l.Published)
            .OrderBy(l => l.Name)
            .ToList();
    }
}

Page view

@page
@model ListReportagesModel

@if (Model.PageSettings?.Data?.Count > 1 && Model.PageSettings.Data[1].Html?.Length > 0)
{
    <section class="content @(Model.PageSettings.SectionClass ?? "")">
        <div>
            @if (Model.PageSettings.Data[1].Values?.Length > 0)
                {
                    @Html.Raw(String.Format(Model.PageSettings.Data[1].Html, Model.PageSettings.Data[1].Values))
                }
                else
                {
                    @Html.Raw(@Model.PageSettings.Data[1].Html)
                }
        </div>
    </section>
}

<section class="content @(Model.PageSettings.SectionClass ?? "")">
    <div class="aflex aflexwrap aflexspacebetween">
        <partial name="/Pages/Partial/@Model.PageSettings.PartialView" />
    </div>
</section>

Partial view

@for (var i = 0; i < Model.itemListReportages.Count; i++)
{
    <div class="item">
        <div class="itembkg" style="background-image: url('@Model.itemListReportages[i].BkgImage');"></div>
        <div class="itemtxt">
            <span>@Model.itemListReportages[i].Text</span>
        </div>
    </div>
}

It now start to become a lot of PageModel's, so I started to wonder where the proper location to run C# code is. In above the data loading/select/sort is done in the PageModel and the rest in the view's.

I was planning to split it up like below instead, moving the select/sort from Pagemodel to the partial view, and with that reduce the amount of Pagemodel's and make it simpler to add data by simply create a new class + data file + partial view.

The question, which way is recommended (if any), perform select/sort in PageModel or in View?

Page model

public PagesSettings PageSettings { get; set; }
public string fileContent { get; set; }

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);
}

Partial view

if (fileContent?.Length > 0)
{
    var itemListReportages = (JsonConvert.DeserializeObject<List<Reportage>>(fileContent))
        .Where(l => l.Published)
        .OrderBy(l => l.Name)
        .ToList();

    @for (var i = 0; i < Model.itemListReportages.Count; i++)
    {
        <div class="item">
            <div class="itembkg" style="background-image: url('@Model.itemListReportages[i].BkgImage');"></div>
            <div class="itemtxt">
                <span>@Model.itemListReportages[i].Text</span>
            </div>
        </div>
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I had to rollback your last edit as it is not allowed to modify the code after reviews have been posted. It could (and in this case it did) invalidate them. \$\endgroup\$ – t3chb0t Aug 23 at 6:52
  • \$\begingroup\$ This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 23 at 6:59
  • \$\begingroup\$ When you do this, then I'll vote to close your qustion for posting hyphotetical code as this is off-topic on Code Review. I'll also delete my answer. Let's preted this has never happened ;-P and next time, please just copy/paste all of your code without changing it. \$\endgroup\$ – t3chb0t Aug 23 at 6:59
  • \$\begingroup\$ @t3chb0t -- Are you saying that I should have posted 1200 lines of code + all the linked/used utility library methods, and then ask about 20 lines of them (the one I posted), which is the main logic of my question? \$\endgroup\$ – LGSon Aug 23 at 7:06
  • \$\begingroup\$ Nope, I mean that the posted code should be complete. Not your entire application but a method... a class, a (partial)view etc. Otherwise this is the result... you get a review and you replay but I already do this... it simply backfires and isn't helpful to anyone, especially to you. \$\endgroup\$ – t3chb0t Aug 23 at 7:11
2
\$\begingroup\$

I find you should keep the library code in the model and only parts that are relevant to views inside them.


In your case there is actually a third option. Let OnGet only handle the file. Remove OrderBy as this is a view's matter how it's displayed. Additionally initialize itemListReportages to an empty list if a file was empty. It'll save you the trouble of null checks later.

public void OnGet()
{
    PageSettings = (PagesSettings)HttpContext.Items["pagesettings"];

    if (PageSettings == null)
    {
        return;
    }

    var fileContent = _fileService.GetFileContentCached(PageSettings.DataFile);

    itemListReportages = 
        fileContent?.Length > 0
            ? JsonConvert.DeserializeObject<List<Reportage>>(fileContent).Where(l => l.Published).ToList()
            : new List<Reportage>();
}

We don't see whether you initialize itemListReportages but this view could crash because you don't use the null propagation operator here: ?.Count

@for (var i = 0; i < Model.itemListReportages.Count; i++)
{
    <div class="item">
        <div class="itembkg" style="background-image: url('@Model.itemListReportages[i].BkgImage');"></div>
        <div class="itemtxt">
            <span>@Model.itemListReportages[i].Text</span>
        </div>
    </div>
}

With a non-null list you can turn this into a foreach loop and put OrderBy here. It'll make the code shorter too as you now have an item variable and no longer have to acces them by index like Model.itemListReportages[i]:

@foreach (var item in Model.itemListReportages.OrderBy(x => x.Name))
{
    <div class="item">
        <div class="itembkg" style="background-image: url('@item.BkgImage');"></div>
        <div class="itemtxt">
            <span>@item.Text</span>
        </div>
    </div>
}
\$\endgroup\$
  • 1
    \$\begingroup\$ @dfhwze how do you even spot these whitespaces? haha \$\endgroup\$ – t3chb0t Aug 23 at 7:02
  • \$\begingroup\$ That's what I do o_O I spot zombies and white space :p \$\endgroup\$ – dfhwze Aug 23 at 7:02
  • \$\begingroup\$ For the record, I do check for null propagation operator ?.Count, though simply missed that, and thanks for your answer. \$\endgroup\$ – LGSon Aug 23 at 7:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.