Расширяемый код для поддержки различных правил HR

В последнее время мне бросили вызов код со следующими пунктами:

  
  1. Расширяемый код для поддержки различных правил ежегодного отпуска для отделов кадров

  2.   
  3. Поддерживаемый код для добавления /изменения существующих правил без какого-либо воздействия на других клиентов

  4.   
  5. Настраиваемый и настраиваемый код для разных клиентов

  6.   
  7. Обработка и регистрация исключений для защиты кода, а также упрощения поддержки

  8.   
  9. Следуя принципам проектирования и ООП

  10.   
  11. Единичный тестируемый код

  12.   

Я закодировал, сохраняя принцип SOLID, и вот мой код. Что я упустил? Я не смог получить хороший результат. Какие стратегии дизайна мне не хватает?

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;

        }
    }

И ниже мое подразделениеТест.

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


        }
    }
27 голосов | спросил Simsons 20 TueEurope/Moscow2016-12-20T13:06:46+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 13:06:46 +0300 2016, 13:06:46

5 ответов


40
  

Я закодировал, сохраняя принцип SOLID, и вот мой код. Что я пропустил?

Давайте посмотрим ...


SRP - принцип единой ответственности

  

У класса должна быть только одна причина для изменения.

Нарушается. Имя EmployeeLeave предполагает, что это класс, который просто хранит некоторые данные о выходе сотрудника, но его API говорит что-то еще - я - репозиторий. Так что это?

У вашего класса есть две причины для изменения:

  • правила запроса
  • сохранить запрос

OCP - Открытый /Закрытый принцип

  

Программные объекты (классы, модули, функции и т. д.) должны быть открыты для расширения, но закрыты для модификации.

Нарушается. У вас есть жестко закодированные все детали хранимой процедуры, хотя вы передаете абстракцию базы данных через DI.

Пользователь IDataBaseService не должен знать, что он работает с хранимой процедурой или чем-то еще. Пользователь просто хочет сохранить свои данные.

Вы не можете расширять его, используя другой тип хранилища. Если вы удалите хранимую процедуру, вы нарушите эту реализацию.

LSP - Принцип замены Лискова

  

Детские классы не должны прерывать определения типа родительского класса.

Не имеет значения.

ISP - Принцип сепарации интерфейса

  

Принцип разделения интерфейса (ISP) гласит, что ни один клиент не должен зависеть от методов, которые он не использует.

Нарушается. Как человек, который создает запросы на отпуск и реализует ILeaveRequest, мне нужно реализовать SaveLeaveRequest и FindEmployee. Мне это не нужно. Я просто хочу создать новый запрос. Я не хочу знать, как его сохранить (по крайней мере, не здесь).

DIP - Принцип инверсии зависимостей

  

а. Модули высокого уровня не должны зависеть от модулей низкого уровня. Оба должны зависеть от абстракций.   B. Абстракции не должны зависеть от деталей. Детали должны зависеть от абстракций.

Нарушается. SaveLeaveRequest зависит от хранимой процедуры низкого уровня, хотя используется абстракция IDataBaseService.


Резюме

  

Расширяемый код для поддержки различных правил ежегодного отпуска для отделов кадров

Не удалось. Вам необходимо реализовать логику сохранения /поиска для каждого запроса на выход. Это много избыточного кода.

  

Поддерживаемый код для добавления /изменения существующих правил без какого-либо влияния на других клиентов

Не удалось. Вызов хранимой процедуры и ее данные относятся к репозиторию. Пользователь не должен знать, как он реализован, и в настоящее время ему нужно точно знать детали реализации, чтобы иметь возможность использовать его.

  

Настраиваемый и настраиваемый код для разных клиентов

Частично встречается. Вы начали с слоя данных, но обнаружили много деталей о хранилище для внешнего мира.

  

Обработка исключений и протоколирование для защиты кода, а также упрощение поддержки   Следуя принципам проектирования и ООП

Не удалось. пустой catch не является обработкой исключений. Сообщения об ошибках не очень полезны. Они не помогают решить проблему, объясняя причину или подсказку, как ее исправить.

  

Единичный тестируемый код

