Здравствуйте, Dziman, Вы писали:

D>public void setEntityName(String entityName) {
D>  this.entity = loadFromDBByName(entityName);
D>  this.entityName = entityName;
D>}

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName

вне зависимости от того как оно могло бы быть, loadEntityByName — плохое имя, т.к. подразумевает action. в сущности оно уже есть и называется loadFromDBByName
правильной была бы рекомендация задуматься над тем, что метод фактически должен называться setEntityNameСЗагрузкойИПобочнойУстановкойEntity, и как этого избежать

D>На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения.

сильно зависит от сложности побочных действий
с моей точки зрения loadFromDBЧто-то — сложный метод, который обычно throws DataAccessException или SQLException или в этом духе
как следствие, на уровне объекта-контейнера данных тяжело его использовать корректно

D>Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу.

возможно, но здесь, скорее о другом

D>Или я что-то пропустил изучая ООП?

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

Для сеттера типа:

public void setEntityName(String entityName) {
  this.entity = loadFromDBByName(entityName);
  this.entityName = entityName;
}

На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?

добавление
например некий кулхацкер хочет сменить имя пользователя у какого-то обьекта с "вася иванов-раздолбай и двоешник" на "сисадмин".
через set_name(string)
в данном случае динамическим инвариантом обьекта является то-что
1. раздолбаи и двоешники не могут менять имя на сисадминов

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

Здравствуйте, Dziman, Вы писали:

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?
Пропустил. Сеттер — это как property в С#. Соответственно никакой логики он выполнять не должен, и генериться он должен автоматически средствами среды разработки, а не вручную.
По хорошему, таким образом, надо установить значения переменной посредствами сеттера, и далее дернуть метод loadEntityByName. Отдельно дернуть!!!! А то, студенты частично еще в сеттерах любят вызывать методы записи в базу или еще что похуже (круче всего, когда это в геттерах делают — нет слов! )

Здравствуйте, Dziman, Вы писали:

D>А геттер не должен проверять валидность данных, что ли ?

геттер — нет
контролем консистентности данных и инвариантов должен заниматься конструктор и сеттеры

Здравствуйте, elmal, Вы писали:

E>Пропустил. Сеттер — это как property в С#. Соответственно никакой логики он выполнять не должен, и генериться он должен автоматически средствами среды разработки, а не вручную.

Гхм… А как на счёт, например, отправки событий <PropertyName>Changed? Или, например, некоторых дополнительных действий (например, при изменении значения свойства Size у элемента управления может быть вызвана пачка функций из WinAPI)

E>По хорошему, таким образом, надо установить значения переменной посредствами сеттера, и далее дернуть метод loadEntityByName. Отдельно дернуть!!!!

Часто, наоборот, требуется скрыть от внешнего мира логику установки свойств и, возможно, обращение к БД вообще.

E>А то, студенты частично еще в сеттерах любят вызывать методы записи в базу

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

E>или еще что похуже (круче всего, когда это в геттерах делают — нет слов! )

Ленивая загрузка списка дочерних объектов так, часто, и делается

Здравствуйте, elmal, Вы писали:
E>Пропустил. Сеттер — это как property в С#. Соответственно никакой логики он выполнять не должен, и генериться он должен автоматически средствами среды разработки, а не вручную.
А геттер не должен проверять валидность данных, что ли ? Т.е. надо делать примерно так:

  someObj.validate(inputData);
  someObj.setData(inputData);

Здравствуйте, Dziman, Вы писали:

D>А геттер не должен проверять валидность данных, что ли ? Т.е. надо делать примерно так:
D>

D>  someObj.validate(inputData);
D>  someObj.setData(inputData);
D>

Иногда, для специфических случаев, в геттере для списка например имеет смысл проверять на null и в случае null преобразовывать к пустому списку допустим. Пользуюсь такой техникой около года, зачастую весьма неплохо упрощает жизнь, недостатков пока не заметил. Геттеры для entity бинов имеются ввиду. В сеттере в принципе проверить входные данные имеет смысл, но я чаще всего делаю централизованную валидацию для всех полей перед сохранением (а сеттер может дернуть ORM, и будет не очень хорошо, если приложение упадет из-за корявых данных в базе).
Но в основном геттеры и сеттеры делаю автогенерацией.

Здравствуйте, Dziman, Вы писали:

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?

Наверное, никто не ожидает от простого сеттера, что он будет лазить в базу данных, если только сам класс для этого не заводили.
Может, лучше сделать что-то вроде entity=null, чтоб было просто и кэшированные данные совпадали с именем.

