Сколько логики в добытчиках

46

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

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

Пример того, что я делаю:

public List<Stuff> getStuff()
{
   if (stuff == null || cacheInvalid())
   {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

Пример того, как работа говорит мне делать вещи (они цитируют «Чистый код» от дяди Боба):

public List<Stuff> getStuff()
{
    return stuff;
}

public void loadStuff()
{
    stuff = getStuffFromDatabase();
}

Сколько логики уместно в сеттере / геттере? Какая польза от пустых методов получения и установки, кроме нарушения сокрытия данных?

Маартен ван Люнен
источник
6
Для меня это больше похоже на tryGetStuff () ...
Билл Мичелл,
16
Это не «добытчик». Этот термин используется для средства доступа к свойству для чтения, а не для того, чтобы вы случайно указали в имени «get».
Борис Янков
6
Я не знаю, является ли этот второй пример хорошим примером этой чистой кодовой книги, о которой вы упомянули, или кто-то неправильно понимает, но одной вещью, которой нет хрупкого беспорядка, является чистый код.
Джон Ханна
@BorisYankov Ну ... второй способ. public List<Stuff> getStuff() { return stuff; }
Р. Шмитц
В зависимости от конкретного случая использования, мне нравится выделять мое кэширование в отдельный класс. Создайте StuffGetterинтерфейс, реализуйте StuffComputerвычисления, которые выполняют вычисления, и оберните его внутри объекта StuffCacher, который отвечает либо за доступ к кешу, либо за переадресацию вызовов на тот, StuffComputerкоторый он переносит.
Александр

Ответы:

71

То, как работа говорит вам делать что-то, хромает.

Как правило, я делаю так: если получение материала обходится в вычислительном отношении дешево (или, если есть большая вероятность, что он будет найден в кеше), тогда ваш стиль getStuff () подойдет. Если известно, что получение материала является вычислительно дорогостоящим, настолько дорогим, что реклама его дороговизны необходима на интерфейсе, то я бы не назвал его getStuff (), я бы назвал его CalculateStuff () или что-то в этом роде, чтобы указать, что там будет некоторая работа, чтобы сделать.

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

Майк Накис
источник
23
+1 за упоминание сложности порядка операций. В качестве обходного пути, возможно, работа попросит меня всегда вызывать loadStuff () в конструкторе, но это тоже будет плохо, потому что это означает, что его всегда нужно загружать. В первом примере данные загружаются лениво только тогда, когда это необходимо, и это настолько хорошо, насколько это возможно.
Лоран
6
Я обычно следую правилу «если это действительно дешево, используйте средство получения свойства. Если это дорого, используйте функцию». Обычно это хорошо мне подходит, и правильное название, как вы указали, подчеркивает, что это также хорошо для меня.
Денис Троллер
3
если он может потерпеть неудачу - это не добытчик. В этом случае, что, если ссылка на БД не работает?
Мартин Беккет
6
+1, я немного шокирован тем, как много неправильных ответов было опубликовано. Методы получения / установки существуют, чтобы скрыть детали реализации, в противном случае переменная должна быть просто сделана общедоступной.
Изката
2
Не забывайте, что требование вызова loadStuff()функции перед getStuff()функцией также означает, что класс неправильно абстрагирует то, что происходит под капотом.
rjzii
23

Логика в геттерах отлично работает.

Но получение данных из базы данных намного больше, чем «логика». Он включает в себя серию очень дорогих операций, в которых многое может пойти не так, и недетерминированным образом. Я бы не решился сделать это безоговорочно в получателе.

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

Майкл Боргвардт
источник
18

Я думаю, что согласно «Чистому коду» он должен быть как можно больше разбит на что-то вроде:

public List<Stuff> getStuff() {
   if (hasStuff()) {
       return stuff;
   }
   loadStuff();
   return stuff;
}

private boolean hasStuff() {
    if (stuff == null) {
       return false;
    }
    if (cacheInvalid()) {
       return false;        
    }
    return true;
} 

private void loadStuff() {
    stuff = getStuffFromDatabase();
}

Конечно, это полная чушь, учитывая, что красивая форма, которую вы написали, правильно делает с частью кода, который каждый понимает с первого взгляда:

public List<Stuff> getStuff() {
   if (stuff == null || cacheInvalid()) {
       stuff = getStuffFromDatabase();
   }
   return stuff;
}

Это не должно быть головной болью вызывающего абонента, как вещи попадают под капот, и особенно это не должно быть головной болью вызывающего абонента, чтобы не забыть назвать вещи в некотором произвольном «правильном порядке».

Joonas Pulakka
источник
8
-1. Настоящая головная боль будет, когда вызывающий абонент застрянет, выясняя, почему простой вызов getter привел к медленному доступу к базе данных.
Доменик
14
@ Domenic: Доступ к базе данных должен быть сделан в любом случае, вы не сохраняете никому производительность, не делая этого. Если вам это нужно List<Stuff>, есть только один способ получить это.
DeadMG
4
@lukas: Спасибо, я не помню всех трюков, использованных в «Чистом» коде, чтобы сделать тривиальные кусочки кода еще на одну строку длиннее ;-) Исправлено.
Joonas Pulakka
2
Вы клевещете на Роберта Мартина. Он никогда не расширит простое логическое дизъюнкция в функцию из девяти строк. Ваша функция hasStuffпротивоположна чистому коду.
Кевин Клайн
2
Я прочитал начало этого ответа, и я собирался обойти его, думая, что «идет еще один поклонник книги», и затем часть «Конечно, это полная чушь» попалась на глаза. Хорошо сказано! C -: =
Майк Накис
8

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

Должно быть столько логики, сколько необходимо для удовлетворения потребностей класса. Мое личное предпочтение заключается в том, чтобы как можно меньше, но при поддержке кода вам обычно приходится оставлять исходный интерфейс с существующими получателями / установщиками, но при этом использовать в них много логики для исправления более новой бизнес-логики (например, «клиенты»). «добытчик в среде после 911 года должен соответствовать правилам « знай своего клиента »и OFAC в сочетании с политикой компании, запрещающей появление клиентов из определенных стран [таких как Куба или Иран]).

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

