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

1. Extensible code to support different annual leave rules for HR departments




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




3. Customizable and configurable code for different clients




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




5. Following design and OOP principles




6. 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 <= 90 && !employee.IsMarried)

                    throw new Exception("Invalid leave request.");



                if (days > 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<Parameters>

                {

                   new Parameters {ParameterName="@EmployeeId",

                       ParameterValue =leaveRequest.EmployeeId },



                   new Parameters {ParameterName="@StartDate",

                       ParameterValue =leaveRequest.LeaveStartDateTime },



                   new Parameters {ParameterName="@EndDate",

                       ParameterValue =leaveRequest.LeaveEndDateTime }

                });

            }

            catch (Exception)

            {



                throw;

            }

        }



        public Employee FindEmployee(int employeeId)

        {

            Employee employee = default(Employee);



            try

            {





                var sqlReader = databaseService.ExecuteProcedure("spLocal_GetEmployeeFromId", new List<Parameters> { new DBLayer.Parameters {

                ParameterName="@EmployeeId",

                ParameterValue=employeeId

            } });





                using (sqlReader)

                {

                    while (sqlReader.Read())

                    {

                        employee = new Employee

                        {

                            EmployeeId = long.Parse(sqlReader["EmployeId"].ToString()),

                            Name = sqlReader["FirstName"].ToString(),

                            LastName = sqlReader["LastName"].ToString(),

                            StartDate = DateTime.Parse(sqlReader["StartDate"].ToString()),

                            Salary = Decimal.Parse(sqlReader["Salary"].ToString()),

                            IsMarried = bool.Parse(sqlReader["Married"].ToString())

                        };



                    }

                }

            }

            catch (Exception)

            {



                throw;

            }

            return employee;



        }

    }

And below is my Unit Test.

public class AnualLeaveTest

    {

        [TestMethod]

        public void IsMarriedAndGreaterThan90Days()

        {

            //TOdo: Need to check if connection ScopeIdentity returned is same

            EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());

            leave.ProcessLeaveRequest(DateTime.Now, 6, "", 212);

        }



        [TestMethod]

        public void IsMarriedAndLessThan90Days()

        {

            //To do: Need to check if connection ScopeIdentity returned is same

            EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());

            leave.ProcessLeaveRequest(DateTime.Now, 6, "", 454);

        }



        [TestMethod]

        public void NotMarriedAndGreaterThan90Days()

        {

            //TOdo: Need to check if connection ScopeIdentity returned is same

            EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());

            leave.ProcessLeaveRequest(DateTime.Now, 6, "", 767);

        }



        [TestMethod]

        [ExpectedException(typeof(Exception),"Invalid leave request.")]

        public void NotMarriedAndLessThan90Days()

        {

            EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());



            leave.ProcessLeaveRequest(DateTime.Now, 6, "", 645);





        }



        [TestMethod]

        [ExpectedException(typeof(Exception), "Invalid leave request.")]

        public void LeaveRequestGreaterThan20Days()

        {

            EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());



            leave.ProcessLeaveRequest(DateTime.Now, 26, "", 645);





        }

    }

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

{

    public EmergencyVacationRequest(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.");



        // other rules might not apply here...

    }

}

To create all the different leave requests you would write a factory (not included in the example).

Finally a simple leave request processor validates each request and saves it in the leave repository.

class LeaveRequestProcessor

{

    public LeaveRequestProcessor(ILeaveRepository leaveRepository) {..}



    public void ProcessLeaveRequest(ILeaveRequest leaveRequst)

    {

        leaveRequst.Validate(); // You might want to catch this somewhere an log it.



        leaveRepository.SaveLeave(

            leaveRequst.EmployeeId, 

            leaveRequst.LeaveStartDate, 

            leaveRequst.DayCount

        );

    }

}

The leave repository can of course modify various tables so that the leave requests have access to this data if they need to in order to validate their rules.

Here are some of the advantages of the new solution:

• You can create new leave requests at any time without breaking anything with whatever logic you want


• You implement only what you really need for a new leave request


• You always know what and why it didn't work


• You are independent of the storage type


• You can test all units by mocking the repositories

Appendix - Exceptions vs ValidationResult

