Taking advantage of inheritance in Controllers and Views

user_8211528 0
Viewed 2252

I had posted this review on codereview.stackexchange.com a while ago... I feel it may be more suitable for stackoverflow, since it is more of a question than a code review.

Its going to take a bit of explanation, please bear with me.

I am developing an e-commerce website in ASP.NET MVC. Users can post advertisements of different types on the site.

I am using inheritance to define my Ad types, and this question is about taking advantage of the hierarchical structure to remove repeated code in Controllers and Views.

I have different Ad types: SimpleAd, Car and RealEstateRental.

Every Ad is derived from AdBase which has all the common properties:

public abstract class AdBase
{
    public long AdBaseId { get; set; }
    public bool IsActive { get; set; }
    public long UserId { get; set; }
    public string Title { get; set; }
    public short AdDurationInDays { get; set; }
    public string PhotosFolder { get; set; }
}

Now other Ads are derived from this base class:

public class SimpleAd : AdBase
{
    public decimal Price { get; set; }
}

public class Car : AdBase
{
    public decimal Price { get; set; }
    public string Make { get; set; }
}

public class RealEstateRental : AdBase
{
    public decimal WeeklyRent { get; set; }
    public DateTime AvailableFrom { get; set; }
    public short NoOfBedrooms { get; set; }
    public short NoOfBathrooms { get; set; }
}

I am using Entity Framework to interact with database and I am using Unit of Work and Repository patterns:

I have a generic AdBaseRepository with all the common ad methods:

public abstract class AdBaseRepository<TEntity> where TEntity : AdBase
{
    protected readonly ApplicationDbContext Context;

    public AdBaseRepository(ApplicationDbContext context)
    {
       Context = context; 
    }

    public TEntity Get(long adBaseId)
    {
        return Context.AdBase.OfType<TEntity>()
                  .Where(r => r.IsActive == true && r.AdBaseId == adBaseId)
                  .FirstOrDefault();
    }

    // more common methods here...
}

Other ad repositories inherit from the above class:

public class SimpleAdRepository : AdBaseRepository<SimpleAd>
{
    public SimpleAdRepository(ApplicationDbContext context) : base(context)
    {
    }
}

public class CarRepository : AdBaseRepository<Car>
{
    public CarRepository(ApplicationDbContext context) : base(context)
    {
    }

    // methods which apply only to car here...
}

And this is my Unit of Work:

public class UnitOfWork
{
    protected readonly ApplicationDbContext Context;

    public UnitOfWork(ApplicationDbContext context)
    {
        Context = context;
        SimpleAd = new SimpleAdRepository(Context);
        RealEstateRental = new RealEstateRentalRepository(Context);
        Car = new CarRepository(Context);
    }

    public SimpleAdRepository SimpleAd { get; private set; }
    public RealEstateRentalRepository RealEstateRental { get; private set; }
    public CarRepository Car { get; private set; }

    public int SaveChanges()
    {
        return Context.SaveChanges();
    }

    public void Dispose()
    {
        Context.Dispose();
    }
}

I am happy with everything so far... but the problem is I don't know how I can take advantage of this inheritance hierarchy in my Controllers and Views.

At the moment, I have 3 Controllers: SimpleAdController, CarController and RealEstateRentalController:

public class SimpleAdController : ControllerBase
{
    private UnitOfWork _unitOfWork;

    public SimpleAdController(UnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    [HttpGet]
    // display specific ad
    public ActionResult Display(long id)
    {
        SimpleAd simpleAd = _unitOfWork.SimpleAd.Get(id);
        /* 
         * I have not included my ViewModel Classes in this question to keep
         * it small, but the ViewModels follow the same inheritance pattern
         */
        var simpleAdDetailsViewModel = Mapper.Map<SimpleAdDetailsViewModel>(simpleAd);
        return View(simpleAdDetailsViewModel);
    }
}

CarController and RealEstateRentalController have the same Display method, except the type of the Ad is different (e.g. in CarController I have):

    public ActionResult Display(long id)
    {
        Car car = _unitOfWork.Car.Get(id);
        var carViewModel = Mapper.Map<CarViewModel>(car);
        return View(car);
    }

What I wanted to achieve was to create an AdBaseController to put all the common methods in it, something like this:

public class AdBaseController : ControllerBase
{
    private UnitOfWork _unitOfWork;

