MVC design pattern approaches

Question

I am actually more confused rather than understanding the concept. I have been developing a system and want to know the MVC design pattern. which one is best and the concept and usability of the layers?

In general, I have two approaches:

1.

ViewModel layer (contains viewmodels), Models (contains models and datacontext), Repository (communicating to datacontext), Controller (communicating to view), Presentation (views)

In general, if one page for example, country (crude operations) will be look like this

[AdminRoleAuthorize(Roles = "Admin")][CustomHandleError] 
public class CountriesController : BaseController
{
 #region Members
 private IUnitOfWork unitOfWork;
 #endregion Members
 #region Constructor
 public CountriesController(iMSTUnitOfWork _unitOfWork)
 {
 unitOfWork = _unitOfWork;
 }
 #endregion Constructor
 #region Actions
 public ActionResult Index()
 {
 return View();
 }
 public async Task<ActionResult> Details(int? id)
 {
 if (id == null)
 {
 return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
 }
 Country country = await unitOfWork.CountryRepository.GetByIdAsync((int)id);
 if (country == null)
 {
 return HttpNotFound();
 }
 return View(country);
 }
 public ActionResult Create()
 {
 return View();
 }
 [HttpPost]
 [ValidateAntiForgeryToken]
 public async Task<ActionResult> Create(Country country)
 {
 try
 { 
 if (ModelState.IsValid)
 {
 unitOfWork.CountryRepository.Insert(country);
 await unitOfWork.SaveAsync();
 return RedirectToAction("Index");
 }
 return View(country);
 }
 catch (Exception ex)
 {
 ModelState.HandleError(ex);
 return View(country);
 }
 }
 public async Task<ActionResult> Edit(int? id)
 {
 if (id == null)
 {
 return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
 }
 Country country = await unitOfWork.CountryRepository.GetByIdAsync((int)id);
 if (country == null)
 {
 return HttpNotFound();
 }
 return View(country);
 }
 [HttpPost]
 [ValidateAntiForgeryToken]
 public async Task<ActionResult> Edit(Country country)
 {
 try
 { 
 if (ModelState.IsValid)
 {
 var original= unitOfWork.CountryRepository.GetById(country.Id);
 unitOfWork.CountryRepository.Update(original);
 await unitOfWork.SaveAsync();
 return RedirectToAction("Index");
 }
 return View(country);
 }
 catch (Exception ex)
 {
 ModelState.HandleError(ex);
 return View(country);
 }
 }
 public async Task<ActionResult> Delete(int id)
 {
 await unitOfWork.CountryRepository.DeleteAsync(id);
 await unitOfWork.SaveAsync();
 return RedirectToAction("Index");
 }
 #endregion Actions
 #region DataTable
 public JsonResult GetJsonData(DTParameterModel param)
 {
 try
 {
 var results = GetCountry(param);
 return Json(results, JsonRequestBehavior.AllowGet);
 }
 catch (Exception ex)
 {
 return Json(new { error = ex.Message });
 }
 }
 #endregion DataTable
 #region Methods
 public DTResult<Country> GetCountry(DTParameterModel param)
 {
 var errors = unitOfWork.CountryRepository.GetQueryable().Where(a => (a.Name.Contains(param.Search.Value) || param.Search.Value == null) );
 return DataTableFiltering<Country>.GetResult(param, errors); 
 }
 protected override void Dispose(bool disposing)
 {
 if (disposing)
 {
 // unitOfWork.Dispose();
 }
 base.Dispose(disposing);
 }
 #endregion Methods
}

Please note that there is no business layer comes here and I am accessing repository directly.

2. ViewModel layer (contains viewmodels), Models (contains models and datacontext), Repository (communicating to datacontext), Controller (communicating to view), Presentation (views) Business layer

In this scenario my code will look like this:

