3
\$\begingroup\$

I have created a ViewModel for a Product Edit page because it has specific related entities I need to load - the class currently looks like this:

public class ProductEditView
{
    public ProductEditView(Product product)
    {
        this.ID = product.ID;
        this.Code = product.Code;
        this.ProductLanguage = product.ProductLanguages.Where(x => x.LanguageID == Global.Language).FirstOrDefault();
        this.PrimaProduct = product.PrimaProduct;
        this.Multimedias = product.Multimedias;
    }

    public int ID { get; set; }
    public string Code { get; set; }
    public ProductLanguage ProductLanguage { get; set; }
    public PrimaProduct PrimaProduct { get; set; }
    public IEnumerable<Multimedia> Multimedias { get; set; }

}

So then in my controller I can go:

public ActionResult Edit(int? id)
{
    if (id == null) return new HttpStatusCodeResult(HttpStatusCode.BadRequest);

    Product product = db.Products.Find(id);
    if (product == null) return HttpNotFound();

    ProductEditView productEditView = new ProductEditView(product);
    return View(productEditView);
}

I've not really seen anybody do it like this though, is it bad practice - and for what reason? What are the better alternatives to accomplish this if this is wrong?

\$\endgroup\$
2
\$\begingroup\$

Its a very tight coupling between HTTP and your domain. However I do not like classic layered architecture. I often utilizes CQS for my service API's

Your example would look like

public class GetProductQueryHandler: IQueryHandler<GetProductQuery, Product>
{
   private readonly DbContext db;

   public GetProductQueryHandler(DbContext db) {
      this.db = db;
   }    
   public async Task<Product> Handle(GetProductQuerycommand query) {
      if (!query.Id.HasValue) throw new ArgumentExpection("Id is required"); //Not accepting null for a Nullable is a bit strange

      var product = await db.Products.FindAsync(id);
      if (product == null) throw new NotFoundExcpetion("Product");

      return product;
   }
}

You can then translate the errors into HTTP codes using a Exception filter, here is an example from my current project

    public class ExceptionFilter : ExceptionFilterAttribute 
    {
        public override void OnException(HttpActionExecutedContext actionExecutedContext)
        {
            base.OnException(actionExecutedContext);

            var message = "Server error";
            var details = "Please contact support if the problem persists";

            var code = HttpStatusCode.InternalServerError;

            if(actionExecutedContext.Exception is ArgumentException)
            {
                code = HttpStatusCode.BadRequest;
                message = "Bad Arguments";
                details = actionExecutedContext.Exception.Message;
            }

#if DEBUG
    details =   string.Format("{0}: {1}", actionExecutedContext.Exception.Message, actionExecutedContext.Exception.StackTrace);
#endif

            var response = new
            {
                Message = message,
                Details = details
            };

            actionExecutedContext.Response = actionExecutedContext.Request.CreateResponse(code, response);
        }
    }
\$\endgroup\$
1
\$\begingroup\$

The way you're doing it at the moment is fine, though it creates a bond between Product and ProductEditView.

I'd consider your constructor more like an helper method than a necessity, considering all the properties are public get/set (which is very good in the case of a view).

You just need to consider if ProductEditView could ever be built from information that wouldn't arrive from a Product. Now, I've typed this sentence and realized it makes no sense :p

Your way is fine.

Instead of .Where(x => x.LanguageID == Global.Language).FirstOrDefault(); you should use .FirstOrDefault(x => x.LanguageID == Global.Language) which has the same behavior but it's clearer! :)

Also, in a case like this : ProductEditView productEditView = new ProductEditView(product); Consider using var instead of ProductEditView, it shortens the code and it is okay to use var in such a case because you can see what is the type of productEditView just by looking at the code.

\$\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.