Здравствуйте, Dziman, Вы писали:

D>Для сеттера типа:
D>

D>public void setEntityName(String entityName) {
D>  this.entity = loadFromDBByName(entityName);
D>  this.entityName = entityName;
D>}
D>

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?

Согласен с код ревью. Если я правильно понял, на листинге изображен класс-обёртка. Что ожидают от обёртки: что сеттеры устанавливают значения свойств подлежащего объекта (в данном случае entity). То есть код типа

cover.EntityDescription = "The entity description goes here";
cover.EntityName = "Name";

должен бы по идее устанавливать два поля Description и Name, а что оно сделает на самом деле? Если нет возможности присвоить entity новое имя, то и не должно быть сеттера (геттер может), если реализуется загрузка, то лучше делать это методом LoadEntity(String name), если есть или предполагается несколько методов загрузки, и загрузка по имени — частный случай, то логично добавить ByName.

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

P.S. Код ревью проводил не я, но я бы предъявил ровно такое же требование.

Здравствуйте, elmal, Вы писали:

E>Иногда, для специфических случаев, в геттере для списка например имеет смысл проверять на null и в случае null преобразовывать к пустому списку допустим.

Это не проверка, это преобразование данных в соответствие с оговоренным интерфейсом.

E>Геттеры для entity бинов имеются ввиду. В сеттере в принципе проверить входные данные имеет смысл, но я чаще всего делаю централизованную валидацию для всех полей перед сохранением (а сеттер может дернуть ORM, и будет не очень хорошо, если приложение упадет из-за корявых данных в базе).
E>Но в основном геттеры и сеттеры делаю автогенерацией.

А данные в базе, в которую лазят руками, кривые по определению.

Здравствуйте, Darth Jurassic, Вы писали:

DJ>Согласен с код ревью. Если я правильно понял, на листинге изображен класс-обёртка. Что ожидают от обёртки: что сеттеры устанавливают значения свойств подлежащего объекта (в данном случае entity).

А если ты неправильно понял?
Если entity — это закэшированное значение из базы данных, уникальным идентификатором которого является entityName?
Они установили новый entityName, а сам entity куда должен показывать? Если его загружать через loadEntity(),
то это везде в коде, перед использованием класса надо будет писать loadEntity() просто так, на всякий случай.
Это сделает код громоздким, неудобным, и главное — всё равно не будет гарантии, что где-то не забудут сделать
loadEntity(). А если сделать

getEntity() { if (entity == null) entity = loadEntity(); return entity; }

то точно так-же, неожиданно для пользователя getter-а, он начнёт лазить в базу данных и
занимать непредсказуемое кол-во времени, выдавать непонятные exception-ы и т.п.
Короче, хрен редьки не слаще.

Здравствуйте, _FRED_, Вы писали:

_FR>Гхм… А как на счёт, например, отправки событий <PropertyName>Changed? Или, например, некоторых дополнительных действий (например, при изменении значения свойства Size у элемента управления может быть вызвана пачка функций из WinAPI)
ИМХО для таких случаев лучше создать отдельный метод (например changeSize, который из себя дернет сеттер и вызовет другие какие-то обработчики. Причем еще и коммент было бы неплохо написать к этому changeSize, да и юнит тест бы не помешал). Отправку событий тоже неплохо было б написать обобщенно и прозрачно (например используя аннотации). Но вообще — обработка событий вроде всегда вешалась на контрол, а не на сеттер модели, модель ни о каких событиях знать не должна.

_FR>Часто, наоборот, требуется скрыть от внешнего мира логику установки свойств и, возможно, обращение к БД вообще.
Можно пример? То есть дергаем сеттер, выполняется insert в базу, дергаем геттер — выполняется select из базы? ИМХО не имеет такое никакого смысла, если только не для черти каких костылей.

_FR>В данном случае вызывается некий "load", который, в принципе, может в никакую БД не ходить и вернуть данные из кэша.
Обычно, если пишут подобным образом — значит с архитектурой какие-то очень серьезные проблемы (в микроархитектуре). В любом случае, та функциональность, которая была изначально должна быть реализована как угодно, но только не сеттером (в лоб я вижу простейшее решение — сначала дернуть сеттер, а потом дернуть метод загрузки из базы, на основании тех данных, которые содержатся в просетанных свойствах (в некоторых случаях приходится вот так извращаться на некоторых фреймворках, скорее всего необходимости извращаться в исходном случае нет вообще и от этого метода вообще следует избавиться (независимо от его названия), и есть решение проще ) )

