Krdytkyu

Question

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 <= 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);





        }

    }

You need to read this: Enterprise Rules Engine. Just fyi. ;) — Dec 22 '16 at 1:19

Community♦ 1 · Accepted Answer · 2017-05-23 12:40:57Z

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.

This is a very good answer, but I've got to question creating exceptions like OutOfVacationException. Let's pretend that we neglected to catch it, should the entire application crash because an employee is out of vacation time?? — Dec 21 '16 at 1:53
I have a question regarding the use of employeeId vs. the actual instance of Employee. I would have passed the employee as an argument to the constructor so inside the request I could directly query it regarding any information I might need. By just handing over the ID I might have to make another trip to the database to get an employee that - most likely - was already retrieved before the request was created. Is there any specific argument for passing the ID / against passing the employee? — Dec 21 '16 at 7:20
@t3chb0t exceptions are for exceptional behavior. You're talking about using them for expected control flow. — Dec 21 '16 at 10:00
Let's agree to disagree @t3chb0t. I see a need for a ValidationResult, you don't. That's fine. — Dec 21 '16 at 10:59
Your proposed solution isn't ideal. Why should a leave request be able to validate itself? You're breaking the Open/Closed principle right there - you have to change the source to change the rules that apply to each type. The caller also has to know which type they're dealing with to know which exceptions might be thrown. I'd say that's pretty close to a LSP violation too. — Dec 21 '16 at 12:08

404 2,261515 · Accepted Answer · 2016-12-20 12:18:11Z

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.

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.

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.

score 14 · Accepted Answer · 2016-12-20 11:56:27Z

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.

I always wondered about that...what's the point of wrapping in try/catch when I'm only gonna throw it to a higher function? — Dec 20 '16 at 15:30
@Abdul there is no point in wrapping try/catch if all you're doing is throwing higher. Only catch an exception if you're going to do something about it (add information, reclassify it, or handle it). — Dec 20 '16 at 17:09

Paweł Łukasik 491310 · Accepted Answer · 2016-12-20 19:15:23Z

A lot of useful comments is here but no-one has commented about the usage of DateTime.

EmployeeLeave

DateTime.Now - employee.StartDate

I encourage you to process all the dates as UTC and only convert them to local time when you need to display. Even if you are sue your application will be used in one time-zone you save yourself from a lot of hassle in the future if you dedicated that you need to support timezones. The second one is in tests and I'll get back to that at the end.

Another problem with this class is that it does too much. You are violating the SRP (Single Responsibility Principle). This class handles holidays, find employee and validating the correctness of holidays. I think you should split that into several classes each responsible for simple task. You can then inject them into your EmployeeLeave class and only construct the logic by calling specific methods.

public EmployeeLeave(IEmployeeLeaveStore employeeLeaveStore, 

                     IEmployeeFinder emploeeFinder, 

                     IHolidayValidator holidayValidator)

{

        if (employeeLeaveStore == null) throw new ArgumentNullException(nameof(employeeLeaveStore));

        if (employeeFinder == null) throw new ArgumentNullException(nameof(employeeFinder));

        if (holidayValidator == null) throw new ArgumentNullException(nameof(holidayValidator));



        this.employeeLeaveStore = employeeLeaveStore;

        this.employeeFinder = employeeFinder;

        this.holidayValidator = holidayValidator;

}

Each interface here is so-called role interface that has only the method(methods) for this particular role. So in above example:

IEmployeeLeaveStory - would only contain method for Saving the employee's leave object

IEmployeeFinder - with one method Find

IHolidayValidator - with one method IsValid

I would be useful to create single rules for validating the correctness of holidays and then compose an aggregate holiday validator that would only execute IsValid on its children. It could look like this:

var compositeValidator = new CompositeLeaveValidator(

    new NoMoreThanTwentyDaysValidator(), 

    new MarriedAndEmployedForLessThan3MonthsValidator());

You can also create a composition based on employee-type as probably different rules are applicable. It's also a good way to extend with new rules.

Additionally in the constructor we check that all the parameters are not null and in case they are we Fail fast which is also a good thing to do.

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

