- 01
- 02
- 03
- 04
- 05
- 06
- 07
- 08
- 09
- 10
- 11
- 12
- 13
public int GetModuleId(int userId)
{
return moduleIdGet(userId);
}
protected int moduleIdGet(int userId)
{
int moduleId;
// calculate moduleId
// ...
return moduleId;
}
Нашли или выдавили из себя код, который нельзя назвать нормальным, на который без улыбки не взглянешь? Не торопитесь его удалять или рефакторить, — запостите его на говнокод.ру, посмеёмся вместе!
+136
public int GetModuleId(int userId)
{
return moduleIdGet(userId);
}
protected int moduleIdGet(int userId)
{
int moduleId;
// calculate moduleId
// ...
return moduleId;
}
Дал открытый доступ, но в то же время как бы сохранил защищённый.
3Doomer 29.01.2014 14:57 # 0
bormand 29.01.2014 15:12 # +4
А вот следующий ход - уже говно. Имхо, достаточно было поменять протектед на паблик, а не городить костыль со вторым методом...
Dummy00001 29.01.2014 17:32 # 0
А сужать то зачем? Если другие девелы пользуются, значит им это нужно. Значит код полезный.
Единственные причины которые знаю это повыпендриватся и/или кому-то падлу сделать. И это ненормально.
bormand 29.01.2014 17:50 # +1
Ну вот дошло до разраба, что сдуру выставил что-то лишнее в паблик. Человеку свойственно ошибаться. Если это прога, а не либа - вполне можно загрепать вызовы и сузить, пока на эту фичу не сильно завязались.
> Если другие девелы пользуются
То поздно пить боржоми, разве что объявить как deprecated ;( О чем я говорил: расширить - да запросто, была бы веская причина, сузить - 7 кругов ада, и 5 лет поддержки.
В libpng (вроде бы в ней) сколько лет пытались переделать сдуру выставленые напоказ поля структуры на геттеры... Как бы не больше 5 лет... И один хрен народ опомнился только тогда, когда эти поля "внезапно" пропали.
Dummy00001 29.01.2014 18:22 # 0
> То поздно пить боржоми, разве что объявить как deprecated
Как у нас на проекте: в одной либе есть метод deprecated который уже как 5й год депрекэйтед. А толку, если другого способа доступится к функциональности так никто и не придумал.
Или как в одной либе хэш функция для генерации номеров. С самого начала сделали внутренней, что бы типа а не хер. 11 лет спустя, в проекте есть как минимум пять копий оного хэша. Хотел поменять на более эффективный - а мля обломись.
> В libpng (вроде бы в ней) сколько лет пытались переделать сдуру выставленые напоказ поля структуры на геттеры...
Это разрабы как-то по GNOME-овски поступли. Чисто технически, такое как раз решается просто: если ломаешь публичный интерфейс, так ломай его полностью, и переводи в новый нэймспэйс. Старый интерфейс просто делается как враппер поверху нового.
wissenstein 29.01.2014 18:26 # 0
Ну, это уже неграмотное перепроектирование. Зочэм тогда его депрецировать, если не знать, чем заменить?
Dummy00001 29.01.2014 18:40 # 0
А причем тут то проектирование? Развешивают public/protected/private на методы и аттрибуты сами девелоперы, авторы кода. И у каждого своя идея как оно должно в идеале работать. И public/protected/private/deprecated более или менее единственное средство как они могут свой идеал до других донести.
На ютюбе как-то презентацию видел. Там CTO какой-то мелкой фирмы на тему почему у них в интерфейсах нету public/protected/private ответил "We are all adults here." Что в принципе и отражает мой сентимент. Политические игры с public/protected/private (и в жабе с deprecated) я видел только в ситуациах которые можно легко обобщить термином "детский сад".
bormand 29.01.2014 19:42 # +1
private это же деталь реализации, она по определению не может быть частью публичного API :)
package private в яве и friend в крестах - тоже деталь реализации и очень годная вещь, позволяющая не выставлять лишнего в паблик.
Ну а protected... Тут спорная тема. Юзать его чтобы "разрешить вызывать метод только потомкам" или чтобы "дать потомку доступ к полям" - ССЗБ (привет, QThread::sleep()). А вот если юзать его как простенький service provider interface - почему нет? Поясню, что имел в виду:
- Сабклассы никогда не вызывают protected методы, а только перекрывают их
- Все остальные работают с публичными методами, и даже не думают об этих protected'ах
Это дает какую-никакую развязку между теми кто сабклассит и теми кто юзает. И это чуть проще, чем полноценный SPI.
Так что они абсолютно правы ;)
P.S. Я поди криво выразил свою мысль?
roman-kashitsyn 29.01.2014 19:52 # +2
А вот правильный ответ на вопрос "зачем нужны приватные виртуальные функции в C++"
bormand 29.01.2014 20:04 # 0
bormand 29.01.2014 20:23 # 0
Т.е. protected нинужен?
P.S. Может кому-то пригодится: http://www.gotw.ca/publications/mill18.htm
3.14159265 29.01.2014 20:26 # +1
И еще больше того что дублирует другой функционал.
bormand 29.01.2014 20:39 # +2
Lure Of Chaos 29.01.2014 23:45 # 0
roman-kashitsyn 29.01.2014 21:08 # +1
Ну так это всё в рамках парадигмы: известно, что компилятор сначала выбирает функцию, а только потом проверяет уровни доступа. Поэтому, например, происходит вот такое:
Sabrian 04.03.2014 12:49 # 0
Dummy00001 29.01.2014 20:35 # 0
Так и я про то же!!!
Все к чему public/protected/privatе/deprecated приводят это к лишним спорам.
bormand 29.01.2014 20:36 # 0
TarasB 29.01.2014 20:38 # 0
Нужен гибкий способ подробного описания зоны видимости на запись и зоны видимости на чтение.
Dummy00001 29.01.2014 20:42 # 0
большая часть всех этих проблем происходит из-за того что народ копи-паздит откуда нипопади кривое использование класса/объекта, а потом менять не хочет потому что уже как бы протестили и зарелизили.
TarasB 29.01.2014 20:44 # 0
bormand 29.01.2014 20:47 # 0
Dummy00001 29.01.2014 20:50 # 0
Dummy00001 29.01.2014 20:43 # 0
паблик - виноват тем что не дефолт.
bormand 29.01.2014 20:44 # +1
В сишке то с пабликом и приватом все очень просто.
Dummy00001 29.01.2014 20:48 # 0
?
bormand 29.01.2014 20:53 # 0
В ашке пишем только публичный интерфейс, и одно приватное поле с указателем на структуру, в которой хранятся поля.
Кутишники юзают эту хреновину для бинарной совместимости, ну и чтобы лишние ашки типа виндовых или линуксовых хидеров не торчали из их API.
Большей изоляции паблика и привата я не видел ;(
Dummy00001 29.01.2014 20:57 # 0
А, да, знаю. Так а другого способа в С++ как бы публичные интерфейсы делать и нету.
Сам то я коммерческий софт пишу. Тут совместимость нужна только на уровне компиляции, поэтому pimpl'ом ни разу в офисе пользоваться не приходилось.
bormand 29.01.2014 21:00 # 0
Ну он все-таки для либ, не для прог.
roman-kashitsyn 29.01.2014 21:03 # 0
pimp часто может неплохо увеличить скорость этой самой компиляции.
bormand 29.01.2014 21:05 # +1
Да есть там способ... он даже идеологически правильней (с точки зрения ООП), но далеко не самый приятный, и не самый шустрый (хотя пимпл тоже не айс в плане удобства и скорости).
Интерфейсы с чистыми виртуальными функциями + фабричные методы, возвращающие эти интерфейсы в каком-нибудь смартпоинтере. Оп, оп, жаба стайл.
Dummy00001 29.01.2014 23:45 # 0
Можно.
Но в долгосрочных проектах это просто невозможно поддерживать, потому что добавлять в интерфейс новые (чисто) виртуальные функции нельзя. Приходится делать новый новый интерфейс (часто унаследованые от старого) и менять у пользователей имя интерфейса на новый. Вообщем, работает если только интерфейс стабильный.
bormand 29.01.2014 23:54 # 0
Скажи это майкрософту с их com ;)
Само собой интерфейсы менять нельзя, только добавлять новые и выпиливать старые, когда все на них забьют (если забьют).
Насчет неудобств - внутри одной подсистемы неудобно, на границе между подсистемами - почему нет?
Dummy00001 30.01.2014 00:26 # 0
Бррр... Ты о чем?
На внутренних интерфейсах - это оверкил.
На внешних интерфейсах - морока поддерживать, и долго не выживает.
К слову у нас к паре либ С++-тых основной метод работы через интерфейс 80+ С-style функций. Пару лет по граблям двоичной совместимости походили, и решили откатится на проверенные методы.
laMer007 30.01.2014 08:30 # 0
Если не ком и не си стайл, то что? сокеты? общая память? сериализация?
Dummy00001 30.01.2014 11:32 # 0
laMer007 30.01.2014 08:28 # 0
Dummy00001 30.01.2014 11:35 # +2
В том что старые интерфейсы когда-нибудь надо удалять. В коммерческом софте это почти всегда значит "никогда".
"Qt вывели какой-то свод правил [...]"
Не Qt, а KDE:
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++
laMer007 30.01.2014 11:48 # 0
в любом случае старый дропаешь и приложение больше не запускается.
потом через какое-то время старый интерфейс просто заменяете на заглушки с диалогом до свидания, ссылка на обновление.
laMer007 30.01.2014 11:49 # 0
Vindicar 29.01.2014 21:00 # 0
А можно с этого места поподробнее? Я вот наоборот, плохо понимаю зачем прятать что-то от потомков.
Иначе потом имеем антипаттерн "паблик морозов".
bormand 29.01.2014 21:06 # 0
А зачем прятать что-то от других, если оно доступно потомкам? :)
Вот этот QThread::sleep заныкали, когда идеология говорила "засабклассь тред и перекрой метод run()". А потом партия сказала "не надо сабклассить QThread, он не для того", а sleep() так и валялся в протектедах до пятой версии. И чтобы его юзать надо было есть кактус сабклассить тред чисто ради него.
А поля и прочие кишки давать потомкам - плохая идея. Потом будешь долго материться и рвать волосы, когда захочешь перепилить базовый класс.
bormand 29.01.2014 21:18 # 0
Vindicar 29.01.2014 21:18 # 0
Ну при архитектурных сдвигах по фазе уже ничто не поможет.
Имхо, имеют смысл три степени инкапсуляции.
1. Обычное инстанциирование - только паблик, чтобы не смущать пользователя кишками. Если класс не абстрактный, конечно.
2. Наследование с ожидаемым перекрытием методов - вот типа того-же Thread::run() во многих языках. Тоже паблик.
3. И наследование с серьёзными изменениями, не предусмотренными изначально. Тут protected, чтобы понапрасну обычные инстансы за эти кишки не дергали. Т.е. здесь держать только то, что менять можно, но только на свой страх и риск, и необходимость сабклассить должна служить тому предупреждением.
bormand 29.01.2014 21:24 # +1
run() это не часть публичного API треда, это часть, которую тред требует от своих потомков. Нахрена тут паблик? Ты же никогда не будешь вызывать этот run() сам.
> наследование с серьёзными изменениями, не предусмотренными изначально
Бедные люди, которым потом поддерживать эти "серьезные изменения"...
Vindicar 29.01.2014 22:10 # +1
Хм. Логично. Хотя мне нравится питоновская реализация, где по умолчанию Thread::run() просто вызывает заранее переданную необязательным параметром конструктора функцию.
Т.е. можно просто скормить тело потока реализации по умолчанию, а можно отсабклассить.
Впрочем, в этом случае паблик тоже незачем.
Убедил.
bormand 29.01.2014 21:27 # +2
Спорная вещь, кстати. Явно показывающая, что наследник не является предком, а паразитирует на нем.
Vindicar 29.01.2014 22:13 # +1
bormand 29.01.2014 22:24 # +3
О чем думали: Блин, лень агрегировать хештейбл, столько методов ему пробрасывать придется... лучше я от него унаследуюсь и получу все нахаляву.
Что получилось (затыкаем дырки в абстракции путем их документирования): Because Properties inherits from Hashtable, the put and putAll methods can be applied to a Properties object. Their use is strongly discouraged as they allow the caller to insert entries whose keys or values are not Strings. The setProperty method should be used instead. If the store or save method is called on a "compromised" Properties object that contains a non-String key or value, the call will fail. Similarly, the call to the propertyNames or list method will fail if it is called on a "compromised" Properties object that contains a non-String key.
Vindicar 30.01.2014 08:22 # 0
Типа такого (Питон т.к. Яву не знаю)
bormand 30.01.2014 09:18 # +1
А оно расширяет Hashtable<Object,Object>, а до этого вообще тупо Hashtable. Поэтому эти put() и putAll() будут работать с Object'ами, и обрезать их до String'ов уже не получится. Разве что исключение в рантайме кидать. А компайл-тайм проверку проебали в любом случае.
Vindicar 30.01.2014 12:36 # +1
Всё же скриптовые языки деформируют мышление.
roman-kashitsyn 30.01.2014 10:24 # +2
В любом случае, наследование ради реализации, а не ради интерфейса, ещё никого до добра не доводило.
Vindicar 30.01.2014 12:36 # +2
bormand 29.01.2014 22:52 # +2
Пойми, ты не пользователя этими кишками пугаешь... Да пользователь будет рад-радёхонек ими воспользоваться при любом удобном случае, если это как-то облегчит ему работу.
А вот тебе потом будет плохо, ибо изменить эти кишки ты уже не сможешь, т.к. они стали частью паблик интерфейса и вросли в кучу говнокода, который всем вломы править.
Vindicar 30.01.2014 08:26 # +1
Тут, конечно, можно отмазаться, мол, гарантируем обратную совместимость только на первых двух уровнях абстракции, если лезешь ниже - ССЗБ.
Но скорее всего такую либу пошлют и правильно сделают.
Sabrian 04.03.2014 12:47 # 0
bormand 29.01.2014 19:10 # +1
Ну депрекейтед как бы по определению должен предлагать альтернативу (UPD: чуть выше об этом уже написали)... Собственно "старый интерфейс как враппер поверх нового" это тоже своего рода deprecated и not recommended for new projects.
> 11 лет спустя, в проекте есть как минимум пять копий оного хэша
Ну дык можно было сделать этому генератору публичный API, когда он понадобился во втором месте... Причем, имхо, даже не выставлять этот генератор в паблик у того модуля, а вынести в отдельный, дабы не создавать ложную зависимость. Тот кто его изначально сделал приватным, имхо, не виноват, и делал все правильно.
> Старый интерфейс просто делается как враппер поверху нового.
А тут согласен.
Dummy00001 29.01.2014 20:38 # +1
Сделать можно все. Но разраб сказал что хэш функцию никому больше не надо знать - и точка.
Lure Of Chaos 29.01.2014 15:55 # 0
wissenstein 29.01.2014 15:59 # 0
Назначение видимости public вполне соответвует паттерну High Cohesion, когда мы группируем обязанности и распределяем их между несколькими специализированными обработчиками, так что нельзя сказать, что это само по себе говно.
Lure Of Chaos 29.01.2014 16:11 # 0
wissenstein 29.01.2014 16:14 # 0
Lure Of Chaos 29.01.2014 16:17 # 0
это как если бы ваша жена пошла бы на панель.
wissenstein 29.01.2014 16:33 # 0
Sabrian 04.03.2014 12:43 # 0