There are various opinions about whether you should use exceptions or validation results for thing like this.

• Is it a good practice to throw an exception on Validate() methods or better to return bool value?


• Is it a good or bad idea throwing Exceptions when validating data?

As a matter of fact I use both patterns. It's sometimes a personal or project preference which one you pick.

public class EmployeeLeave : ILeaveRequest

...

var leaveRequest = new EmployeeLeaveDetail();

This is confusing. Going by name, ILeaveRequest implies that the class implementing that interface is a "leave request" (I'm aware that classes don't have an "is a" relationship with interfaces). But then we see an actual leave request in the form of an EmployeeLeaveDetail. This EmployeeLeave class simply processes leave requests. So why does this class implement an interface called ILeaveRequest when it's not a request and doesn't implement an interface consistent with what you might expect from a leave request? And for that matter, if EmployeeLeaveDetail is a leave request (as per your variable's name), why is it called EmployeeLeaveDetail rather than EmployeeLeaveRequest? I see you created an interface called ILeaveRequestDetail. You should rename that interface to ILeaveRequest, and rename the current ILeaveRequest to something more accurate.

if ((DateTime.Now - employee.StartDate).TotalDays <= 90 && !employee.IsMarried)

  throw new Exception("Invalid leave request.");



if (days > 20)

  throw new Exception("Invalid leave request.");

What's the significance of 90 and 20? Don't use magic numbers, create well named constants and use those instead.

Also as Heslacher said, don't throw an Exception; use one of the more specific types or create your own domain-specific exceptions. And at least say what's wrong, otherwise you're left scratching your head as to which of your (potentially) 20 checks failed when you get an Exception and it simply says Invalid leave request.

public void ProcessLeaveRequest(DateTime leaveStartDate, int days, string reason, int employeeId)

I take some issue with the name of this method, because it's not processing (whatever that means) a "leave request". You have a method right underneath, SaveLeaveRequest(ILeaveRequestDetail), which actually deals with a leave request. ProcessLeaveRequest just deals with ints, strings, etc.

What I think should be happening here is you have a method in some class to create a leave request for an employee. That method takes the ints, strings, etc, performs validation, and returns a leave request. You can then call the Save method to save the request.

Also in general you should be using domain constructs more. For example, you're accepting an intfor the employee id. Why? Surely at this point you should have already constructed your Employee - it should be impossible to create a request without selecting which Employee to create it for - so when creating a request you can just pass in the Employee, which removes the need to look it up and potentially fail on request creation.

1. Extensible code to support different annual leave rules for HR
   departments.


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

Your rules:

public void ProcessLeaveRequest(DateTime leaveStartDate, int days, string reason, int employeeId)

{

  if ((DateTime.Now - employee.StartDate).TotalDays <= 90 && !employee.IsMarried)

      throw new Exception("Invalid leave request.");



  if (days > 20)

      throw new Exception("Invalid leave request.");

In what way does hardcoding the rules into the single method that creates and validates the holiday requests meet those requirements? You completely ignored requirement 1.

Overall this class feels like it should be a repository, given all the database work it's doing.

catch (Exception)

{



    throw;

}  

Well this won't buy you anything but a few more lines of code. Just remove the try..catch at all. This construct just let the exception bubble up the call stack like it would if you wouldn't have the try..catch at all.

public void ProcessLeaveRequest

• You don't validate all of the given method arguments.


• The reason argument isn't used at all.


• You are throwing Exception instead of e.g ArgumentOutOfRangeException which would be the better fit.


• it seems (from the FindEmployee() method) that Employee is a struct (hint default(Employee)) which makes it possible that you add something into your database which doesn't belongs there. Assume you pass an epmloyeeId which doesn't exists into the FindEmployee() method you would the get a default(Employee) back which in the case of a struct would have default property values for StartDate and IsMarried. If this properties would be seen as valid in your validateion part, you would end up with a EmployeeLeaveDetail in your database for an employee which doesn't exist.

You should always use braces {} although they might be optional. By not using them you are in danger of introducing bugs which are hard to track. I would like to encourage you to always use them. This will help you to make your code less error prone.

This is meant for if, else if and else.