E>>или еще что похуже (круче всего, когда это в геттерах делают — нет слов! )
_FR>Ленивая загрузка списка дочерних объектов так, часто, и делается
Вообще — ленивая загрузка да, так делается, но это по хорошему делается через proxy класс, который эту логику скрывает. Так поступает Hibernate, например. По хорошему, ленивую загрузку надо бы как-нибудь обобщенно написать, а не городить костыли на каждый геттер. Хотя признаюсь, был у меня проект, в котором пришлось вот так явно реализовывать ленивую загрузку (все было сделано криво, а к этому кривому надо было функциональность дописывать — вот тогда приходилось городить велосипеды и костыли)

Здравствуйте, elmal, Вы писали:

_FR>>Гхм… А как на счёт, например, отправки событий <PropertyName>Changed? Или, например, некоторых дополнительных действий (например, при изменении значения свойства Size у элемента управления может быть вызвана пачка функций из WinAPI)
E>ИМХО для таких случаев лучше создать отдельный метод (например changeSize, который из себя дернет сеттер и вызовет другие какие-то обработчики.

Не понимаю: почему "лучше"? Вот потому, что вызывающену не нужно знать о всяких оконных сообщениях, как раз-таки скрыть этот факт за свойством проще.

E>Причем еще и коммент было бы неплохо написать к этому changeSize, да и юнит тест бы не помешал).

Комент и тест на что?

E>Отправку событий тоже неплохо было б написать обобщенно и прозрачно (например используя аннотации).

Можно пример? Что за "аннотации"?

E>Но вообще — обработка событий вроде всегда вешалась на контрол, а не на сеттер модели, модель ни о каких событиях знать не должна.

А почему тут рассматривается "модель"? В корневом сообщении не было сказано ни слова о том, где рассматриваемое свойство находится. Я же говорю о конкретном свойстве Size класса System.Windows.Forms.Control, которое, будучи реализованным через метод было бы не столь удобным _в использовании_ как метод (который так же есть, но с несколько более удобной функциональностью).

_FR>>Часто, наоборот, требуется скрыть от внешнего мира логику установки свойств и, возможно, обращение к БД вообще.
E>Можно пример? То есть дергаем сеттер, выполняется insert в базу, дергаем геттер — выполняется select из базы? ИМХО не имеет такое никакого смысла, если только не для черти каких костылей.

"Дёргаем" сеттер, шлём (пускай даже асинхронно) некую команду "куда-то". Такая техника часто используется в различных проксях. Инсерт это в базу или смс агенту — дело внутренней реализации. Сейчас это может быть одно, завтра — другое. Если открытый интерфейс чего-то (в данном случае некоего класса) получается зависимым от способа хранения\обработки данных (если обработка простая — свойство, если сложная — метод), то это плохой интерфейс. Потому что менять интерфейс — тяжело, а вот реализацию куда как проще.

_FR>>В данном случае вызывается некий "load", который, в принципе, может в никакую БД не ходить и вернуть данные из кэша.
E>Обычно, если пишут подобным образом — значит с архитектурой какие-то очень серьезные проблемы (в микроархитектуре).

Каким именно образом? Какие проблемы?

E>В любом случае, та функциональность, которая была изначально должна быть реализована как угодно, но только не сеттером (в лоб я вижу простейшее решение — сначала дернуть сеттер, а потом дернуть метод загрузки из базы, на основании тех данных, которые содержатся в просетанных свойствах (в некоторых случаях приходится вот так извращаться на некоторых фреймворках, скорее всего необходимости извращаться в исходном случае нет вообще и от этого метода вообще следует избавиться (независимо от его названия), и есть решение проще ) )

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

E>>>или еще что похуже (круче всего, когда это в геттерах делают — нет слов! )
_FR>>Ленивая загрузка списка дочерних объектов так, часто, и делается
E>Вообще — ленивая загрузка да, так делается, но это по хорошему делается через proxy класс, который эту логику скрывает.

А какая разница — прямой там будет вызов или через прокси? Главное, что на "геттере" весит логика, которая куда-то ходит, и это, как оказывается, достаточно удобно.

Здравствуйте, _FRED_, Вы писали:

_FR>Не понимаю: почему "лучше"? Вот потому, что вызывающену не нужно знать о всяких оконных сообщениях, как раз-таки скрыть этот факт за свойством проще.
Плохо то, что часто уже в подсознании сидит, что если дергаем сеттер — я меняю внутреннее состояние объекта, касающееся только этого свойства.

