Support ticket application: ticket listing index with sorting and filtering

Question

I am new to ASP.NET (C#) and MVC 5. I'm really enjoying it so far, but want to take a step back and make sure that I'm following best practices here. My controller seems like it may be getting too fat, but perhaps that's just part of it?

This is the Index action for the TicketsController in a support ticket application that I'm building to familiarize myself with MVC5. Everything works just fine, but it's starting to smell and I am looking for feedback.

TicketsController -> Index:

    /// GET /Tickets/
    public ActionResult Index(string sortOrder, int? ShopFilter, bool showAll = false)
    {
        TicketIndexViewModel ticketIndexVM = new TicketIndexViewModel();

        ticketIndexVM.SortOrder = sortOrder;
        ticketIndexVM.IdSortParam = String.IsNullOrEmpty(sortOrder) ? "id_asc" : "";
        ticketIndexVM.ShopNameSortParam = sortOrder == "shop_desc" ? "shop_asc" : "shop_desc";
        ticketIndexVM.StatusSortParam = sortOrder == "status_desc" ? "status_asc" : "status_desc";
        ticketIndexVM.ShowAllParam = showAll ? "true" : "";
        ticketIndexVM.ShopFilterParam = ShopFilter;

        ticketIndexVM.ShopListing = Mapper.Map(db.Shops, new List<ShopViewModel>());

        var tickets = from t in db.Tickets.Include(t => t.User).Include(t => t.Shop)
                      select t;

        // hide completed and cancelled tickets
        if(!showAll)
        {
            tickets = tickets.Where(t => t.Status < 3);
        }

        // shop filtering
        if(ShopFilter != null)
        {
            var shopById = db.Shops.Where(s => s.ShopId == ShopFilter);

            if (shopById.Count() > 0)
            {
                tickets = tickets.Where(t => t.ShopId == ShopFilter);
            }
            else
            {
                // that shop doesn't exist.
                ticketIndexVM.ShopFilterParam = null;
                ticketIndexVM.Errors.Add(
                    String.Format("No shop exists with the ID# {0}", ShopFilter));
            }
        }

        // handle sorting
        switch (sortOrder)
        {
            case "id_asc":
                tickets = tickets.OrderBy(t => t.TicketId);
                break;
            case "shop_desc":
                tickets = tickets.OrderByDescending(t => t.Shop.Name);
                break;
            case "shop_asc":
                tickets = tickets.OrderBy(t => t.Shop.Name);
                break;
            case "status_desc":
                tickets = tickets.OrderByDescending(t => t.Status);
                break;
            case "status_asc":
                tickets = tickets.OrderBy(t => t.Status);
                break;
            default:
                tickets = tickets.OrderByDescending(t => t.TicketId);
                break;
        }

        var ticketsVM = Mapper.Map(tickets, new List<TicketViewModel>());

        ticketIndexVM.Tickets = ticketsVM;

        return View(ticketIndexVM);
    }

Tickets/Index.cshtml

@model Tots.Models.ViewModels.TicketIndexViewModel

@{
    ViewBag.Title = "Tickets";
}

@if(Model.ShowAllParam == "true")
{
    <h1>All Tickets</h1>
}
else
{
    <h1>Open Tickets</h1>
}

<div class="page_toolbar text-right">
    @if (Model.ShowAllParam == "true")
    {
        @Html.ActionLink("Show Open Only", "Index", null, new { @class = "btn btn-success" })
    }
    else
    {
        @Html.ActionLink("Show All", "Index", new { showAll = true }, new { @class = "btn btn-success" })
    }
    @Html.ActionLink("Create New", "Create", null, new { @class = "btn btn-primary" })
</div>

@* Error placeholder: *@
@Html.Partial("_Error", @Model)

<div class="row no-gutters">
    <div class="col-md-12">
        <p class="result-count">
            Displaying <strong>@Model.Tickets.Count</strong> results...
        </p>
    </div>
</div>

<table class="table">
    <thead>
        <tr>
            <th>
                @Html.ActionLink("Ticket ID#", "Index", new { sortOrder = @Model.IdSortParam, showAll = @Model.ShowAllParam, ShopFilter = @Model.ShopFilterParam })
            </th>
            <th>
                Date
            </th>
            <th>
                Submitted By
            </th>
            <th>
                @Html.ActionLink("Shop Name", "Index", new { sortOrder = @Model.ShopNameSortParam, showAll = @Model.ShowAllParam, ShopFilter = @Model.ShopFilterParam })
            </th>
            <th>
                Description
            </th>
            <th>
                @Html.ActionLink("Status", "Index", new { sortOrder = @Model.StatusSortParam, showAll = @Model.ShowAllParam, ShopFilter = @Model.ShopFilterParam })
            </th>
            <th></th>
        </tr>
    </thead>
    <tbody>

@Html.DisplayFor(x => x.Tickets)
    </tbody>
</table>

Answer 1

I think methods for interacting with db classes, manipulating and filtering data belong in the model.

So if there is already an appropriate model class for the controller then move the code there or create a new model to service the controller.

I would create a model TicketsService and a method inside it to return tickets; something like the following -

public class TicketService 
{

public TicketIndexViewModel getTickets (string sortOrder, int? ShopFilter,     bool showAll = false) {

TicketIndexViewModel ticketIndexVM = new TicketIndexViewModel();

    ticketIndexVM.SortOrder = sortOrder;
    ticketIndexVM.IdSortParam = String.IsNullOrEmpty(sortOrder) ? "id_asc" : "";
    ticketIndexVM.ShopNameSortParam = sortOrder == "shop_desc" ? "shop_asc" : "shop_desc";
    ticketIndexVM.StatusSortParam = sortOrder == "status_desc" ? "status_asc" : "status_desc";
    ticketIndexVM.ShowAllParam = showAll ? "true" : "";
    ticketIndexVM.ShopFilterParam = ShopFilter;

    ticketIndexVM.ShopListing = Mapper.Map(db.Shops, new List<ShopViewModel>());

    var tickets = from t in db.Tickets.Include(t => t.User).Include(t => t.Shop)
                  select t;

    // hide completed and cancelled tickets
    if(!showAll)
    {
        tickets = tickets.Where(t => t.Status < 3);
    }

    // shop filtering
    if(ShopFilter != null)
    {
        var shopById = db.Shops.Where(s => s.ShopId == ShopFilter);

        if (shopById.Count() > 0)
        {
            tickets = tickets.Where(t => t.ShopId == ShopFilter);
        }
        else
        {
            // that shop doesn't exist.
            ticketIndexVM.ShopFilterParam = null;
            ticketIndexVM.Errors.Add(
                String.Format("No shop exists with the ID# {0}", ShopFilter));
        }
    }

    // handle sorting
    switch (sortOrder)
    {
        case "id_asc":
            tickets = tickets.OrderBy(t => t.TicketId);
            break;
        case "shop_desc":
            tickets = tickets.OrderByDescending(t => t.Shop.Name);
            break;
        case "shop_asc":
            tickets = tickets.OrderBy(t => t.Shop.Name);
            break;
        case "status_desc":
            tickets = tickets.OrderByDescending(t => t.Status);
            break;
        case "status_asc":
            tickets = tickets.OrderBy(t => t.Status);
            break;
        default:
            tickets = tickets.OrderByDescending(t => t.TicketId);
            break;
    }

    var ticketsVM = Mapper.Map(tickets, new List<TicketViewModel>());

    ticketIndexVM.Tickets = ticketsVM;

    return ticketIndexVM; 



}

}

And my controller method would look something like -

public ActionResult Index(string sortOrder, int? ShopFilter, bool showAll = false)
{

return View(new TiceketService().getTickets(sortOrder,ShopFilter,showAll));
}

On a side note : You might want to look into the dependency injection design pattern. I have been following it in my project and it has worked well for me.

You can read more about it here

http://www.asp.net/mvc/overview/older-versions/hands-on-labs/aspnet-mvc-4-dependency-injection

Hope this helps.

Answer 2

I have multiple issues with this part of the code:

ticketIndexVM.IdSortParam = String.IsNullOrEmpty(sortOrder) ? "id_asc" : "";
ticketIndexVM.ShopNameSortParam = sortOrder == "shop_desc" ? "shop_asc" : "shop_desc";
ticketIndexVM.StatusSortParam = sortOrder == "status_desc" ? "status_asc" : "status_desc";
ticketIndexVM.ShowAllParam = showAll ? "true" : "";

First of all, there's the inconsistent behavior: in one case the IdSortParam is either "id_asc" or it's an empty string, whereas in two other cases it's a name plus a relevant suffix, e.g. "shop_asc" or "shop_desc".

Equally inconsistent is how you fill ShowAllParam: this can contain "true" or an empty string. That's not an expected behavior. Also, it's bad enough that a bool has to be converted to a string, so why not use the available tools, i.e. change this line to ticketIndexVM.ShowAllParam = showAll.ToString().ToLowerInvariant();?

---

There is also the presence of several magic strings, which reappear elsewhere in your code (the switch). This indicates to me that these values should be const strings in a separate class, so that should there ever be a need to change them you only need to do this in one place.

I would even consider something like this:

public static class SortOrder
{
    private const string AscSuffix = "asc";
    private const string DescSuffix = "desc";
    private const string Delimiter = "_";

    public static class Shop
    {
        private const string Id = "shop";

        public const string Asc = Id + Delimiter + AscSuffix;
        public const string Desc = Id + Delimiter + DescSuffix;
    }

    public static class Status
    {
        private const string Id = "status";

        public const string Asc = Id + Delimiter + AscSuffix;
        public const string Desc = Id + Delimiter + DescSuffix;
    }
}

Though I have to say there's still too much repetition there and thus room for error, but I fear that's inevitable when you want to stick to const strings.

I would also consider moving repeated logic like this: sortOrder == "shop_desc" ? "shop_asc" : "shop_desc"; and sortOrder == "status_desc" ? "status_asc" : "status_desc";. There's a lot of repetition there, which suggests it could be moved to a method, something like this:

public string GetSortOrder(string itemType, string sortOrder)
{
    return sortOrder.EndsWith(SortOrder.DescSuffix) 
        ? itemType + SortOrder.Delimiter + SortOrder.AscSuffix 
        : itemType + SortOrder.Delimiter + SortOrder.DescSuffix;
}

Note that these are some ideas I quickly whipped up now, so don't just blindly copy-paste them. They need to be considered because right now there's too much repetition. Ideally I'd like to stick with const strings, but perhaps there's more "profit" to be had by having one method that contains the concatenation logic, combined with a class with some useful consts.

The most important message is: move away from magic strings, especially if they're used repeatedly.

---

You also have a magic number: Status < 3. "3" doesn't tell me anything, which you acknowledge by adding the comment // hide completed and cancelled tickets. IMHO Status should be an enum and 3 one of its values.

I'd also be careful with logic like Status < 3: what if you need to add a new status? I would do this: Status != TicketStatus.Cancelled && Status != TicketStatus.Completed.