Potential Improvement to ProductController and its method PrepareProductDetailsPageModel

I would like to suggest a potential refactoring of the ProductController.

The ProductController currently contains a method named PrepareProductDetailsPageModel(...) used to convert a Product EF model to a ProductDetailsModel view model ready for a ProductTemplate view.

The responsibility of the controller is to receive requests, handle the activities based on the request parameters, delegate the tasks to be performed and handle the next view to be shown.  The responsiblity of this PrepareProductDetailsPageModel is to convert a model to a view model.  Therefore, I would argue that this responsibility should be moved outside of the ProductController.

Potentially, this method could be moved to a (lets call it) ProductDetailsModelFactory say:

public class ProductDetailsModelFactory : IProductDetailsModelFactory
{
   protected virtual ProductDetailsModel PrepareProductDetailsPageModel(Product product, ShoppingCartItem updatecartitem = null, bool isAssociatedProduct = false)
   {
      // existing code here
   }
}

As the existing code is approximately 700 lines long, this would greatly simplify the controller (potentially halving the size of it).  Additionally, the following dependencies (constructor injected) could potenitally be removed from the controller as they would instead be required by the ProductDetailsModelFactory.

manufacturerService
vendorService
productTemplateService
productAttributeService
productTagService
downloadService
productAttributeParser
shippingService
vendorSettings
seoSettings

Going one step further, the existing (approx) 700 line block of code for PrepareProductDetailsPageModel could be broken down further into methods within the ProductDetailsModelFactory.  For example:

public class ProductDetailsModelFactory : IProductDetailsModelFactory
{
   protected virtual ProductDetailsModel PrepareProductDetailsPageModel(Product product, ShoppingCartItem updatecartitem = null, bool isAssociatedProduct = false)
   {
      // simplified code
      
      var somePictureModel = GetPictureModel();
      //use somePictureModel to populate ProductDetailsModel
   }

   protected virtual SomePictureModel GetPictureModel()
   {
      //existing picture code to get pictures
   }

   //other methods
}

This would greatly simplify the PrepareProductDetailsPageModel method and furthermore, would allow developers to override these routines.  If for example, a developer wanted to modify the way that pictures were retrieved (as I did before writing this post), I believe they would find it very difficult with the currently solution.  With the proposal above, a developer could inherit from ProductDetailsModelFactory, override the GetPictureModel() method and alter as required and set up their new factory to be injected into the controller instead.

I would be interested to know what other developers who have more experience with nopCommerce think about this and to hear some of the disadvantages that I may not have considered.

Additionally, in the PrepareProductDetailsPageModel method, I have noticed that the defaultPictureModel.Title and defaultPictureModel.AlternateText appears to be set twice.  For example:

   var defaultPictureModel = new PictureModel
   {
      ImageUrl = _pictureService.GetPictureUrl(defaultPicture, defaultPictureSize, !isAssociatedProduct),
      FullSizeImageUrl = _pictureService.GetPictureUrl(defaultPicture, 0, !isAssociatedProduct),
      Title = string.Format(_localizationService.GetResource("Media.Product.ImageLinkTitleFormat.Details"), model.Name),
      AlternateText = string.Format(_localizationService.GetResource("Media.Product.ImageAlternateTextFormat.Details"), model.Name),
   };
   //"title" attribute
   defaultPictureModel.Title = (defaultPicture != null && !string.IsNullOrEmpty(defaultPicture.TitleAttribute)) ?
      defaultPicture.TitleAttribute :
      string.Format(_localizationService.GetResource("Media.Product.ImageLinkTitleFormat.Details"), model.Name);
   //"alt" attribute
   defaultPictureModel.AlternateText = (defaultPicture != null && !string.IsNullOrEmpty(defaultPicture.AltAttribute)) ?
      defaultPicture.AltAttribute :
      string.Format(_localizationService.GetResource("Media.Product.ImageAlternateTextFormat.Details"), model.Name);

Notice that the Title and AlternateText set in the initialisation of the PictureModel is overriden in the lines that follow, therefore (I believe) the intial lines are not necessary.

This also seems to be the case for pictureModel.Title and pictureModel.AlternateText.

Nice refactoring :)

Hi David,

Thanks a lot for these suggestions! Here are the first and the second work items

a.m. wrote:

Wow!  Thanks for creating these.  I hope they help.

I would like to have the option to show all (or let's say 4) pictures and zoom functionality in the _ProductBox View for Featured products, now assuming I have modified ProductOverviewModel.cs and the appropriate View, how easy it would it be to modify the Product Controller to achieve this. At the moment one can only load the default picture?

Can I simply take the //all pictures region from PrepareProductDetialsModel and add that to the PrepareProductOverviewModels?

Jon.Hopley.Net wrote:

To clarify, I believe that you are trying to execute the CategoryController.Category() method but you are saying (that by default), the CategoryModel returned by this action will have a FeaturedProducts property that will contain featured products but only a single picture image for each product will be set.  You would like all pictures (or just 4 pictures) for each featured product to be set.

As you say, you could copy the code from the ProductController.PrepareProductDetailsModel() method to set additional images but personally, I try to avoid altering the existing source code to allow for easier upgrades in the future.

I would therefore leave the CategoryController.Category() action alone and let it be called and complete in its usual way.  I would then add an action filter to the method to add the additional images that you require after the action has complete.

An overview of how to do this can be found here: https://www.pronopcommerce.com/overriding-intercepting-nopcommerce-controllers-and-actions

As an example, your action filter might look like the following:

   public class FeaturedProductPicturesActionFilter : ActionFilterAttribute, IFilterProvider
   {
      public IEnumerable<Filter> GetFilters(ControllerContext controllerContext, ActionDescriptor actionDescriptor)
      {
         if (controllerContext.Controller is CatalogController &&
            actionDescriptor.ActionName.Equals("Category",
            StringComparison.InvariantCultureIgnoreCase))
         {
            return new List<Filter>() { new Filter(this, FilterScope.Action, 0) };
         }

         return new List<Filter>();
      }

      public override void OnActionExecuted(ActionExecutedContext filterContext)
      {
         var result = filterContext.Result as ViewResult;

         if (result != null) // then the view was returned and not a redirect
         {
            var categoryModel = (CategoryModel)result.Model;

            // now you have access to the categoryModel after the action has completed so you can add additional images to the
            // categoryModel.FeaturedProducts properties using code similar to that from the ProductController.PrepareProductDetailsModel()
            // n.b. you may want to consider caching those additional images also
         }

         base.OnActionExecuted(filterContext);
      }
   }

And finally, be careful altering the _ProductBox view as this is a shared view and therefore is used by other views in addition to CategoryTemplate.ProductsInGridsOrLines.  You may need to check for the presence of additional images in this model to display appropriately.

Hope this helps

Most helpful Thank you; I had not considered an action filter, I'll see if I can get it working.

Good news! We decided to go with model factories in the upcoming version 3.90. Already implemented! Please see "develop" branch on Github or find a list of commits referenced in this work item

More good news! I have made a plugin that enables multiple product picture for featured and new products available as a plugin. MegaMetro Plugin