current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
2

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?

share|improve this question
up vote 2 down vote

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);
        }
    }
share|improve this answer
up vote 1 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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