Частично встречается. Вы можете ввести другой уровень данных, но реализация EmployeeLeave будет нарушена, если новый слой данных не поддерживает жестко закодированную хранимая процедуру.


Решение (пример)

Интерфейс является хорошим началом, но он слишком велик и ему не хватает некоторых важных свойств, которые являются частью подписи ProcessLeaveRequest, но не должны быть.

Минимальный интерфейс должен содержать некоторые базовые данные и метод для проверки этогоданных.

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

Вы реализуете его, реализуя фактически только метод Validate, добавляя любые другие зависимости через DI, если, например, вам нужно проверить, может ли сотрудник все же оставить отпуск.

Обратите внимание на новые типы исключений и сообщения, которые четко объясняют, почему запрос недействителен.

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.");
    }
}

Вы можете создавать несколько запросов, добавляя больше свойств и зависимостей для более сложных правил - или меньше.

Это простой пример, но в более сложном решении вы можете иметь ILeaveRequestRule, который вы передаете через DI в конкретный запрос в виде набора из нескольких правил, чтобы вы могли их расширить. В таком случае каждое правило бросает осмысленное исключение, объясняющее нарушение. Все зависит от того, насколько динамична система. Если вы считаете, что вам, возможно, придется часто их менять, это, вероятно, будет кстати.

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...
    }
}

Чтобы создать все разные запросы на отпуск, вы должны написать фабрику (не включенную в пример).


Наконец, простой запрос на отправку запроса проверяет каждый запрос и сохраняет его в репозитории.

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

Репозиторий отпуска может, конечно, изменять различные таблицы, чтобы запросы на отпуск имели доступ к этим данным, если им нужно, чтобы проверить их правила.


Вот некоторые из преимуществ нового решения:

  • Вы можете создавать новые запросы на отпуск в любое время, не нарушая ничего с помощью любой логики.
  • Вы выполняете только то, что вам действительно нужно для нового запроса на отпуск
  • Вы всегда знаете, что и почему это не сработало.
  • Вы не зависимы от типа хранилища
  • Вы можете протестировать все устройства, высмеивая репозитории

Приложение - Исключения против ValidationResult

Существуют разные мнения о том, следует ли использовать исключения или результаты проверки для такой вещи.

На самом деле я использую оба шаблона. Это иногда личное или проектное предпочтение, которое вы выбираете.

ответил t3chb0t 20 TueEurope/Moscow2016-12-20T19:40:54+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 19:40:54 +0300 2016, 19:40:54
17
public class EmployeeLeave : ILeaveRequest
...
var leaveRequest = new EmployeeLeaveDetail();

Это сбивает с толку. Идя по имени, ILeaveRequest подразумевает, что класс, реализующий этот интерфейс, является «запросом на отпуск» (я знаю, что у классов нет отношения «есть» с интерфейсами). Но затем мы видим запрос actual оставить в виде EmployeeLeaveDetail. Этот класс EmployeeLeave просто обрабатывает запросы на отправку. Итак, почему этот класс реализует интерфейс под названием ILeaveRequest, когда он не является запросом и не реализует интерфейс, совместимый с тем, что можно ожидать от запроса на отпуск? И в этом случае, если EmployeeLeaveDetail - это запрос на отпуск (в соответствии с именем вашей переменной), почему он называется EmployeeLeaveDetail, а не EmployeeLeaveRequest? Я вижу, что вы создали интерфейс под названием ILeaveRequestDetail. Вы должны переименовать этот интерфейс в ILeaveRequest и переименовать текущий ILeaveRequest в нечто более точное.

if ((DateTime.Now - employee.StartDate).TotalDays <= 90 && !employee.IsMarried)
  throw new Exception("Invalid leave request.");

if (days > 20)
  throw new Exception("Invalid leave request.");

В чем смысл 90 и 20? Не используйте магические числа, создавайте именованные константы и используйте их вместо.

Также, как сказал Хеслачер, не бросайте Exception; используйте один из более конкретных типов или создайте свои собственные исключения для домена. И, по крайней мере, скажите, что не так, иначе вы почесаете голову о том, какая из ваших (потенциально) 20 проверок завершилась неудачно, когда вы получите Exception, и он просто говорит Invalid leave request.

  

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