    public AdBaseController(UnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    // Display for generic ad type
    [HttpGet]
    public ActionResult Display(long id)
    {
        // SimpleAd simpleAd = _unitOfWork.SimpleAd.Get(id);
        /* 
         * I need to replace the above line with a generic ad type... 
         * something like: _unitOfWork<TAd>.GenericAdRepository.Get(id)
         */

        // var simpleAdDetailsViewModel = Mapper.Map<SimpleAdDetailsViewModel>(simpleAd);
        // return View(simpleAdDetailsViewModel);
        /* 
         * similarly I have to replace the above 2 lines with a generic type
         */
    }
}

If I do the above, then my Ad Controllers can inherit from it and I don't need to repeat the same Display Method in every one of them... but then I need to make my UnitOfWork generic... or have 2 UoW (generic and non-generic)... which I am not sure if it is a good idea? Any recommendation on having a AdBaseController?

Similarly I am repeating a lot of code in my Views. For example, this is the display SimpleAdView:

<div class="row">
    <div class="col-l">
        @*this partial view shows Ad photos and is common code for all ad types*@
        @Html.Partial("DisplayAd/_Photos", Model)
    </div>
    <div class="col-r">
        <div class="form-row">
            @*Common in all ads*@
            <h5>@Model.Title</h5>
        </div>

        @*showing ad specific fields here*@
        <div class="form-row">
            <h5 class="price">$@Model.Price</h5>
        </div>

        @*Ad heading is common among all ad types*@
        @Html.Partial("DisplayAd/_AdBaseHeading", Model)
    </div>
</div>
@*Ad Description is common among all ad types*@
@Html.Partial("DisplayAd/_Description", Model)

And this is my display CarView:

<div class="row">
    <div class="col-l">
        @*Common in all ads*@
        @Html.Partial("DisplayAd/_Photos", Model)
    </div>
    <div class="col-r">
        <div class="form-row">
            @*Common in all ads*@
            <h5>@Model.Title</h5>
        </div>

       @*Price and Make are specific to Car*@ 
        <div class="form-row">
            <h5 class="price">$@Model.Price</h5>
        </div>
        <div class="form-row">
            <h5 class="make">@Model.Make</h5>
        </div>

        @*Common in all ads*@ 
        @Html.Partial("DisplayAd/_AdBaseHeading", Model)
    </div>
</div>
@*Common in all ads*@
@Html.Partial("DisplayAd/_Description", Model)

Again, I feel like I am repeating a lot of code in each view. I have tried to reduce the amount of repeated code by putting them in common Partial Views. I am not sure if there is a better way to do this?

unit-of-work repository inheritance asp.net-mvc oop
3 Answers

Technically it is possible. For similar entities you can introduce enumeration and use it to indicate what entity type you deal with in controller. You can create generic view to handle similar ads (but of course you will need to show/hide corresponding UI elements depending on the model ad type). this is the pseudo code for controller to illustrate the idea:

using System.Threading.Tasks;
using AutoMapper;
using MyNamespace.Data;
using Microsoft.AspNetCore.Mvc;
using MyNamespace.ViewModels;

namespace MyNamespace
{
    public enum AdType
    {
        [Description("Simple Ad")]
        SimpleAd = 0,

        [Description("Car")]
        Car = 1,

        [Description("Real Estate Rental")]
        RealEstateRental = 2
    }

    public class AdController : Controller
    {
        private readonly ApplicationDbContext _context;
        private readonly IMapper _mapper;

        public AdController(
            ApplicationDbContext context,
            IMapper mapper)
        {
            _context = context;
            _mapper = mapper;
        }

        [HttpGet("Ad/{type}")]
        public IActionResult Index(AdType? type = AdType.SimpleAd)
        {
            switch (type)
            {
                case AdType.RealEstateRental:
                    return RedirectToAction("RealEstateRental");
                case AdType.Car:
                    return RedirectToAction("Car");
                case AdType.SimpleAd:
                default:
                    return RedirectToAction("SimpleAd");
            }
        }

        [HttpGet("Ad/Car")]
        public IActionResult Car()
        {
            return View("Index", AdType.Car);
        }

        [HttpGet("Ad/RealEstateRental")]
        public IActionResult RealEstateRental()
        {
            return View("Index", AdType.RealEstateRental);
        }

        [HttpGet("Ad/SimpleAd")]
        public IActionResult SimpleAd()
        {
            return View("Index", AdType.SimpleAd);
        }

        [HttpGet("Ad/List/{type}")]
        public async Task<IActionResult> List(AdType type)
        {
            // var list = ... switch to retrieve list of ads via switch and generic data access methods 
            return list;
        }