{

    var employee = employeeFinder.Find(employeeId);



   if (!holidayValidator.IsValid(leaveStartDate, days, reason, employee))    

       throw new InvalidHolidayException("Specified holiday is invalid.")



    var leaveRequest = new EmployeeLeaveDetail();



    leaveRequest.EmployeeId = employeeId;

    leaveRequest.LeaveStartDateTime = leaveStartDate;

    leaveRequest.LeaveEndDateTime = leaveStartDate.AddDays(days);



    employeeLeaveStore.Save(leaveRequest);

}

I would also like to extract this EmployeeLeaveDetail creation to a separate class but that's up to you.

As I've mentioned above - there's also one issue with DateTime. This time in Unit Tests.

UnitTest

Basically due to the fact that you use DateTime.Now (or UtcNow as you should) in your ProcessLeaveRequest that means that every time you run your test for this method you run different tests as the DateTime. The better approach to this would be to create a SystemTime as follow.

public static class SystemTime

{

    public static Func<DateTime> Now = () => DateTime.UtcNow;

}

then later in your test you can specify what is the current date-time when the test should execute and do not rely on DateTime when the test was run.

[TestMethod]

public void IsMarriedAndLessThan90Days()

{

    SystemTime.Now = () => new DateTime(2016,12,20, 0,0,0);

    // do the testing with DateTime fixed on 20th of December 2016.

}

You also use this class wherever you need a to get a DateTime. This way you are sure that everywhere you use UTC or non-UTC and you are consistent.

Additionally check Five Common Daylight Saving Time Antipatterns of .NET Developers as there might be some issues with DST when you do calculations on DateTime.

germi 38817 · Accepted Answer · 2016-12-20 16:07:44Z

Generally speaking, unit tests should follow the "Arrange, Act, Assert"-pattern. Yours only have the "Arrange, Act"-part. You're not asserting anything (with the exception of the one where you're expecting an exception to be thrown).

[TestMethod]

public void IsMarriedAndLessThan90Days()

{

    // Arrange

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

    // Act

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

    // Assert?

}

Your unit tests depend on your database. You should try to avoid any external dependencies in your unit tests.

Related to the point above: You're testing your functionality with existing users. If anything changes in your database (someone marries, the startdate is more than 90 days ago, ...) you have to rewrite your tests. You should explicitly construct your Employees in your test cases so they don't change and someone else can easily see what's going on.

In my opinion your EmployeeLeaveRequest knows to much about how it's going to be saved. Your IDatabaseService interface should have methods like void SaveLeaveRequest(ILeaveRequest request) where you just hand over your request and let it handle all the SQL or whatever it needs to save the requests. Similar with FindEmployee. It should not be the responsibility of the EmployeeLeaveRequest to retrieve the employee record from a database.

Sᴀᴍ Onᴇᴌᴀ 8,34261853 · Accepted Answer · 2018-12-31 18:01:50Z

This is an awesome explanation. I just see one more extensible point by using strategies here, i.e. if the different types of processing can happen based employment categories like Permanent Employee, Contractors etc.

So here we can use processor strategies for different types of leaves and different categories, which can be return by a factory. And the ProcessLeaveRequest can take either strategy interface as input or can make a call to leaveprocessorfactory to return the proper strategy.

I hope I am making some sense here.

Community♦ 1 · Accepted Answer · 2017-05-23 12:40:57Z

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.

This is a very good answer, but I've got to question creating exceptions like OutOfVacationException. Let's pretend that we neglected to catch it, should the entire application crash because an employee is out of vacation time?? — Dec 21 '16 at 1:53
I have a question regarding the use of employeeId vs. the actual instance of Employee. I would have passed the employee as an argument to the constructor so inside the request I could directly query it regarding any information I might need. By just handing over the ID I might have to make another trip to the database to get an employee that - most likely - was already retrieved before the request was created. Is there any specific argument for passing the ID / against passing the employee? — Dec 21 '16 at 7:20
@t3chb0t exceptions are for exceptional behavior. You're talking about using them for expected control flow. — Dec 21 '16 at 10:00
Let's agree to disagree @t3chb0t. I see a need for a ValidationResult, you don't. That's fine. — Dec 21 '16 at 10:59
Your proposed solution isn't ideal. Why should a leave request be able to validate itself? You're breaking the Open/Closed principle right there - you have to change the source to change the rules that apply to each type. The caller also has to know which type they're dealing with to know which exceptions might be thrown. I'd say that's pretty close to a LSP violation too. — Dec 21 '16 at 12:08