Я беру некоторую проблему с именем этого метода, потому что это не обработка (независимо от того, что это означает) «запрос на отпуск». У вас есть метод под ним, SaveLeaveRequest(ILeaveRequestDetail), который фактически имеет дело с запросом на отпуск. ProcessLeaveRequest имеет дело только с ints, строками и т. д.

То, что я думаю, должно быть здесь, заключается в том, что у вас есть метод в каком-то классе для создания запроса на отпуск для сотрудника. Этот метод принимает ints, строки и т. Д., Выполняет проверку и возвращает запрос на отпуск. Затем вы можете вызвать метод Save для сохранения запроса.

Кроме того, в общем случае вы должны больше использовать конструкторы домена. Например, вы принимаете int для идентификатора сотрудника. Зачем? Разумеется, в этот момент вы должны были уже создать свой Employee - создать запрос нельзя, не выбрав, какой Employee создать для него - так что при создании запроса вы можете просто перейдите в Employee, который устраняет необходимость поиска и потенциально не удается создать запрос.

  
  1. Расширяемый код для поддержки различных правил ежегодного отпуска для HR   ведомства.
  2.   
  3. Поддерживаемый код для добавления /изменения существующих правил   без какого-либо влияния на других клиентов.
  4.   

Ваши правила:

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

Каким образом жестко кодирование правил в единый метод, который создает и проверяет запросы на отпуск, отвечает этим требованиям? Вы полностью проигнорировали требование 1.


В целом этот класс чувствует себя такдолжен быть хранилищем, учитывая всю работу с базой данных, которую он выполняет.

ответил eurotrash 20 TueEurope/Moscow2016-12-20T15:18:11+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 15:18:11 +0300 2016, 15:18:11
14
catch (Exception)
{

    throw;
}  

Хорошо, это не купит вам ничего, кроме нескольких строк кода. Просто удалите try..catch вообще. Эта конструкция просто позволяет исключению пузыря вверх по стеку вызовов, как если бы у вас не было try..catch вообще.


public void ProcessLeaveRequest

  • Вы не проверяете все аргументы метода.
  • Аргумент reason не используется вообще.
  • Вы выбрасываете Exception вместо e.g ArgumentOutOfRangeException, который лучше подходит.
  • кажется (из метода FindEmployee()), что Employee является структурой (hint default(Employee))), что позволяет что вы добавляете что-то в свою базу данных, которая там не принадлежит. Предположим, вы передали epmloyeeId, который не существует в методе FindEmployee(), который вы получили бы default(Employee), который в случай структуры будет иметь значения свойств по умолчанию для StartDate и IsMarried. Если эти свойства будут считаться действительными в вашей части validateion, вы получите EmployeeLeaveDetail в своей базе данных для сотрудника, которого не существует.

Вы всегда должны использовать фигурные скобки {}, хотя они могут быть необязательными. Не используя их, вы можете ввести ошибки, которые трудно отслеживать. Я хотел бы призвать вас всегда использовать их. Это поможет сделать ваш код менее подверженным ошибкам.

Это предназначено для if, else if и else.

ответил Heslacher 20 TueEurope/Moscow2016-12-20T14:37:40+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 14:37:40 +0300 2016, 14:37:40
9

Здесь много полезных комментариев, но никто не комментировал использование DateTime.

EmployeeLeave

  

DateTime.Now - employee.StartDate

Я призываю вас обрабатывать все даты в формате UTC и только конвертировать их в локальное время, когда вам нужно отображать. Даже если вы подали в суд, ваше приложение будет использоваться в одном часовом поясе, в котором вы избавитесь от большого количества хлопот в будущем, если вы выделите, что вам нужно поддерживать часовые пояса. Второй - в тестах, и я вернусь к этому в конце.

Другая проблема с этим классом заключается в том, что он делает слишком много. Вы нарушаете SRP (принцип единой ответственности). Этот класс обрабатывает праздники, находит сотрудника и проверяет правильность праздников. Я думаю, вы должны разделить это на несколько классов, каждый из которых отвечает за простую задачу. Затем вы можете вставить их в свой класс EmployeeLeave и только построить логику, вызвав определенные методы.

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;
}

