It feels somewhat awkward to answer a question this old, but there are some things that can be done.
First, I want to comment on the naming, simply from the fact that I'm currently building an ASP.NET app which is very large, and I've learned through the process that the ViewModel name becomes cumbersome, and is partially a lie. I've begun to name my 'View Models' as 'DTOs' instead, that is: Data-Transfer Object.
This has begun to make my code much more clear, as it's now obvious that the DTO can be a two-way object: data can be fed to it from the back-end or from the UI.
Regarding the service-layer handling database access: this is a good approach, I'll tell you that a lot of my code in this project I've mentioned previously is in the controller, and it's cumbersome. What I would change is reject the idea of needing to create a dict (or pass it to the .Merge method) controller-side, and do one of two things:
- Create a
BaseController that will handle adding the validation errors to the ModelState itself, though this can become complicated.
- Pass the
Controller to the method doing the validation, and allow it to add the errors to the ModelState.
It's been a while since I've touched ASP.NET MVC 3, but in 4 and 5 you can directly manipulate the ModelStateDictionary, and use .Add or .AddModelError. I do this on various validation methods to remove that responsibility from the controller, and reduce the call to:
[Authorize(Roles = "ADMIN")]
[HttpPost]
public ActionResult Create(PostEditViewModel postEditViewModel)
{
BlogPost post = new BlogPost();
postEditViewModel.MapTo(post);
_postService.CreatePost(post, this);
if (!ModelState.IsValid)
return View();
return RedirectToAction("Index", "Home");
}
Another pointer: I would recommend converting your Roles attribute parameters to constants, I've found that it becomes unpleasant to change the name of a Role later, if you need to. I actually have a Constants.Roles class that has each role string associated to a type, so that if I have to change the name from Controllers to Inventory, it's right there, and my code can still reference the InventoryControllers constant. (You can also use the constants with XML-docs to make things more descriptive in the future.)
You have:
BlogPost post = new BlogPost();
But then:
var dic = _postService.CreatePost(post);
Be consistent. If you're using var in one location, use it in the others. I used to be anti-var period because I felt it made my code look less organized, but to be honest, it helps align all the variable names to the fifth textual column (counting the space), so it's easy to identify where something is declared. (Of course, with the highlighting Visual Studio does it's not necessary, and F12 is a wonderful tool, but it's still helpful at times.)
It also allows you to ignore what type it is when declaring. I'd have to type BlogPost out in one of those two places on that line (though one can argue that with intellisense when you type BlogPost thing = new it automatically suggests BlogPost() to finish it) so I don't mind changing the manual typing from BlogPost post = to var post = new BlogPost();, personally I type faster that way anyway.