Tangurena
источник
6

Вы правы, ваши коллеги не правы.

Забудьте общие правила о том, что метод get должен или не должен делать. Класс должен представлять абстракцию чего-либо. Ваш класс имеет читабельный stuff. В Java принято использовать методы get для чтения свойств. Были написаны миллиарды строк фреймворков, ожидающих чтения stuffпо вызову getStuff. Если вы назовете свою функцию fetchStuffили что-то еще getStuff, то ваш класс будет несовместим со всеми этими фреймворками.

Вы можете указать им Hibernate, где getStuff () может делать очень сложные вещи и выдает RuntimeException в случае сбоя.

Кевин Клайн
источник
Hibernate - это ORM, поэтому сам пакет выражает намерение. Это намерение не так легко понять, если сам пакет не является ORM.
FMJaguar
@FMJaguar: это легко понять. Hibernate абстрагирует операции базы данных, чтобы представить сеть объектов. OP абстрагирует операцию базы данных, чтобы представить объект с именем stuff. Оба скрывают детали, чтобы было проще написать вызывающий код.
Кевин Клайн
Если этот класс является классом ORM, то намерение уже выражено в других контекстах: остается вопрос: «Как другой программист узнает о побочных эффектах вызова метода получения?». Если программа содержит 1 тыс. Классов и 10 тыс. Получателей, политика, которая разрешает обращения к базам данных в любом из них, может стать проблемой
FMJaguar
4

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

List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

В отличие от:

myObject.load();
List<String> names = clientRoster.getNames();
List<String> emails = clientRoster.getEmails();

Или еще хуже

myObject.loadNames();
List<String> names = clientRoster.getNames();
myOjbect.loadEmails();
List<String> emails = clientRoster.getEmails();

Это просто делает другой код намного более избыточным и трудным для чтения, потому что вы должны начать пробираться через все подобные вызовы. Кроме того, вызов функций загрузчика или подобных объектов нарушает всю цель даже использования ООП, поскольку вы больше не отвлекаетесь от деталей реализации объекта, с которым вы работаете. Если у вас есть clientRosterобъект, вам не нужно заботиться о том getNames, как он работает, как если бы вам приходилось вызывать a loadNames, вы должны просто знать, что getNamesдает вам List<String>имена клиентов.

