Possible bad practice in Web API ViewModel pattern?

Question

I'm currently implementing ViewModels within my WebApi by injecting a Model object into the constructor of my 'ViewModelProduct' Object, as shown:

public class ViewModelProduct
{
    private IProduct _product;

    private int _ID;
    private string _name;

    public ViewModelProduct(IProduct product)
    {
        _product = product;

        ID = _product.ID;
        Name = _product.Name;
    }

    public int ID
    {
        set { _ID = _product.ID; }
        get { return _ID; }
    }

    public string Name
    {
        set { _name = value; }
        get { return _name;}
    }

    public string Description
    {
        set { _product.Description = value; }
        get { return _product.Description; }
    }

Within the Controller - in my case, the 'ProductController' I want to create an instance of the 'ViewModelProduct'. I want to reduce close coupling where ever possible.

I inject an 'IFacade' object into the constructor of my controller through the use of Unity from a BootStrapper class.

The real question here is I currently have a static method simply returning a new instance of the 'ViewModelProduct' object which I send the Model to to set within its constructor, as shown. Is this bad practice? I can't think how I could create an instance with Unity as I dont know what the model will be before runtime

public List<ViewModelProduct> GetProducts()
{
    var V2Ops = _facade.GetOperatorV2();

    var productList = V2Ops.GetProducts();

    List<ViewModelProduct> listObjects = new List<ViewModelProduct>();

    foreach (var product in productList)
    {
        //*****Setting a new instance from a static method.*****
        var viewModel = CreateNewViewModelV2.CreateViewModel(product);

        listObjects.Add(viewModel);
    }

    return listObjects;
}

Static Class returning a new 'ViewModelProduct' instance:

public static ViewModelProduct CreateViewModel(IProduct passedProductModel)
{
    return new ViewModelProduct(passedProductModel);
}

Answer 1

It's not bad practice, I actually do it all the time but as an extension method (for IProduct). However, in this particular case, do you really need a factory method? Just doing a new ViewModelProduct(product) should be enough.

Your other option though is not quite good. It's a view model, a DTO for an IProduct, using a DI Container is way overkill and has no benefits. Btw, I don't think the viewmodel needs to be abstracted. As data structure it has no real behaviour (at most some helpers), it's not like you'll have multiple variations of it.

Answer 2

Another way I have gone about this, is as follows:

 public static IViewModelProduct CreateViewModel(IProduct passedProductModel)
    {
        var viewModelContainer = new UnityContainer();

        viewModelContainer.RegisterType<IViewModelProduct, ViewModelProduct>(new InjectionConstructor(passedProductModel));

        var newViewModelObject = viewModelContainer.Resolve<IViewModelProduct>();

        return newViewModelObject;
    }

Simply using unity to construct an object from the interface base class. Whilst registering the type, the passed IProduct object has been set to the constructor of the new ViewModelProduct Instance.