Конструктивная критика в этом классе - PullRequest
Новая
       1

Конструктивная критика в этом классе

4 голосов
/ 21 сентября 2009

Я только что просмотрел код, который выглядел так, как раньше 

public class ProductChecker
{
     // some std stuff
     public ProductChecker(int AccountNumber)
     {
         var account = new AccountPersonalDetails(AccountNumber);
         //Get some info from account and populate class fields
     } 
     public bool ProductACriteriaPassed()
     {
          //return some criteria based on stuff in account class 
          //but now accessible in private fields
     }

}

Теперь добавлены некоторые дополнительные критерии, для которых нужны данные, отсутствующие в классе AccountPersonalDetails

новый код выглядит так 

public class ProductChecker
{
     // some std stuff
     public ProductChecker(int AccountNumber)
     {
         var account = new AccountPersonalDetails(AccountNumber);
         var otherinfo = getOtherInfo(AccountNumber)
         //Get some info from account and populate class fields
     } 
     public bool ProductACriteriaPassed()
     {
          //return some criteria based on stuff in account class  
          // but now accessible in private fields and other info
     }

     public otherinfo getOtherInfo(int AccountNumber)
     {
        //DIRECT CALL TO DB TO GET OTHERINFO 
     }

}

Меня беспокоит часть БД, но могут ли люди объяснить мне, почему это не так? Или это?

Ответы [ 7 ]

6 голосов
/ 21 сентября 2009

В многоуровневом представлении вашей системы это выглядит так, как будто ProductChecker относится к слою (слоям) бизнес-правил / бизнес-логики, поэтому его не следует "загрязнять" ни одной из функций взаимодействия с пользователем (которая принадлежит слою). (s) выше ) или - и это уместно для вашего случая - функциональность хранения (которая относится к слою (слоям) ниже ).

«Другая информация» должна быть инкапсулирована в своем собственном классе для уровней хранения, и этот класс должен быть тем, который обрабатывает функции сохранения / извлечения (точно так же, как я представляю, что AccountPersonalDetails делает для своих собственных вещей). Являются ли «личные данные» и «другая информация» лучше всего разделенными или объединенными в один класс, я не могу сказать по представленной информации, но этот вариант следует критически рассмотреть и тщательно взвесить.

Практическое правило хранения отдельных слоев может временами казаться жестким, и часто бывает заманчиво сократить его, чтобы добавить функцию путем смешения слоев - но чтобы поддерживать вашу систему в обслуживании и чистоте по мере ее роста, я почти неизменно приводить доводы в пользу разделения слоев всякий раз, когда возникает такая проблема проектирования. В терминах ООП это говорит о «сильной сплоченности, но слабой связи»; но в некотором смысле он более фундаментален, чем ООП, поскольку он также применим к другим парадигмам программирования и их сочетаниям! -)

1 голос
/ 21 сентября 2009

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

Прежде всего, если объект OtherInfo относится к Учетной записи, а не к Продукту, который вы проверяете ... он вводит обязанности для вашего класса, которых там быть не должно.

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

В-третьих, я не уверен, что метод GetOtherInfo должен быть открытым. Похоже, что-то, что должно использоваться только внутри вашего класса (если, на самом деле, оно на самом деле принадлежит там с самого начала). В этом случае вам также не нужно передавать идентификатор accountId (ваш класс должен уже где-то его хранить).

Но ... если OtherInfo относится к продукту, который вы проверяете, и у вас нет реального уровня доступа к данным, тогда я вижу, как это может быть действительным проектом.

Тем не менее, я на вашей стороне. Мне это не нравится.

1 голос
/ 21 сентября 2009

Кажется, что дополнительные данные, захваченные в getOtherInfo, должны быть инкапсулированы как часть класса AccountPersonalDetails и, таким образом, уже являются частью вашей переменной account в конструкторе при создании объекта new AccountPersonalDetails. Вы передаете AccountNumber обоим, так почему бы не сделать AccountPersonalDetails сбор всех необходимой вам информации? Тогда вам не придется прибегать к дополнительным вещам извне, как вы делаете сейчас.

0 голосов
/ 21 сентября 2009

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

Это "кажется" неуклюжим для создания нового экземпляра класса для каждого номера учетной записи. Аргумент конструктора должен быть чем-то необходимым для правильного поведения класса. Это параметр класса, а не зависимость. Это приводит к искушению проделать большую работу в конструкторе. Использование класса должно выглядеть так: 

result = new ProductChecker().ProductACriteriaPassed(accountNumber)

Который я бы быстро переименовал, чтобы указать, что он работает. 

result = new ProductChecker().PassesProductACriteria(accountNumber)

Несколько других отметили, что вы можете разделить логику базы данных. Вы хотели бы сделать это, если вы хотите быстрые юнит-тесты. Большинству программ нужны юнит-тесты (если только вы не играете), и они лучше, если они быстрые. Они быстры, когда вы можете убрать базу данных с дороги.

Давайте создадим фиктивный объект, представляющий результаты базы данных, и передадим его методу, который определяет, проходит ли продукт. Если бы не тестируемость, это было бы личное. Тестируемость побеждает. Предположим, я хочу проверить правило, такое как «продукт должен быть зеленым, если номер счета простое». Такой подход к модульному тестированию прекрасно работает без красивой инфраструктуры. 

// Maybe this is just a number of items. 
DataRequiredToEvaluateProduct data =  // Fill in data
// Yes, the next method call could be static. 
result = new ProductChecker().CheckCriteria(accountNumber, data)
// Assert result

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

public class ProductRepository {} // Define data access here. 

// Use the ProductChecker as follows. 
result = new ProductChecker(new ProductRepository()).CheckCriteria(accountNumber)

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

result = ProductCheckerFactory().GimmeProductChecker().CheckCriteria(accountNumber)

Пока что я не использовал какой-либо инфраструктурный код. Как правило, мы делаем это проще и красивее с помощью насмешек и внедрения зависимостей (я использую rhinomocks и autofac ). Я не буду вдаваться в это. Это проще, если оно уже у вас на месте.

0 голосов
/ 21 сентября 2009

Я бы порекомендовал это: 

public class AccountPersonalDetails
{
   public OtherInfo OtherInfo { get; private set; }
}

public class ProductChecker
{
     public ProductChecker(AccountPersonalDetails) {}
}

// and here's the important piece
public class EitherServiceOrRepository
{
   public static AccountPersonalDetails GetAccountDetailsByNumber(int accountNumber)
   {
       // access db here
   }

   // you may also feel like a bit more convinience via helpers
   // this may be inside ProductCheckerService, though
   public static ProductChecker GetProductChecker(int accountNumber)
   {
      return new ProductChecker(GetAccountDetailsByNumber(accountNumber));
   }
}

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

0 голосов
/ 21 сентября 2009

Несколько баллов

  • Имена параметров - в паскалях, а не в верблюде (возможно, это ошибка)
  • getOtherInfo() похоже, что это ответственность AccountPersonalDetails и так должно быть в этом классе
  • Вы можете использовать класс Façade или шаблон Repository для получения вашего AccountPersonalDetails вместо использования конструктора
  • getOtherInfo() также может иметь отношение к этому рефакторингу, поэтому логика базы данных не встроена в объект домена, а в класс обслуживания (фасад / хранилище)
  • ProductACriteriaPassed() находится в нужном месте
0 голосов
/ 21 сентября 2009

учитывая, что accountNumber был передан в конструктор, вам не нужно передавать его другому методу, подобному этому.

...