public class ParticipationStatusController : BaseController
 {
 private readonly IParticipationStatusManager _participationStatusManager;
 public ParticipationStatusController(IParticipationStatusManager participationStatusManager, ISecurityManager securityManager) : base(securityManager)
 {
 _participationStatusManager = participationStatusManager;
 }
 #region NotUsed
 public async Task<ActionResult> ParticipationStatusListList()
 {
 var serviceResponse = new DataResponse<string> { ErrorCode = ErrorCodes.Failure };
 var clientId = GetAuthenticatedClientId();
 if (clientId == 0)
 {
 serviceResponse.Message = HttpStatusCode.Unauthorized + " :Set Authenticated Client Id";
 return View(serviceResponse);
 }
 int languageId = await GetClientLanguageId(clientId);
 var response = await _participationStatusManager.GetParticipationStatus(languageId);
 if (!response.Exceptions.Any())
 {
 if (response.Response != null)
 {
 var resultResponse = new DataResponse<IEnumerable<ParticipationStatusTranslateViewModel>>
 {
 Data = response.Response
 };
 ErrorMessage.Getmessage(response.Response.Count(), "Participation Status", ref resultResponse);
 return Ok(resultResponse);
 }
 else
 return NotFound();
 }
 foreach (var keyValuePair in response.Exceptions)
 {
 ModelState.AddModelError(keyValuePair.Key, new Exception(keyValuePair.Value));
 }
 if (response.Response != null)
 return View(response.Response);
 else
 return BadRequest(ModelState);
 }
 #endregion
}

Please note that all the communications to database are going through the business layer which is communicating to the repository.

---

Which is a good to use and what is the role of each layer? I do not have good experience developing system design.

Also, should I use viewmodel only in presentation? and use automapper to communicate to the repository?

• Jamal: The title is still generic, and I couldn't think of a better one. Please make it more specific based on the code's purpose.
• user34073: Is this the full code of these methods? It looks like it is to me, but there are some close votes on the question using the reason that it is stub code.
• Just code: @Hosch250 I think this should be enough to understand the whole flow. As I described all the layer's names.

Answer 1

Unit of Work

What strikes me the most in you design is the invalid usage of the Unit of Work. You use it only to get the repository via its property but

a Unit of Work keeps track of everything you do during a business transaction that can affect the database. When you're done, it figures out everything that needs to be done to alter the database as a result of your work.

Like an Entity in Entity Framework that tracks changes.

You might as well pass via DI the repository directly and it wouldn't change anything.

Naming a Unit of Work a unit-of-work is also a bad practice. It's just a placeholder for a more specific type in your domain. Maybe a Country or something but definitely not just a unit-of-work.

I cannot say anything more then this because you didn't post more code but its design doesn't look right.

It's difficult to come up with an example. The only thing I would change about your code in the first place would be to use the repository instead of the invalid unit-of-work. So instead:

public class CountriesController : BaseController
{
 private IUnitOfWork unitOfWork;
 public CountriesController(iMSTUnitOfWork _unitOfWork)
 {
 unitOfWork = _unitOfWork;
 }
}

you'll have:

public class CountriesController : BaseController
{
 private readonly IRepository _repository;
 public CountriesController(IRepository repository)
 {
 _repository = repository;
 }
}

It might be a good idea to ask another question about your unit-of-work and repository that you use here.

MVC

The first design looks OK. I cannot say anything about the second one, it's too short and it doesn't interact with the model - perhaps it doesn't have to. I don't know.

Minor issues

Using regions like you use them is worse then not using them at all. I had to remove them first so that your code becomes readable. Personally I don't like them. Furthermore your code formatting is very strange with those multiple line breaks which makes it hard to read too.

• Just code: Hello, thanks for answering. Can you please post one small example of good practice and design pattern?
• t3chb0t: @Justcode there isn't much to change here, just replace the unit-of-work with a repository as shown (I've added a really small example but I think you already knew this). Consider another question about the unit-of-work/repository design. I think there are also things that can be improved.
• Just code: That's ok. But, I want to understand the business rules only. My goal is to get the information about the business rules. The thing is we are using two approaches use business interfaces in controller and second is using Irepository in controllers. I want to understand which one is good and where comes the business layer in picture?
• t3chb0t: @Justcode I think you'd be better off with this question at Programmers. Your questions seem to go beyond CR. They are more of a general design nature then a simple code review. But even an old question at SO can provide you an answer Business logic in MVC