E>>Причем еще и коммент было бы неплохо написать к этому changeSize, да и юнит тест бы не помешал).
_FR>Комент и тест на что?
На то, что кроме изменения переменной size кидается определенные события. Так как это неочевидное поведение. О котором надо помнить.
E>>Отправку событий тоже неплохо было б написать обобщенно и прозрачно (например используя аннотации).
_FR>Можно пример? Что за "аннотации"?
Забыл как это в C# называется, кажется в фигурных скобках — аттрибуты, если не изменяет память. Ну и этими аттрибутами и изменять поведение сеттера (я так полагаю, что аттрибут можно навесить и на property). Получается вполне элегантно и читаемо (в тех случаях, когда действительно необходимо менять поведение каким-то образом).

_FR>А почему тут рассматривается "модель"? В корневом сообщении не было сказано ни слова о том, где рассматриваемое свойство находится. Я же говорю о конкретном свойстве Size класса System.Windows.Forms.Control, которое, будучи реализованным через метод было бы не столь удобным _в использовании_ как метод (который так же есть, но с несколько более удобной функциональностью).
Метод у меня лично всегда ассоциируется с каким-либо действием. Свойство же с действием не ассоциируется, оно ассоциируется с описанием объекта. Удобно то может и удобно, но когда на сеттер логика навешена — возникает путаница, об этом поведении надо помнить. Ну и допустим — у меня есть контрол, у которого есть свойство size и свойство text. Мне надо поменять и то, и то, а затем перерисовать. Если мне вызов перерисовки дергать автоматом в сеттере, то получится лишний вызов — более оптимально сначала выставить, а потом один раз перерисовать .

_FR>>>В данном случае вызывается некий "load", который, в принципе, может в никакую БД не ходить и вернуть данные из кэша.
E>>Обычно, если пишут подобным образом — значит с архитектурой какие-то очень серьезные проблемы (в микроархитектуре).
_FR>Каким именно образом? Какие проблемы?
Так, из опыта . Время от времени в условиях спешки я иногда в черновом варианте добавляю логику на сеттер (и пишу черти какой комментарий к этому сеттеру). Но через некоторое время у меня всегда получается от этого уйти, и код в результате становится гораздо лучше — никакие комментарии не требуются, логика становится видна как на ладони.

_FR>В этом подходе как раз пагубно то, что когда изменится _внутренняя_ логика работы класса или способ хранения данных, придётся переписывать вызывающий код.
Почему переписывать? Из названия метода убрать изначально упоминание о способе хранения данных — и все будет хорошо. А во вторых, хорошим тоном для загрузки и сохранения в базу (отсылки СМС или еще чего) является дергание сервисов, которые уже и будут выполнять действия с объектом, для которых дергался сеттер до этого. Так что предложенное решение изначально скорее всего было плохое.

_FR>А какая разница — прямой там будет вызов или через прокси? Главное, что на "геттере" весит логика, которая куда-то ходит, и это, как оказывается, достаточно удобно.
Ну, это редкие случаи, например lazy связи.

Здравствуйте, Dziman, Вы писали:

D>Для сеттера типа:
D>

D>public void setEntityName(String entityName) {
D>  this.entity = loadFromDBByName(entityName);
D>  this.entityName = entityName;
D>}
D>

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?

Стилистика не соблюдается.

setEntityName не вполне описывает то что делает функция. Функция делает setEntityNameAndLoadEntity.

Если хочешь сделать отложенную загрузку entity то стоит навернео при обращении к entity загружать из базы а не при установке entityName.


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

Здравствуйте, Dziman, Вы писали:

D>Для сеттера типа:
D>

D>public void setEntityName(String entityName) {
D>  this.entity = loadFromDBByName(entityName);
D>  this.entityName = entityName;
D>}
D>

D>На код ревью было сделано замечание, что сеттер слишком сложный и надо переименовать метод в что-то типа loadEntityByName. На мой же взгляд это нормально, что сеттер что-то делает помимо чисто установки значения. Для того и задумывалось использование оберток надо членами класса, чтобы выполнять в них необходимую дополнительную работу. Или я что-то пропустил изучая ООП?

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

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

Здравствуйте, mkizub, Вы писали:

M>А если ты неправильно понял?
M>Если entity — это закэшированное значение из базы данных, уникальным идентификатором которого является entityName?

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