1

I have the following class:

internal class LeaveRequest : ServiceBase
{

    private const string InvalidRequestMessage = "Specified Request does not exist";
    private const string InvalidApproverMessage = "You are not an approver for the specified user";

    private const string RequestWrongStatusMessage = "The request status cannot be changed due to the current status";

    private const string CancelRequestIncludes = "LeaveRequestStatuses";

    internal LeaveRequest(DataAccess.BookItContext context) : base(context) { }

    private static IQueryable<Model.LeaveRequest> ProcessIncludeProperties(string includeProperties,
                                                                           IQueryable<Model.LeaveRequest> query)
    {
        foreach (var includeProperty in includeProperties.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries))
        {
            query = query.Include(includeProperty);
        }
        return query;
    }

    private static IQueryable<Model.LeaveRequest> ProcessFilter(Expression<Func<Model.LeaveRequest, bool>> filter,
                                                                IQueryable<Model.LeaveRequest> query)
    {
        if (filter != null)
        {
            query = query.Where(filter);
        }
        return query;
    }

    private ICollection<Model.LeaveRequest> Find(Expression<Func<Model.LeaveRequest, bool>> filter = null,
                                                 string includeProperties = "")
    {
        var requestQuery = Context.LeaveRequests.AsQueryable();
        requestQuery = ProcessFilter(filter, requestQuery);
        requestQuery = ProcessIncludeProperties(includeProperties, requestQuery);

        return requestQuery.ToList();
    }

    private Model.LeaveRequest FindByID(int id,
                                        string includeProperties = "")
    {
        var requestQuery = Context.LeaveRequests.AsQueryable();
        requestQuery = ProcessFilter(r => r.ID == id, requestQuery);
        requestQuery = ProcessIncludeProperties(includeProperties, requestQuery);

        return requestQuery.FirstOrDefault();
    }

    private Model.LeaveRequestStatus CreateStatus(RequestStatuses status,
                                                  int approverID,
                                                  string notes,
                                                  DateTime editDate)
    {
        return new Model.LeaveRequestStatus()
        {
            IsCurrent = true,
            Notes = notes,
            StatusID = (int)status,
            UserID = approverID,
            StatusDate = editDate
        };
    }

    private void SetNewRequestStatus(Model.LeaveRequest request,
                                     Model.LeaveRequestStatus currentStatus,
                                     RequestStatuses status,
                                     int approverID,
                                     string notes,
                                     DateTime editDate)
    {
        currentStatus.IsCurrent = false;
        Context.Entry(currentStatus).State = EntityState.Modified;

        var newStatus = CreateStatus(status, approverID, notes, editDate);
        request.LeaveRequestStatuses.Add(currentStatus);
        Context.Entry(currentStatus).State = EntityState.Added;
    }

    internal OperationResult CancelRequest(int requestID, int userID, string notes)
    {
        var result = new OperationResult() { Success = true };

        DateTime editDate = DateTime.UtcNow;

        var request = this.FindByID(requestID, CancelRequestIncludes);

        if (request == null)
        {
            throw new ArgumentException(InvalidRequestMessage);
        }

        var currentStatus = request.LeaveRequestStatuses.Where(s => s.IsCurrent).FirstOrDefault();

        if (currentStatus.StatusID == (int)RequestStatuses.RequestPending || currentStatus.StatusID == (int)RequestStatuses.RequestApproved)
        {
            if (currentStatus.StatusID == (int)RequestStatuses.RequestPending)
            {
                SetNewRequestStatus(request, currentStatus, RequestStatuses.CancellationApproved, userID, notes, editDate);
            }
            else if (currentStatus.StatusID == (int)RequestStatuses.RequestApproved)
            {
                var approver = new Approver(Context);

                if (approver.Validate(request.UserID, userID))
                {
                    SetNewRequestStatus(request, currentStatus, RequestStatuses.CancellationPending, userID, notes, editDate);
                }
                else
                {
                    throw new InvalidApproverException(InvalidApproverMessage);
                }

                Context.SaveChanges();
            }
        }
        else
        {
            result.Success = false;
            result.Message = RequestWrongStatusMessage;
        }

        return result;
    }
}

It uses EF to access the data. My question is should it be split into two classes?

I'm beginning to think maybe it should be split into a class to handle the various validations (business rules) and another to actually do the creation of the correct object graph and setting the correct entity state and do the save changes. My only concern about splitting into two classes I may end up with various 'pass-through' methods.

Also if I do split into two classes I'm not sure whether I should use two namespaces eg. Business.LeaveRequest and Service.LeaveRequest or whether I should name the classes LeaveRequest and LeaveRequestDB.

2

Considering the Single Responsibility Principle, it would be worthwhile to look at each of the different concerns that your class is handling.

(Also worth reading: Command Query Responsibility Segregation).

looking through your code, I can see 4 potential responsibilities in your LeaveRequest class:

  • Building DB queries based on your property list.
  • Basic CRUD operations in/out of the DB, including running the query.
  • Request Validation
  • Handling inbound Service requests.

Each of those seem logically separate to me, so you might consider breaking that down into as many classes.

It's not a bad thing for the class which handles the inbound requests to be very thin/lightweight. If the service class is reduced down to being just 'glue', then you should be able to decouple all the others from each other.

If your service class is only responsible for "being the service" and behaving as glue, then it should be easier to arrive at a loosely-coupled design, with small self-contained objects which need little or no interaction with each other.

Often it's easier to see how classes should be divided by looking at them from the outside; i.e. Inputs-vs-Outputs, and looking to minimise any side-effects from within each class (Immutability is a good thing - i.e. if you pass an argument to a class/method, try avoid the class/method modifying that argument).

For example; without suggesting whether this might be a correct design, more to illustrate how the service class might look when all the other areas are de-coupled from each other:

internal class LeaveRequestService : ServiceBase
{
    private QueryBuilder _queryBuilder;
    private DataLayer _dataLayer;
    private StatusUpdater _statusUpdater;

    internal OperationResult CancelRequest(int requestID, int userID, string notes)
    {
        var query = _queryBuilder.CreateQuery(requestID, userID, notes);
        var queryResult = _dataLayer.RunQuery(query);
        var validator = new ValidatingStatusAnalyser(queryResult);
        var operationResult = _statusUpdater.Update(_dataLayer, validator);
        return operationResult;
    }
}

You may end up with several classes which only have 1 or 2 methods - that's usually a sign that your design is aligned with SRP, although you need to make a judgement call as to whether you want that level of decoupling. e.g. If you're aiming for complete unit test coverage then keeping very small objects like that has a lot of advantages.

  • I can't see why I need to separate query builder and the data layer. I am using Entity Framework so to run the query I just invoke ToList of FirstOrDefault. – James Culshaw Mar 26 '16 at 23:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.