404 2,261515 · Accepted Answer · 2016-12-20 12:18:11Z

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.

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.

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.

score 14 · Accepted Answer · 2016-12-20 11:56:27Z

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.

I always wondered about that...what's the point of wrapping in try/catch when I'm only gonna throw it to a higher function? — Dec 20 '16 at 15:30
@Abdul there is no point in wrapping try/catch if all you're doing is throwing higher. Only catch an exception if you're going to do something about it (add information, reclassify it, or handle it). — Dec 20 '16 at 17:09

Paweł Łukasik 491310 · Accepted Answer · 2016-12-20 19:15:23Z

A lot of useful comments is here but no-one has commented about the usage of DateTime.

EmployeeLeave

DateTime.Now - employee.StartDate

I encourage you to process all the dates as UTC and only convert them to local time when you need to display. Even if you are sue your application will be used in one time-zone you save yourself from a lot of hassle in the future if you dedicated that you need to support timezones. The second one is in tests and I'll get back to that at the end.

Another problem with this class is that it does too much. You are violating the SRP (Single Responsibility Principle). This class handles holidays, find employee and validating the correctness of holidays. I think you should split that into several classes each responsible for simple task. You can then inject them into your EmployeeLeave class and only construct the logic by calling specific methods.

public EmployeeLeave(IEmployeeLeaveStore employeeLeaveStore, 

                     IEmployeeFinder emploeeFinder, 

                     IHolidayValidator holidayValidator)

{

        if (employeeLeaveStore == null) throw new ArgumentNullException(nameof(employeeLeaveStore));

        if (employeeFinder == null) throw new ArgumentNullException(nameof(employeeFinder));

        if (holidayValidator == null) throw new ArgumentNullException(nameof(holidayValidator));



        this.employeeLeaveStore = employeeLeaveStore;

        this.employeeFinder = employeeFinder;

        this.holidayValidator = holidayValidator;

}

Each interface here is so-called role interface that has only the method(methods) for this particular role. So in above example:

IEmployeeLeaveStory - would only contain method for Saving the employee's leave object

IEmployeeFinder - with one method Find

IHolidayValidator - with one method IsValid

I would be useful to create single rules for validating the correctness of holidays and then compose an aggregate holiday validator that would only execute IsValid on its children. It could look like this:

var compositeValidator = new CompositeLeaveValidator(

    new NoMoreThanTwentyDaysValidator(), 

    new MarriedAndEmployedForLessThan3MonthsValidator());

You can also create a composition based on employee-type as probably different rules are applicable. It's also a good way to extend with new rules.

Additionally in the constructor we check that all the parameters are not null and in case they are we Fail fast which is also a good thing to do.

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

{

    var employee = employeeFinder.Find(employeeId);



   if (!holidayValidator.IsValid(leaveStartDate, days, reason, employee))    

       throw new InvalidHolidayException("Specified holiday is invalid.")



    var leaveRequest = new EmployeeLeaveDetail();



    leaveRequest.EmployeeId = employeeId;

    leaveRequest.LeaveStartDateTime = leaveStartDate;

    leaveRequest.LeaveEndDateTime = leaveStartDate.AddDays(days);



    employeeLeaveStore.Save(leaveRequest);

}

I would also like to extract this EmployeeLeaveDetail creation to a separate class but that's up to you.

As I've mentioned above - there's also one issue with DateTime. This time in Unit Tests.

UnitTest

Basically due to the fact that you use DateTime.Now (or UtcNow as you should) in your ProcessLeaveRequest that means that every time you run your test for this method you run different tests as the DateTime. The better approach to this would be to create a SystemTime as follow.

public static class SystemTime

{

    public static Func<DateTime> Now = () => DateTime.UtcNow;

}

then later in your test you can specify what is the current date-time when the test should execute and do not rely on DateTime when the test was run.

[TestMethod]

public void IsMarriedAndLessThan90Days()

