Extensible code to support different HR rules

Recently, I got challenged to code with following bullet points:

-
Extensible code to support different annual leave rules for HR departments

-
Maintainable code to add/change the existing rules without any impact on the other clients

-
Customizable and configurable code for different clients

-
Exception handling and logging to protect the code and also make support easier

-
Following design and OOP principles

-
Unit testable code

I coded keeping SOLID principle in mind and here is my code. What did I miss? I was not able to get a good score with it. What are the design strategies I am missing?

```
public class EmployeeLeave : ILeaveRequest
{
IDataBaseService databaseService;

public EmployeeLeave(IDataBaseService databaseService)
{
this.databaseService = databaseService;
}

public void ProcessLeaveRequest(DateTime leaveStartDate, int days, string reason, int employeeId)
{
try
{
var employee = FindEmployee(employeeId);

if ((DateTime.Now - employee.StartDate).TotalDays 20)
throw new Exception("Invalid leave request.");

var leaveRequest = new EmployeeLeaveDetail();

leaveRequest.EmployeeId = employeeId;
leaveRequest.LeaveStartDateTime = leaveStartDate;
leaveRequest.LeaveEndDateTime = leaveStartDate.AddDays(days);

SaveLeaveRequest(leaveRequest);

}
catch (Exception)
{

throw;
}
}

public void SaveLeaveRequest(ILeaveRequestDetail leaveRequest)
{
try
{
databaseService.ExecuteProcedure("spLocal_InsertIntoEmployees", new List
{
new Parameters {ParameterName="@EmployeeId",
ParameterValue =leaveRequest.EmployeeId },

new Parameters {Paramet

I coded keeping SOLID principle in mind and here is my code. What did I miss?

Let's see...

SRP - Single Responsibility Principle


A class should have only one reason to change.

Violated. The name EmployeeLeave suggests it's a class that just stores some data about an employee-leave but its API says something else - I'm a repository. So what is it?

Your class has two reasons to change:

• request rules

• save the request

OCP - Open/Closed Principle


Software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.

Violated. You have hard-coded all the stored procedure details although you pass a database abstraction via DI.

The user of the IDataBaseService should not know that he's working with a stored procedure or anything else. The user just wants to save his data.

You cannot extend it by using a different storage type. If you remove the stored procedure you break this implementation.

LSP - Liskov Substitution Principle


Child classes should never break the parent class' type definitions.

Not relevant.

ISP - Interface Segregation Principle


The interface-segregation principle (ISP) states that no client should be forced to depend on methods it does not use.

Violated. As a person who creates leave-requests and implements ILeaveRequest I need to implement the SaveLeaveRequest and FindEmployee. I don't need it. I just want to create a new request. I don't want to know how to save it (at least not here).

DIP - Dependency Inversion Principle


A. High-level modules should not depend on low-level modules. Both should depend on abstractions.
B. Abstractions should not depend upon details. Details should depend upon abstractions.

Violated. SaveLeaveRequest depends on a low level stored procedure although it uses an abstraction IDataBaseService.

Summary


Extensible code to support different annual leave rules for HR departments

Failed. You need to implement the save/find logic for each leave request. This is a lot of redundant code.


Maintainable code to add/change the existing rules without any impact on the other clients

Failed. The stored procedure call and its details belong to the repository. The user should not know how it's implemented and currently he needs to exactly know the implementation details to be able to use it.


Customizable and configurable code for different clients

Partially met. You started with a data layer but reveal to much details about the storage to the outside world.


Exception handling and logging to protect the code and also make support easier
Following design and OOP principles

Failed. The empty catch is not exception handling. The error messages are not very helpful. They don't help to solve the problem by giving a reason or a hint how to correct it.


Unit testable code

Partially met. You can inject another data layer but the implementation of the EmployeeLeave will break if the new data layer doesn't support the hard-coded stored procedure.

Solution (Example)

The interface is a good start but it is too big and it lacks some vital properties that are part of the ProcessLeaveRequest signature but shouldn't be.

The minimal interface should require some basic data and a method to validate this data.

interface ILeaveRequest
{
    int EmployeeId { get; }
    DateTime LeaveStartDate { get; }
    int DayCount { get; }
    void Validate();
}

You implement it by implementing actually only the Validate method any adding any other dependencies via DI if for example you need to check if an employee can still take a leave.

Notice the new exception types and messages that clearly explain why the request isn't valid.

class VacationRequest : ILeaveRequest
{
    public VacationRequest(IEmployeeRepository employeeRepository, int employeeId, DateTime leaveStartDate, int dayCount) {..}
    public int EmployeeId { get; }
    public DateTime LeaveStartDate { get; }
    public int DayCount { get; }
    public void Validate()
    {
        // check if employee has enough vacation days...
        throw new OutOfVacationException("Employee 123 does not have any more vacation.");

        // check if max employees have vacation...      
        throw new MaxEmployeeOnVactaionExceededException("The maximum number of 3 employees on vacation at the same time reached.");
    }
}

You can create multiple requests by adding more properties and dependencies for more complex rules - or fewer.

This is just a simple example but in a more complex solution you can have an ILeaveRequestRule that you pass via DI to concrete request as a collection of several rules so that you may extend them too. In such a case each rule would throw a meaningful exception explaining the violation. It all depends how dynamic the system is. If you think you might need to change them often then it would probably by the way to go.

```
class EmergencyVacationRequest : ILeaveRequest
{
publi

StackExchange Code Review Q#150372, answer score: 43