1
\$\begingroup\$

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>
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for reply. What do you mean by making the view strongly typed? \$\endgroup\$ – Marinaro Jun 27 '16 at 13:05
  • \$\begingroup\$ One more thing. If I saved categories in ViewBag and then use that in the partial view to list categories, would it be appropriate solution? \$\endgroup\$ – Marinaro Jun 27 '16 at 13:15
  • \$\begingroup\$ @WannaBeGnome if you don't specify a model directive, the model will be considered dynamic on which you don't get IntelliSense and potentially you can use any property getting a runtime error if it doesn't exist. Have a look here: codereview.stackexchange.com/questions/133180/… \$\endgroup\$ – Stefano d'Antonio Jun 27 '16 at 13:22
  • \$\begingroup\$ Instead of using the ViewBag, you can add the categories to the viewmodel from the controller action and then pass it as model of the partial view; even better, you can invoke another controller action which returns the partial view and sets its content to the categories collection. \$\endgroup\$ – Stefano d'Antonio Jun 27 '16 at 13:24
  • \$\begingroup\$ Actually, at least if we're talking EF, then Find will cache the entity in-memory if it was found, so calling it twice won't be "expensive". I agree it'd be "better" to bind it to a local variable though. \$\endgroup\$ – sara Jun 27 '16 at 15:06
1
\$\begingroup\$

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")
\$\endgroup\$
0
\$\begingroup\$

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();
}
\$\endgroup\$

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.