Таким образом, звучит так, будто проблема больше в семантике и в лучшем названии функции для получения данных. Если у компании (и других) есть проблема с префиксом getand set, то как насчет вызова функции как-то retrieveNamesвместо этого? Он говорит о том, что происходит, но не подразумевает, что операция будет мгновенной, как можно было бы ожидать от getметода.

С точки зрения логики в методе доступа, сведите его к минимуму, поскольку обычно подразумевается, что он является мгновенным, и с переменной происходит только номинальное взаимодействие. Однако это также обычно относится только к простым типам, сложным типам данных (то есть List). Мне труднее правильно инкапсулировать в свойство и обычно использовать другие методы для взаимодействия с ними, в отличие от строгого мутатора и метода доступа.

rjzii
источник
2

Вызов метода получения должен демонстрировать то же поведение, что и чтение поля:

  • Это должно быть дешево, чтобы получить значение
  • Если вы устанавливаете значение с помощью установщика, а затем читаете его с помощью получателя, значение должно быть таким же
  • Получение значения не должно иметь побочных эффектов
  • Не должно бросать исключение
Сьерд
источник
2
Я не совсем согласен с этим. Я согласен, что у него не должно быть побочных эффектов, но я думаю, что это прекрасно, чтобы реализовать его таким образом, чтобы отличать его от области. Глядя на .Net BCL, InvalidOperationException широко используется при рассмотрении методов получения. Также см. Ответ MikeNakis на порядок операций.
Макс.
Согласитесь со всеми пунктами, кроме последнего. Конечно, возможно, что получение значения может потребовать выполнения вычисления или какой-либо другой операции, которая зависит от других значений или ресурсов, которые, возможно, не были установлены. В этих случаях я ожидаю, что получатель выбросит какое-то исключение.
TMN
1
@ TMN: в лучшем случае класс должен быть организован таким образом, чтобы получателям не нужно было выполнять операции, способные генерировать исключения. Минимизация мест, где можно создавать исключения, приводит к менее неожиданным сюрпризам
hugomg
8
Я буду не согласен со второй точкой с конкретным примером: foo.setAngle(361); bar = foo.getAngle(). barможет быть 361, но это также может быть законным, 1если углы связаны с диапазоном.
zzzzBov
1
-1. (1) является дешевым в этом примере - после отложенной загрузки. (2) в настоящее время нет «сеттер» в примере, но если кто - то добавляет один за, и это только наборы stuffПоглотитель будет возвращать одинаковое значение. (3) Ленивая загрузка, как показано в примере, не вызывает «видимых» побочных эффектов. (4) является спорным, может быть, допустимым моментом, поскольку введение «отложенной загрузки» впоследствии может изменить прежний контракт API - но для принятия решения необходимо посмотреть на этот контракт.
Док Браун
2

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

Это не означает «никогда не обращаться к другому члену, который не является полем поддержки свойств в получателе», это просто означает, что следует обращать внимание на то, что вы подразумеваете, о том, какое состояние объекта требуется, и соответствует ли оно ожидаемому контексту это свойство должно быть доступно в.

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

Когда именно происходит инициализация, это может быть или не быть важным.

Например, это может быть очень очень медленно, и его нужно делать во время шага загрузки, когда пользователь ожидает задержку, а не из-за неожиданного падения производительности, когда пользователь впервые запускает доступ (т. Е. Пользователь щелкает правой кнопкой мыши, появляется контекстное меню, пользователь уже снова щелкнул правой кнопкой мыши).

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

Тем не менее, Lazy Initialization имеет большой смысл во многих других ситуациях. Таким образом, как это часто бывает в программировании, его трудно сводить к правилу, все сводится к конкретному коду.

Джеймс
источник
0

Просто сделайте это, как сказал @MikeNakis ... Если вы просто получаете материал, тогда все в порядке ... Если вы делаете что-то еще, создайте новую функцию, которая сделает эту работу, и сделайте ее общедоступной.

Если ваша собственность / функция делает только то, что написано в ее названии, тогда не остается места для осложнений. Сплоченность является ключевым ИМО

Иван Крояч Карачич
источник
1
Будьте осторожны с этим, вы можете обнажить слишком много вашего внутреннего состояния. Вы не хотите заканчивать с большим количеством пустых loadFoo()или preloadDummyReferences()или createDefaultValuesForUninitializedFields()методов только потому, что они нужны первоначальной реализации вашего класса.
TMN
Конечно ... Я просто говорил, что если вы делаете то, что в названии говорится, что проблем не должно быть много ... но то, что вы говорите, абсолютно верно ...
Иван Крояч Карачич
0

