Keeping data processing out of controllers

Question

I would like to start by mentioning that I am LAMP stack guy who happens to be making my first ever .NET C# web app and I'm seeking general advises for best practices and to see how more experienced people would have done it.

I am using code first development pattern with latest Entity Framework . I have a AppUser model. The user could be on of few types, one of them is employee.

public class AppUser{
  public int Id { get; set; }
  public UserType Type { get; set; }
  public Title Title { get; set; }
  public string FirstName { get; set; }
  public string MiddleName { get; set; }
  public string LastName { get; set; }
  public virtual AddressDetails AddressDetails { get; set; }
  public virtual Company Company { get; set; }
  ..................................................
}

public class AddressDetails
{
  public int Id { get; set; }
  public string AddressLine { get; set; }
  public string City { get; set; }
  public string County { get; set; }
  public string PostCode { get; set; }
  public float Latitude { get; set; }
  public float Longtitude { get; set; }
  [JsonIgnore]
  public virtual AppUser User { get; set; }
}

I believe that smaller methods are the better. So, I am trying to keep controllers as small as possible. I made classes which handles all interactions with database and keeps business logic out of Models and Controllers. First class is abstract MasterRepository ,which has logic for Database Update/Add/SaveChanges. All other "repositories" inherit it. ( "AppDB" is inheriting from DbContext and holds all DbSets)

public abstract class MasterRep
{
  protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }

  protected int _companyId { get; set; }
  protected string _applicationUserId { get; set; }
  public MasterRep(string applicationUserId, AppDB db = null)
  {
    _db = db ?? new AppDB ();
    _currentUser = GetUserByApplicationId(applicationUserId);
    _companyId = _currentUser.Company.Id;
    _applicationUserId = applicationUserId;
   }
  }
  .........................................

Each model has it own "repository". So, User of type "employee" has:

 public class EmployeeRep : UserRep
{
    public EmployeeRep(string applicationUserId, AppDB db = null) :      base(applicationUserId, db) { }

    private IEnumerable<AppUser> Employees
    {
        get
        {
            IEnumerable<AppUser> employees = _db.AppUsers.Where(
               r =>
               r.Company.Id == _companyId &&
               r.Type == UserType.Employee &&
               r.IsDeleted == false
            );
            return employees;
        }
    }
   public User GetById(int Id)
    {
        User employee = Employees.FirstOrDefault(r =>
            r.Id == Id
        );
        return employee;
    }
    ....................... 

The EmployeeRep holds all logic for getting/updating all employees.

And here is example of retrieving a single employee in controller

 public class HomeController : Controller
  {
    private EmployeeRep _employeeRep { get; set; }
    protected override void Initialize(System.Web.Routing.RequestContext requestContext)
    {
        base.Initialize(requestContext);

            _employeeRep = (User.Identity.GetUserId() != null) ? new EmployeeRep(User.Identity.GetUserId()) : null;

    }
    public ActionResult Index(int id)
    {
        NHUser currentUser = _employeeRep.CurrentUser;

        NHUser employee = _employeeRep.GetById(id);
        ViewBag.CurrentUser = currentUser;

        return View(employee );
    .............................................

So, controllers are in charge of what get displayed and "Repositories" in charge of all business logic/database interactions. All methods are small and doing only one task. However, I am confused by open source projects which I found on github. It looks like everyone doing their own thing.

Am I on a right track here? What is more common way of achieving same goal? i.e having a separate class which handles data processing.

Answer 1

Yes you are on right track. Controllers are not much more than traffic directors, they signal the repository/business layer to get or put the data. Then they return the content to the View. They also disallow any posting or getting of data when the state is not valid. Note: You can implement both client side and server side validation.

If you use Entity Framework, you will be able to find many patterns out there on creating generic repositories. Some argue that EF IS a repository itself.

If you know MVVM pattern, the viewmodel is a model on steroids. Besides laying out the content it can also contain methods that implement the business logic. The controller then can chose to accept only strongly typed viewmodels and send them to the view as well. This is known as a strongly-type view, which allows for easy binding using templates.

Answer 2

Your brace style is inconsistent. Allman style is usually applied in C#.

public class AppUser{
    // snipped
}

public class AddressDetails
{
    // snipped
}

---

Longtitude is a spelling error, it should be Longitude.

---

Underscores are usually only used for fields; properties should be PascalCase:

  protected AppDB _db { get; set; }
  protected AppUser _currentUser { get; set; }

---

Don't abbreviate unnecessarily: I don't know what Rep is, I do know what Repository is.

public class EmployeeRep : UserRep

---

Is this a field or a property:

private EmployeeRep _employeeRep { get; set; }

The { get; set; } says "property", but the private and the underscore at the start of the name says "field".

---

Avoid "negative checks" like you do here:

_employeeRep = (User.Identity.GetUserId() != null) 
    ? new EmployeeRep(User.Identity.GetUserId()) 
    : null;

Invert that logic and things become a lot clearer:

_employeeRep = (User.Identity.GetUserId() == null) 
    ? null 
    : new EmployeeRep(User.Identity.GetUserId());

Also, since you call User.Identity.GetUserId() twice, consider calling it once, storing the result in a variable and then using that variable.