        [HttpGet("Ad/{type}/Details/{id}")]
        public async Task<IActionResult> Details(AdType type, int id)
        {
            var ad = // ... switch by type to retrieve list of ads via switch and generic data access methods
            if (ad == null) return NotFound($"Ad not found.");

            // for instance - configure mappings via Automapper from DB entity to model views
            var model = _mapper.Map<AdViewModel>(ad);

            // Note: view will have to detect the exact ad instance type and show/hide corresponding UI fields
            return View(model);
        }

        [HttpGet("Ad/{type}/Add/")]
        public IActionResult Add(AdType type)
        {
            var ad = // ... switch by type to validate/add new entity  

            return View(_mapper.Map<AdEditModel>(ad));
        }

        [HttpPost("Ad/{type}/Add/")]
        public async Task<IActionResult> Add(AdEditModel model)
        {
            // detect ad type and save 
            return View(model);
        }

        [HttpGet("Ad/{type}/Edit/{id}")]
        public async Task<IActionResult> Edit(AdType type, int id)
        {
            // similar to Add
            return View(model);
        }

        [HttpPost("Ad/{type}/Edit/{id}")]
        public async Task<IActionResult> Edit(AdEditModel model)
        {
            // similar to Add
            return View(model);
        }

        // And so on
    }
}

But I should note, that inheritance in UI related code eventually results into more problems than benefits. The code becomes more complex to maintain and keep it clean. So it makes more sense to keep all your Views and Controllers separate, even if they have the code very close to each other. You could start optimiziong the "repeated code" usage below the your DI services (aka business logic) or similar layer.

The repeated code problem for UI level should be solved via extracting components (aka controls, partial views, view components). Controller inheritance is possible but make code harder to maintain.

user_8043850

More abstraction -> more abstraction leaks.

I have complete solution how to generate controllers from EF model definition using exression trees

Check this, how the controller's code looks after all "duplicated code" is removed:

https://github.com/DashboardCode/Routines/blob/master/AdminkaV1/Injected.AspCore.MvcApp/Controllers/UsersController.cs

or this ("Roles" can be created when "Users" was imported from AD )

https://github.com/DashboardCode/Routines/blob/master/AdminkaV1/Injected.AspCore.MvcApp/Controllers/RolesController.cs

Those blocks on start configures full controller with a lot of features (e.g. rowversion support, sql server constraints error parsers and etc., one-to many, many-to-many, unhandled expceptions support)

static ControllerMeta<User, int> meta = new ControllerMeta<User, int>(
            // how to find entity by "id"      
            findByIdExpression: id => e => e.UserId == id,
            // how to extract "id" from http responce      
            keyConverter: Converters.TryParseInt,
            // configure EF includes for Index page
            indexIncludes: chain => chain
                       .IncludeAll(e => e.UserPrivilegeMap)
            // ... and so on, try to read it

But those definitions actually is a kind of new Internal DSL. In fact you are asking "how to write new DSL that defines controllers/pages in bigger bricks". Answer is - it is easy, but there is a reason why people stick to general purpose languages. It is because it is "general".

P.S. One detail: if you want that "full controller" could be contracted/configured at run time, therefore you are forced to parse http requests by youself - and ignore MS parameters binding model - it is because BindAttribute - important binding modificator - can't be "set up" run time simple way. For many people - even when they loose "int id" in parameters list - is too high price. Even if refusing of MS parameters binding is very logical: why would you need to keep MS parameters binding magic when you are going to configure whole controller magically?

user_5061470

Forgive me if I've misunderstood but provided you added a genric UOW it seems to me you could do something like this: I don't see why it would be bad to do this

public class AdBaseController : ControllerBase
{
    private IUnitOfWork _unitOfWork;

    public AdBaseController(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork;
    }

    public ActionResult GetDisplayAction<TAd, TViewModel>(long id)
    {
        SimpleAd simpleAd = _unitOfWork<TAd>.GenericAdRepository.Get(id)
        var viewModel = Mapper.Map<TViewModel>(simpleAd);         
        return View(viewModel);
    }
}

public class SimpleAdController : ControllerBase
{    
    public SimpleAdController(IUnitOfWork unitOfWork) : base(unitOfWork)
    {
    }

    [HttpGet]
    public ActionResult Display(long id)
    {
        return GetDisplayAction<AdType, ViewModelType>();
    }
}
user_32170100
Related