Лично я бы раскрыл требование Stuff через параметр в конструкторе и позволил бы любому классу создавать экземпляры для выполнения работы по выяснению того, откуда они берутся. Если свойство равно нулю, оно должно вернуть ноль. Я предпочитаю не пытаться использовать умные решения, такие как оригинал OP, потому что это простой способ скрыть ошибки глубоко внутри вашей реализации, когда совершенно не очевидно, что может пойти не так, когда что-то сломается.

EricBoersma
источник
0

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

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

  2. Если вы не можете создать такой класс, решите, есть ли у вас компромисс, и вам нужно решить, хотите ли вы разрешить потребителю пропустить код проверки кэша или нет.

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

    2. Если все в порядке, чтобы пропустить проверку кэша, или очень важно, чтобы вы гарантировали производительность O (1) в геттере, тогда используйте отдельные вызовы.

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

hugomg
источник
-1, конструкторы должны создавать, а не инициализировать. Помещение логики базы данных в конструктор делает этот класс полностью не тестируемым, и если у вас их больше, чем несколько, ваше время запуска приложения станет огромным. И это только для начала.
Доменик
@ Domenic: это семантическая и языковая проблема. Точка, в которой объект пригоден для использования и предоставляет соответствующие инварианты после и только после того, как он полностью построен.
hugomg
0

На мой взгляд, геттеры не должны иметь много логики в них. У них не должно быть побочных эффектов, и вы никогда не должны получать от них исключения. Если, конечно, вы не знаете, что делаете. У большинства моих добытчиков нет логики, и они просто идут на поле. Но заметным исключением из этого является публичный API, который должен быть максимально простым в использовании. Таким образом, у меня был один получатель, который потерпит неудачу, если бы другой получатель не был вызван. Решение? Строка кода, как var throwaway=MyGetter;в получателе, которая зависела от него. Я не горжусь этим, но я все еще не вижу более чистого способа сделать это

Earlz
источник
0

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

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

В зависимости от того, как cacheInvalid()работает подпрограмма, проверка на ноль может не потребоваться. Я не ожидал бы, что кэш будет действительным, если он stuffне был заполнен из базы данных.

BillThor
источник
0

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

что-то вроде:

public List<Stuff> getStuff()
{
    return Collections.unmodifiableList(stuff);
}

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

JK.
источник
0

В зависимости от конкретного случая использования, мне нравится выделять мое кэширование в отдельный класс. Создайте StuffGetterинтерфейс, реализуйте StuffComputerвычисления, которые выполняют вычисления, и оберните его внутри объекта StuffCacher, который отвечает либо за доступ к кешу, либо за переадресацию вызовов на тот, StuffComputerкоторый он переносит.

interface StuffGetter {
     public List<Stuff> getStuff();
}

class StuffComputer implements StuffGetter {
     public List<Stuff> getStuff() {
         getStuffFromDatabase()
     }
}

class StuffCacher implements StuffGetter {
     private stuffComputer; // DI this
     private Cache<List<Stuff>> cache = new Cache<>();

     public List<Stuff> getStuff() {
         if cache.hasStuff() {
             return cache.getStuff();
         }

         List<Stuffs> stuffs = stuffComputer.getStuff();
         cache.store(stuffs);
         return stuffs;
     }
}

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

Александр
источник
-1

ИМХО это очень просто, если вы используете дизайн по контракту. Решите, что должен обеспечить ваш геттер, и просто запишите соответствующий код (простой код или некоторая сложная логика, которая может быть задействована или где-то делегирована)

Kemoda
источник
+1: я с тобой согласен! Если объект предназначен только для хранения некоторых данных, то получатели должны возвращать только текущее содержимое объекта. В этом случае ответственность за загрузку данных лежит на другом объекте. Если в контракте говорится, что объект является прокси-сервером записи базы данных, то получатель должен получать данные на лету. Это может стать еще сложнее, если данные были загружены, но не обновлены: должен ли объект быть уведомлен об изменениях в базе данных? Я думаю, что нет уникального ответа на этот вопрос.
Джорджио