{

    SystemTime.Now = () => new DateTime(2016,12,20, 0,0,0);

    // do the testing with DateTime fixed on 20th of December 2016.

}

You also use this class wherever you need a to get a DateTime. This way you are sure that everywhere you use UTC or non-UTC and you are consistent.

Additionally check Five Common Daylight Saving Time Antipatterns of .NET Developers as there might be some issues with DST when you do calculations on DateTime.

germi 38817 · Accepted Answer · 2016-12-20 16:07:44Z

Generally speaking, unit tests should follow the "Arrange, Act, Assert"-pattern. Yours only have the "Arrange, Act"-part. You're not asserting anything (with the exception of the one where you're expecting an exception to be thrown).

[TestMethod]

public void IsMarriedAndLessThan90Days()

{

    // Arrange

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

    // Act

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

    // Assert?

}

Your unit tests depend on your database. You should try to avoid any external dependencies in your unit tests.

Related to the point above: You're testing your functionality with existing users. If anything changes in your database (someone marries, the startdate is more than 90 days ago, ...) you have to rewrite your tests. You should explicitly construct your Employees in your test cases so they don't change and someone else can easily see what's going on.

In my opinion your EmployeeLeaveRequest knows to much about how it's going to be saved. Your IDatabaseService interface should have methods like void SaveLeaveRequest(ILeaveRequest request) where you just hand over your request and let it handle all the SQL or whatever it needs to save the requests. Similar with FindEmployee. It should not be the responsibility of the EmployeeLeaveRequest to retrieve the employee record from a database.

Sᴀᴍ Onᴇᴌᴀ 8,34261853 · Accepted Answer · 2018-12-31 18:01:50Z

This is an awesome explanation. I just see one more extensible point by using strategies here, i.e. if the different types of processing can happen based employment categories like Permanent Employee, Contractors etc.

So here we can use processor strategies for different types of leaves and different categories, which can be return by a factory. And the ProcessLeaveRequest can take either strategy interface as input or can make a call to leaveprocessorfactory to return the proper strategy.

I hope I am making some sense here.

Extensible code to support different HR rules

6 Answers 6

SRP - Single Responsibility Principle

OCP - Open/Closed Principle

LSP - Liskov Substitution Principle

ISP - Interface Segregation Principle

DIP - Dependency Inversion Principle

Summary

Solution (Example)

Appendix - Exceptions vs ValidationResult

public void ProcessLeaveRequest

EmployeeLeave

UnitTest

Your Answer

Sign up or log in

Post as a guest

Post as a guest

6 Answers 6

6 Answers 6

SRP - Single Responsibility Principle

OCP - Open/Closed Principle

LSP - Liskov Substitution Principle

ISP - Interface Segregation Principle

DIP - Dependency Inversion Principle

Summary

Solution (Example)

Appendix - Exceptions vs ValidationResult

SRP - Single Responsibility Principle

OCP - Open/Closed Principle

LSP - Liskov Substitution Principle

ISP - Interface Segregation Principle

DIP - Dependency Inversion Principle

Summary

Solution (Example)

Appendix - Exceptions vs ValidationResult

SRP - Single Responsibility Principle

OCP - Open/Closed Principle

LSP - Liskov Substitution Principle

ISP - Interface Segregation Principle

DIP - Dependency Inversion Principle

Summary

Solution (Example)

Appendix - Exceptions vs ValidationResult

SRP - Single Responsibility Principle

OCP - Open/Closed Principle

LSP - Liskov Substitution Principle

ISP - Interface Segregation Principle

DIP - Dependency Inversion Principle

Summary

Solution (Example)

Appendix - Exceptions vs ValidationResult

public void ProcessLeaveRequest

public void ProcessLeaveRequest

public void ProcessLeaveRequest

public void ProcessLeaveRequest

EmployeeLeave

UnitTest

EmployeeLeave

UnitTest

EmployeeLeave

UnitTest

EmployeeLeave

UnitTest

Sign up or log in

Post as a guest

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Popular posts from this blog

Ellipse (mathématiques)

Quarter-circle Tiles

Mont Emei

6 Answers
6

6 Answers
6

6 Answers
6