ASP.NET Product Listing

Question

I am working on a e-shop in ASP.NET MVC, and I just made the listing of product the way I figured out this night. Is it correct? Or should I make it differently? How?

So this is my Product controller, used for displaying the products from Db.

using WebShop.Models;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.Mvc;

namespace WebShop.Controllers
{
    public class ProductController : Controller
    {
        private WebShopEntities db = new WebShopEntities();

        public ActionResult Category(int? id)
        {
            int pid = id == null ? 1 : (int)id;  //if requested id is null, set is as 1
            if (db.Categories.Find(pid) == null)  //if category does not exist, return Error
                return View("Error");
            var model = db.Categories.Find(pid).Products.ToList(); //return list of projects in requested category
            return View(model);
        }
    }
}

This is the Category View.

@using WebShop.Models
    @{
        ViewBag.Title = "Category";
        Layout = "~/Views/Shared/_Layout.cshtml";
    }
    <div class="vert-offset-top-2">
        <div class="list-group col-sm-3">
            @Html.Partial("Partial/_PartialCategoryMenu")  //display partial view for category menu
        </div>
        <div class="row">
            @foreach(Product p in Model)
            {
                @Html.Partial("Partial/_PartialProduct", p)  //display partial view
            }
        </div>
    </div>

These are partial Views, which are self-explanatory I hope. Category partial view

@using WebShop.Models

@using (var db = new WebShopEntities()) {
foreach (var category in db.Categories)
{
    <a href="@Url.Action("Category","Product", new {id = category.id})" class="list-group-item">@category.name</a>
}
}

Product partial view

<div class="vert-offset-bottom-1 col-sm-3">
    <div class="thumbnail">
        <img src="" class="img-responsive" alt="Product image">
        <div class="caption">
            <h3>@Model.name</h3>
            <h4>@Model.price €</h4>
            <p>@Model.description</p>
            <p><a href="#" class="btn btn-primary" role="button">To Cart</a> <a href="@Url.Action("Details", "Product", new {id = @Model.id})" class="btn btn-default" role="button">Details</a></p>
        </div>
    </div>
</div>

Answer 1

I would definitely remove the database access from the Category partial view and move it to the Controller (you can add it to the view model or have a separate action for returning them which is even more clean).

Views are supposed to be dumb layouts and not perform any business logic. Removing the database context shouldn't affect the compilation of the views.

Small refactoring suggestion for the Controller:

    public ActionResult Category(int? id)
    {
        var pid = id ?? 1;  //if requested id is null, set is as 1

        var category = db.Categories.Find(pid);

        if (category == null)  //if category does not exist, return Error
            return View("Error");

        var model = category.Products.ToList(); //return list of projects in requested category

        return View(model);
    }

Don't call find twice will save an extra database access which is important and code style wise I would use the ?? operator as you see above.

What the View("Error") is doing? Do you have an error view or is it just raising an exception? In both cases I would be more specific.

I would also make the view strongly typed as it will prevent errors and give you IntelliSense.

Answer 2

Building on Uno's answer, I would recommend the following refactor for the controller action:

public ActionResult Category(int? id)
{
    // No need to create a pid variable if it's only 
    // used once here. Inline the null conditional.
    var category = db.Categories.Find(id ?? 1);

    if (category == null) 
    {
        // Assuming you have a global error handler attribute registered,
        // the condition where a requested category is not found is in fact
        // an exceptional state and the HTTP 404 status code is semantically
        // more intuitive than returning a generic error view.
        throw new HttpException(404, "Category not found.");
    }

    // I too tend to avoid returning IEnumerables outside of the context
    // because sometimes the enumeration depends on something that falls 
    // out of scope (like the data context). But I would typically avoid
    // the conversion to a list until the last possible moment.
    // If you need to come back and do some logic with the model later,
    // you can add it before returning the view and work directly with
    // the enumerable which is probably preferable in most cases.
    var model = category.Products;
    return View(model.ToList());
}

As for separating the category partial into a controller action:

Category list action:

public ActionResult CategoryList() 
{
    @using (var db = new WebShopEntities()) 
    {
        return PartialView("CategoryList", db.Categories.ToList());
    }
}

Partial view to go with:

@model List<Category>

foreach (var category in Model)
{
    <a href="@Url.Action("Category","Product", new {id = category.id})" class="list-group-item">@category.name</a>
}

You can render this in any view using

@Html.RenderAction("CategoryList")

Answer 3

You're going to want to use dependency injection instead of hardcoding the implementation being used by the controller.

private IWebShopEntities db;

public ProductController(IWebShopEntities db)
{
    this.db = db;
}

This way you can implement different versions as needed, for testing, for adding a new service that does special caching, etc. By making this constructor, you indicate to the framework that it needs to pass you the appropriate instance, which it will do however you've configured it.

How exactly you do that depends on the framework version you use; for example, using MVC Next and ASP.NET Core, I do something like this in my Startup.cs

public void ConfigureServices(IServiceCollection services)
{
    services.AddOptions();
    services.AddTransient<IWebShopEntitites, WebShowEntitites>();
    services.AddMvc();
}