1. C# / Говнокод #14439

    +136

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    public int GetModuleId(int userId)
    {
        return moduleIdGet(userId);
    }
    
    protected int moduleIdGet(int userId)
    {
        int moduleId;
        // calculate moduleId
        // ...
    
        return moduleId;
    }

    Дал открытый доступ, но в то же время как бы сохранил защищённый.

    Запостил: wissenstein, 29 Января 2014

    Комментарии (66) RSS

    • Говно...Зачем геттер(хоть и не в привычном смысле слова, да) защищать?
      Ответить
      • Ну может быть автор кода считал, что вне данного класса и его потомков этот id никому не понадобится. Вот и сделал его protected'ом. Нормальная практика, т.к. расширить видимость всегда можно, а вот сузить - часто уже нереально.

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

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

          Единственные причины которые знаю это повыпендриватся и/или кому-то падлу сделать. И это ненормально.
          Ответить
          • > А сужать то зачем?
            Ну вот дошло до разраба, что сдуру выставил что-то лишнее в паблик. Человеку свойственно ошибаться. Если это прога, а не либа - вполне можно загрепать вызовы и сузить, пока на эту фичу не сильно завязались.

            > Если другие девелы пользуются
            То поздно пить боржоми, разве что объявить как deprecated ;( О чем я говорил: расширить - да запросто, была бы веская причина, сузить - 7 кругов ада, и 5 лет поддержки.

            В libpng (вроде бы в ней) сколько лет пытались переделать сдуру выставленые напоказ поля структуры на геттеры... Как бы не больше 5 лет... И один хрен народ опомнился только тогда, когда эти поля "внезапно" пропали.
            Ответить
            • > > Если другие девелы пользуются
              > То поздно пить боржоми, разве что объявить как deprecated

              Как у нас на проекте: в одной либе есть метод deprecated который уже как 5й год депрекэйтед. А толку, если другого способа доступится к функциональности так никто и не придумал.

              Или как в одной либе хэш функция для генерации номеров. С самого начала сделали внутренней, что бы типа а не хер. 11 лет спустя, в проекте есть как минимум пять копий оного хэша. Хотел поменять на более эффективный - а мля обломись.

              > В libpng (вроде бы в ней) сколько лет пытались переделать сдуру выставленые напоказ поля структуры на геттеры...

              Это разрабы как-то по GNOME-овски поступли. Чисто технически, такое как раз решается просто: если ломаешь публичный интерфейс, так ломай его полностью, и переводи в новый нэймспэйс. Старый интерфейс просто делается как враппер поверху нового.
              Ответить
              • «если другого способа доступится к функциональности так никто и не придумал»
                Ну, это уже неграмотное перепроектирование. Зочэм тогда его депрецировать, если не знать, чем заменить?
                Ответить
                • "Ну, это уже неграмотное перепроектирование."

                  А причем тут то проектирование? Развешивают public/protected/private на методы и аттрибуты сами девелоперы, авторы кода. И у каждого своя идея как оно должно в идеале работать. И public/protected/private/deprecated более или менее единственное средство как они могут свой идеал до других донести.

                  На ютюбе как-то презентацию видел. Там CTO какой-то мелкой фирмы на тему почему у них в интерфейсах нету public/protected/private ответил "We are all adults here." Что в принципе и отражает мой сентимент. Политические игры с public/protected/private (и в жабе с deprecated) я видел только в ситуациах которые можно легко обобщить термином "детский сад".
                  Ответить
                  • > почему у них в интерфейсах нету public/protected/private ответил "We are all adults here.
                    private это же деталь реализации, она по определению не может быть частью публичного API :)

                    package private в яве и friend в крестах - тоже деталь реализации и очень годная вещь, позволяющая не выставлять лишнего в паблик.

                    Ну а protected... Тут спорная тема. Юзать его чтобы "разрешить вызывать метод только потомкам" или чтобы "дать потомку доступ к полям" - ССЗБ (привет, QThread::sleep()). А вот если юзать его как простенький service provider interface - почему нет? Поясню, что имел в виду:
                    - Сабклассы никогда не вызывают protected методы, а только перекрывают их
                    - Все остальные работают с публичными методами, и даже не думают об этих protected'ах
                    Это дает какую-никакую развязку между теми кто сабклассит и теми кто юзает. И это чуть проще, чем полноценный SPI.

                    Так что они абсолютно правы ;)

                    P.S. Я поди криво выразил свою мысль?
                    Ответить
                    • > - Сабклассы никогда не вызывают protected методы, а только перекрывают их
                      А вот правильный ответ на вопрос "зачем нужны приватные виртуальные функции в C++"
                      Ответить
                      • Жаль, что нет какого-нибудь атрибута override-only.
                        Ответить
                        • Пля. Век живи - век учись, дураком останешься. Попробовал перекрыть приватную виртуальную функцию - работает. Спасибо!

                          Т.е. protected нинужен?

                          P.S. Может кому-то пригодится: http://www.gotw.ca/publications/mill18.htm
                          Ответить
                          • В крестах много чего нинужно.
                            И еще больше того что дублирует другой функционал.
                            Ответить
                            • Хе, затестил на жабе перегрузку приватной функции и постиг сакральный смысл аннотации Override :)
                              Ответить
                          • > работает
                            Ну так это всё в рамках парадигмы: известно, что компилятор сначала выбирает функцию, а только потом проверяет уровни доступа. Поэтому, например, происходит вот такое:
                            http://ideone.com/xuNoqm
                            Ответить
                        • abstract жи. Или ты о чем?
                          Ответить
                    • > Тут спорная тема.

                      Так и я про то же!!!

                      Все к чему public/protected/privatе/deprecated приводят это к лишним спорам.
                      Ответить
                      • Ну public и private то чем виноваты?
                        Ответить
                        • Они не нужны.
                          Нужен гибкий способ подробного описания зоны видимости на запись и зоны видимости на чтение.
                          Ответить
                          • да в жопу это не надо. надо просто нормально документировать. и примеры использования приводить.

                            большая часть всех этих проблем происходит из-за того что народ копи-паздит откуда нипопади кривое использование класса/объекта, а потом менять не хочет потому что уже как бы протестили и зарелизили.
                            Ответить
                            • Мне регулярно надо. Но приходится класть хуй и делать протектед то, что должно быть приватным или делать публичным всё подряд
                              Ответить
                              • friend недостаточно в твоем случае? Или это не внутримодульная связь?
                                Ответить
                                • во. а про френдов я забыл. если убить протектед/приват, то можно еще и бороды классам поотрезать. (борода класса: на одной из фирм идиома для френдов которые всегда в самом низу класса объявлялить.)
                                  Ответить
                        • приват - виноват тем что не нужен.
                          паблик - виноват тем что не дефолт.
                          Ответить
                          • Только PIMPL, только хардкор? Я правильно понимаю идею (ну если мы о крестах)?

                            В сишке то с пабликом и приватом все очень просто.
                            Ответить
                            • > PIMPL

                              ?
                              Ответить
                              • >> PIMPL
                                В ашке пишем только публичный интерфейс, и одно приватное поле с указателем на структуру, в которой хранятся поля.

                                Кутишники юзают эту хреновину для бинарной совместимости, ну и чтобы лишние ашки типа виндовых или линуксовых хидеров не торчали из их API.

                                Большей изоляции паблика и привата я не видел ;(
                                Ответить
                                • > Кутишники юзают эту хреновину для бинарной совместимости

                                  А, да, знаю. Так а другого способа в С++ как бы публичные интерфейсы делать и нету.

                                  Сам то я коммерческий софт пишу. Тут совместимость нужна только на уровне компиляции, поэтому pimpl'ом ни разу в офисе пользоваться не приходилось.
                                  Ответить
                                  • > pimpl'ом ни разу в офисе пользоваться не приходилось
                                    Ну он все-таки для либ, не для прог.
                                    Ответить
                                  • > на уровне компиляции
                                    pimp часто может неплохо увеличить скорость этой самой компиляции.
                                    Ответить
                                  • > Так а другого способа в С++ как бы публичные интерфейсы делать и нету.
                                    Да есть там способ... он даже идеологически правильней (с точки зрения ООП), но далеко не самый приятный, и не самый шустрый (хотя пимпл тоже не айс в плане удобства и скорости).

                                    Интерфейсы с чистыми виртуальными функциями + фабричные методы, возвращающие эти интерфейсы в каком-нибудь смартпоинтере. Оп, оп, жаба стайл.
                                    Ответить
                                    • > Интерфейсы с чистыми виртуальными функциями

                                      Можно.

                                      Но в долгосрочных проектах это просто невозможно поддерживать, потому что добавлять в интерфейс новые (чисто) виртуальные функции нельзя. Приходится делать новый новый интерфейс (часто унаследованые от старого) и менять у пользователей имя интерфейса на новый. Вообщем, работает если только интерфейс стабильный.
                                      Ответить
                                      • > Но в долгосрочных проектах это просто невозможно поддерживать
                                        Скажи это майкрософту с их com ;)

                                        Само собой интерфейсы менять нельзя, только добавлять новые и выпиливать старые, когда все на них забьют (если забьют).

                                        Насчет неудобств - внутри одной подсистемы неудобно, на границе между подсистемами - почему нет?
                                        Ответить
                                        • "Насчет неудобств - внутри одной подсистемы неудобно, на границе между подсистемами - почему нет?"

                                          Бррр... Ты о чем?

                                          На внутренних интерфейсах - это оверкил.

                                          На внешних интерфейсах - морока поддерживать, и долго не выживает.

                                          К слову у нас к паре либ С++-тых основной метод работы через интерфейс 80+ С-style функций. Пару лет по граблям двоичной совместимости походили, и решили откатится на проверенные методы.
                                          Ответить
                                          • > и решили откатится на проверенные методы.
                                            Если не ком и не си стайл, то что? сокеты? общая память? сериализация?
                                            Ответить
                                            • Я же сказал что "интерфейс (из) 80+ С-style функций."
                                              Ответить
                                        • А что неудобного в том, что нужно заводить новые интерфейсы? Qt вывели какой-то свод правил, что можно дополнительно делать с классами, но чтобы продолжалась сохранятся бинарная совместимость. Там вроде не чистые интерфейсы при взаимодействии между модулями.
                                          Ответить
                                          • "А что неудобного в том, что нужно заводить новые интерфейсы?"

                                            В том что старые интерфейсы когда-нибудь надо удалять. В коммерческом софте это почти всегда значит "никогда".

                                            "Qt вывели какой-то свод правил [...]"

                                            Не Qt, а KDE:
                                            http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++
                                            Ответить
                                            • интерфейс из си стайл функций поменять сложнее. нет возможности сохранить старый и новый одновременно.
                                              в любом случае старый дропаешь и приложение больше не запускается.
                                              потом через какое-то время старый интерфейс просто заменяете на заглушки с диалогом до свидания, ссылка на обновление.
                                              Ответить
                                            • В кт вроде тоже было. там у них какой-то особый указатель внутри
                                              Ответить
                    • >чтобы "дать потомку доступ к полям" - ССЗБ (привет, QThread::sleep()).

                      А можно с этого места поподробнее? Я вот наоборот, плохо понимаю зачем прятать что-то от потомков.
                      Иначе потом имеем антипаттерн "паблик морозов".
                      Ответить
                      • > зачем прятать что-то от потомков
                        А зачем прятать что-то от других, если оно доступно потомкам? :)

                        Вот этот QThread::sleep заныкали, когда идеология говорила "засабклассь тред и перекрой метод run()". А потом партия сказала "не надо сабклассить QThread, он не для того", а sleep() так и валялся в протектедах до пятой версии. И чтобы его юзать надо было есть кактус сабклассить тред чисто ради него.

                        А поля и прочие кишки давать потомкам - плохая идея. Потом будешь долго материться и рвать волосы, когда захочешь перепилить базовый класс.
                        Ответить
                        • P.S. QThread::sleep() он относился к первой фразе - "разрешить вызывать метод только потомкам", просто я не туда его прилепил.
                          Ответить
                        • > А потом партия сказала "не надо сабклассить QThread, он не для того", а sleep() так и валялся в протектедах до пятой версии.
                          Ну при архитектурных сдвигах по фазе уже ничто не поможет.
                          Имхо, имеют смысл три степени инкапсуляции.
                          1. Обычное инстанциирование - только паблик, чтобы не смущать пользователя кишками. Если класс не абстрактный, конечно.
                          2. Наследование с ожидаемым перекрытием методов - вот типа того-же Thread::run() во многих языках. Тоже паблик.
                          3. И наследование с серьёзными изменениями, не предусмотренными изначально. Тут protected, чтобы понапрасну обычные инстансы за эти кишки не дергали. Т.е. здесь держать только то, что менять можно, но только на свой страх и риск, и необходимость сабклассить должна служить тому предупреждением.
                          Ответить
                          • > Наследование с ожидаемым перекрытием методов - вот типа того-же Thread::run() во многих языках. Тоже паблик.
                            run() это не часть публичного API треда, это часть, которую тред требует от своих потомков. Нахрена тут паблик? Ты же никогда не будешь вызывать этот run() сам.

                            > наследование с серьёзными изменениями, не предусмотренными изначально
                            Бедные люди, которым потом поддерживать эти "серьезные изменения"...
                            Ответить
                            • >это часть, которую тред требует от своих потомков.
                              Хм. Логично. Хотя мне нравится питоновская реализация, где по умолчанию Thread::run() просто вызывает заранее переданную необязательным параметром конструктора функцию.
                              Т.е. можно просто скормить тело потока реализации по умолчанию, а можно отсабклассить.
                              Впрочем, в этом случае паблик тоже незачем.
                              Убедил.
                              Ответить
                          • > наследование с серьёзными изменениями, не предусмотренными изначально
                            Спорная вещь, кстати. Явно показывающая, что наследник не является предком, а паразитирует на нем.
                            Ответить
                            • Ну... можно, конечно, и так мыслить. "Кто хочет странного - пусть и извращается, а мы это поощрять не будем."
                              Ответить
                              • Вот, кстати, отличный пример такого паразитирования: http://docs.oracle.com/javase/7/docs/api/java/util/Properties.html

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

                                Что получилось (затыкаем дырки в абстракции путем их документирования): 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.
                                Ответить
                                • А что им помешало обернуть put() и putAll() предка?
                                  Типа такого (Питон т.к. Яву не знаю)
                                  class Properties(Hashtable):
                                      def put(self, key, value):
                                          if not (instanceof(key, basestring) and instanceof(value, basestring)):
                                              raise TypeError("Both key and value must be strings.")
                                          return Hashtable.put(self, key, value)
                                          #или так
                                          #return super(Properties, self).put(self, key, value)
                                  Ответить
                                  • > А что им помешало обернуть put() и putAll() предка?
                                    А оно расширяет Hashtable<Object,Object>, а до этого вообще тупо Hashtable. Поэтому эти put() и putAll() будут работать с Object'ами, и обрезать их до String'ов уже не получится. Разве что исключение в рантайме кидать. А компайл-тайм проверку проебали в любом случае.
                                    Ответить
                                    • Блин, точно. Во время компиляции уже фигню не отловишь.
                                      Всё же скриптовые языки деформируют мышление.
                                      Ответить
                                  • Так делать вообще говоря нельзя. Это нарушает принцип подстановки Лисков, т.е. ломает контракт базового класса в наследнике.

                                    В любом случае, наследование ради реализации, а не ради интерфейса, ещё никого до добра не доводило.
                                    Ответить
                                    • Если бытовым языком, "потомок обязан уметь всё то же, что и предок"?
                                      Ответить
                          • > чтобы не смущать пользователя кишками
                            Пойми, ты не пользователя этими кишками пугаешь... Да пользователь будет рад-радёхонек ими воспользоваться при любом удобном случае, если это как-то облегчит ему работу.

                            А вот тебе потом будет плохо, ибо изменить эти кишки ты уже не сможешь, т.к. они стали частью паблик интерфейса и вросли в кучу говнокода, который всем вломы править.
                            Ответить
                            • Вот теперь понял.
                              Тут, конечно, можно отмазаться, мол, гарантируем обратную совместимость только на первых двух уровнях абстракции, если лезешь ниже - ССЗБ.
                              Но скорее всего такую либу пошлют и правильно сделают.
                              Ответить
                    • Это назается non virtual interface. Все реализации виртуальны и protected, перекрываются наследниками. Весь внешний интерфейс класса - невиртуальный. По сути это обобщение понятия свойств в применении к методам. Это правильный подход к проектированию. А вот невиртуальные protected нужны ооооооочень редко.
                      Ответить
              • > А толку, если другого способа доступится к функциональности так никто и не придумал.
                Ну депрекейтед как бы по определению должен предлагать альтернативу (UPD: чуть выше об этом уже написали)... Собственно "старый интерфейс как враппер поверх нового" это тоже своего рода deprecated и not recommended for new projects.

                > 11 лет спустя, в проекте есть как минимум пять копий оного хэша
                Ну дык можно было сделать этому генератору публичный API, когда он понадобился во втором месте... Причем, имхо, даже не выставлять этот генератор в паблик у того модуля, а вынести в отдельный, дабы не создавать ложную зависимость. Тот кто его изначально сделал приватным, имхо, не виноват, и делал все правильно.

                > Старый интерфейс просто делается как враппер поверху нового.
                А тут согласен.
                Ответить
                • > Ну дык можно было сделать этому генератору публичный API

                  Сделать можно все. Но разраб сказал что хэш функцию никому больше не надо знать - и точка.
                  Ответить
    • во первых, писал явно шарпей-джуник. во-вторых, расширять область видимости - явно само по себе говно, которое должно наводить на глубокие раздумия, нахера мы это вообще делаем...
      Ответить
      • «нахера мы это вообще делаем»
        Назначение видимости public вполне соответвует паттерну High Cohesion, когда мы группируем обязанности и распределяем их между несколькими специализированными обработчиками, так что нельзя сказать, что это само по себе говно.
        Ответить
        • но нарушается Закон Деметры, что само по себе говно.
          Ответить
          • Закон Деметры: «Не разговаривай с незнакомцами». Обработчик, выполняющий GetModuleId(userId), вполне знакомец. С ним можно и нужно разговаривать, иначе не узнаешь moduleId.
            Ответить
            • ненене, вы не понимаете. расширение области видимости подразумевает перепроектировку. а тут паблик вообще выставлен на общее обозрение.

              это как если бы ваша жена пошла бы на панель.
              Ответить
    • Сделал бы moduleIdGet виртуальным - был бы норм NVI, а так смысла нет вообще.
      Ответить

    Добавить комментарий