Каждый интерфейс представляет собой так называемый ролевой интерфейс , который имеет только метод (методы) для этой конкретной роли. Итак, в приведенном выше примере:

  • IEmployeeLeaveStory - будет содержать только метод сохранения объекта отпуска сотрудника
  • IEmployeeFinder - с помощью одного метода Find
  • IHolidayValidator - с помощью одного метода IsValid

Мне было бы полезно создать единые правила для проверки правильности праздников, а затем составить агрегированный праздничный валидатор, который будет выполнять только IsValid для своих детей. Это может выглядеть так:

var compositeValidator = new CompositeLeaveValidator(
    new NoMoreThanTwentyDaysValidator(), 
    new MarriedAndEmployedForLessThan3MonthsValidator());

Вы также можете создать композицию на основе типа сотрудника, поскольку применимы, возможно, разные правила. Это также хороший способ расширить с помощью новых правил.

Кроме того, в конструкторе мы проверяем, что все параметры не являются нулевыми, и в случае, если они являются Fail fast, что тоже хорошо делать.

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

Я также хотел бы выделить это EmployeeLeaveDetail для отдельного класса, но это зависит от вас.

Как я уже упоминал выше, есть также одна проблема с DateTime. На этот раз в модульных тестах.

UnitTest

В основном из-за того, что вы используете DateTime.Now (или UtcNow, как вы должны) в ProcessLeaveRequest, что означает, что каждый раз вы запускаете свой тест для этого метода, вы запускаете различные тесты в качестве DateTime. Лучшим подходом к этому было бы созданиеSystemTime, как показано ниже.

public static class SystemTime
{
    public static Func<DateTime> Now = () => DateTime.UtcNow;
}

, а затем в своем тесте вы можете указать, какова текущая дата-время, когда тест должен выполняться, и не выполняйте DateTime, когда тест был запущен.

[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.
}

Вы также можете использовать этот класс везде, где вам нужно, чтобы получить DateTime. Таким образом, вы уверены, что везде, где вы используете UTC или не UTC, и вы согласны.

Кроме того, отметьте Пять простых спасителей на летнее время. NET Developers , так как могут возникнуть проблемы с DST, когда вы выполняете вычисления в DateTime.

ответил Paweł Łukasik 20 TueEurope/Moscow2016-12-20T22:15:23+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 22:15:23 +0300 2016, 22:15:23
8

Вообще говоря, модульные тесты должны следовать «Arrange, Act, Assert» -pattern. У вас есть только «Упорядочить, действовать». Вы ничего не утверждаете (за исключением того, в котором вы ожидаете, что будет выбрано исключение).

[TestMethod]
public void IsMarriedAndLessThan90Days()
{
    // Arrange
    EmployeeLeave leave = new EmployeeLeave(new SQLDatabaseService());
    // Act
    leave.ProcessLeaveRequest(DateTime.Now, 6, "", 454);
    // Assert?
}

Ваши модульные тесты зависят от вашей базы данных. Вы должны стараться избегать любых внешних зависимостей в ваших модульных тестах.


Связано с приведенным выше пунктом: вы тестируете свою функциональность с существующими пользователями. Если что-то изменится в вашей базе данных (кто-то женится, стартовая дата более 90 дней назад, ...), вы должны переписать свои тесты. Вы должны явно создать свой Employees в своих тестовых случаях, чтобы они не менялись, а кто-то еще мог легко видеть, что происходит.


По-моему, ваш EmployeeLeaveRequest знает многое о том, как он будет сохранен. В вашем интерфейсе IDatabaseService должны быть такие методы, как void SaveLeaveRequest(ILeaveRequest request), где вы просто передаете свой запрос и позволяете ему обрабатывать весь SQL или что-то еще, что необходимо для сохранения запросов. Аналогично FindEmployee. Это не должно быть EmployeeLeaveRequest для извлечения записи сотрудника из базы данных.

ответил germi 20 TueEurope/Moscow2016-12-20T19:07:44+03:00Europe/Moscow12bEurope/MoscowTue, 20 Dec 2016 19:07:44 +0300 2016, 19:07:44

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132