1. Java / Говнокод #12690

    +114

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    6. 6
    7. 7
    8. 8
    9. 9
    private List<PalletePath> killDupes(List<PalletePath> pathesNew) {
    	List<PalletePath> noDupes = new ArrayList<PalletePath>();
    	for (PalletePath tp : pathesNew) {
    		if (!noDupes.contains(tp)) {
    			noDupes.add(tp);
    		}
    	}
    	return noDupes;
    }

    Set? Не, не слышали.

    Запостил: someone, 05 Марта 2013

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

    • может упорядоченность важна?
      Ответить
      • LinkedHashSet же.
        Ответить
        • тут мало контекста. в каком-то случае это прокатит, в каком-то нет.
          Ответить
          • return new ArrayList<PalletePath>(new LinkedHashSet<PalletePath>(pathesNew));


            по семантике полностью аналогично приведённой писанине, при этом не требуя квадратичного времени.
            Ответить
            • а-а-а, если в этом смысле - то можно и через Set.

              А этот метод настолько часто вызывается и оперирует такими объемами данных, что квадратичное время становится критичным?
              Ответить
              • А даже если нет, почему бы не сделать правильно?
                Ответить
                • а почему это правильно? и что есть правильно? почему предложенное решение неправильно?
                  Ответить
                  • Или вы - тролль, или вам пора узнать про http://en.wikipedia.org/wiki/Big_O_notation
                    Ответить
                    • это я к тому, что если этот метод вызывается пару раз на массивах пусть даже с 100 элементов - это решение нельзя считать неправильным. все всегда зависит от контектса, в котором код работает, и о правильности можно говорить только в контексте решаемой задачи.

                      Простой пример - singleton. Правильная реализация - DCLP (java 5) или instance holder. Но и
                      public class Singleton {
                          private static  Singleton instance;
                          ....
                          public static Singleton getInstance() {
                              if (instance == null) { instance = new Singleton(); }
                              return instance;
                          }
                      }


                      также является правильной реализацией в однопоточном приложении.

                      все решает контекст
                      Ответить
                      • Я уже забыл, когда последний раз видел однопоточное жабо-приложение.
                        Ответить
                        • видать много серверного кода писать приходится :)
                          Ответить
                          • Любое нетривиальное современное GUI (и даже консольное) приложение значительно выигрывает от работы в несколько потоков, часто многопоточность в них просто необходима.
                            Ответить
                      • Как по мне всегда выигрывает более общее решение, ибо костыль влепить всегда успеешь
                        Ответить
                      • В один прекрасный день разработчик понимает, что общение с каким-нибудь сервером или вычисления занимают несколько секунд, что довольно неприятно для юзера, и он добавляет тред/пул/whatever и... в лучшем случае матерясь идет перепиливать все эти недосинглтоны на нормальные, в худшем - по незнанию/невнимательности оставляет все как есть.

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

                        > Правильная реализация - DCLP (java 5)
                        DCLP это срань, которая работает по счастливой случайности даже с новой моделью памяти java 5. От авторов спеки: There exist a number of common but dubious coding idioms, such as the double-checked locking idiom, that are proposed to allow threads to communicate without synchronization. Almost all such idioms are invalid under the existing semantics, and are expected to remain invalid under the proposed semantics.

                        > видать много серверного кода писать приходится :)
                        Любая прога, использующая swing, не однопоточна - там минимум 2 треда - стартовый и EDT. На том же андроиде в главном треде невозможно работать с сетью (кидает NetworkInMainThreadException). Так что даже совсем не серверные проги бывают многопоточными...
                        Ответить
                        • давайте тогда сразу синхронизированные коллекции везде фигачить :) или вы так и делаете? :)

                          Код - это декларация намерений. Я всегда читаю чужой код с мыслью, что его писал умный человек. Т. о. любая строчка была осмысленной и обоснованной в тот момент когда она писалась. И если, возвращаясь, к singleton'у, я вижу реализацию не потокобезопасную, то я делаю вывод, что эта часть кода не многопоточна, но ни в коей мере не делаю вывод, что автор идиот. Ибо если тут написано для одного потока, то помимо этого несчастного singleton'а наверняка есть еще много кода, который также однопоточен. Непотокобезопасный singleton - это один способов из декларации того, что автор не предполагал использование кода в многопоточном окружении.

                          Возвращаемся к исходному коду.
                          Если бы такой код был у кого-нибудь из моих коллег я бы сделал следующие выводы:
                          1. Быстродействие здесь не критично (маленькие объемы + редко вызывается)
                          2. Коллега запамятовал про LinkedHashSet
                          т. к. я на 100% уверен, что если быстродействие было бы критичным (но про LinkedHashSet коллега не вспомнил) то код выглядел бы как-нибудь так
                          private List<PalletePath> killDupes(List<PalletePath> pathesNew) {
                              List<PalletePath> noDupes = new ArrayList<PalletePath>();
                              Set<PalettePath> processed = new HashSet<PalettePath>();
                              for (PalletePath tp : pathesNew) {
                                  if (processed.add(tp)) {
                                      noDupes.add(tp);
                                  }
                              }
                              return noDupes;
                          }


                          Поэтому я не сторонник говорить правильный код или нет не имея контекста.

                          ПС. Попытка писать сразу универсальный код - это одна из главных проблем умных программистов. Желание писать сразу все универсально приносит больше проблем, чем профита.
                          Ответить
                          • > давайте тогда сразу синхронизированные коллекции везде фигачить :)
                            Это совсем другое дело

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

                            > Быстродействие здесь не критично
                            Нормальное быстрое решение реализуется в три строчки, плохое и потенциально медленное - в девять. Не люблю я лишний код, не люблю.

                            С резюме согласен, к сожалению, желание предусмотреть все варианты и "писать с рассчётом на будущее" далеко не всегда себя оправдывает, редко можно угадать, что именно потребуется.
                            Ответить
                            • 1. А почему это другое дело? Почему в одно месте надо писать потокобезобасно, а в других не надо?

                              Вроде бы есть простое объяснение: где можно малыми силами сделать "правильно" делаем сразу правильно, а где это требует телодвижений и увеличения кода (неоправданно) - там не делаем. Основной посыл получается такой: когда будем доделывать меньше придется переделывать.

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

                              Логично написанный код может рассказать порой намного более javadoc'ов и комментариев

                              2. Смотреть приходится при командной разработке проекта, без этого уж точно никак
                              Ответить
                            • > желание предусмотреть все варианты и "писать с рассчётом на будущее" далеко не всегда себя оправдывает, редко можно угадать, что именно потребуется.

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

                              вывод: нет смысла писать универсальное решение, оно вряд ли себя оправдает. есть смысл писать решение, которое также легко пишется, как и читается, без особых премудростей.
                              Ответить
                          • > давайте тогда сразу синхронизированные коллекции везде фигачить :)
                            Зачем же все? ;) Тем, которые гарантированно крутятся в своем потоке и не выходят за его пределы, синхронизация нахер не сдалась. А вот публичный синглтон, как и любую другую глобально-публичную переменную лучше лишний раз синхронизнуть чем потом об этом жалеть.

                            > Непотокобезопасный singleton - это один способов из декларации того, что автор не предполагал использование кода в многопоточном окружении.
                            Написал публичный однопоточный синглтон - опиши в комментарии/документации к нему какие требования и мысли привели к такому поступку, поясни читателю в каких именно случаях его использование безопасно. Например "Этот синглтон в свое время являлся bottleneck'ом при отрисовке сцены, поэтому было принято решение сделать убрать синхронизацию и использовать его только из event dispatch thread.".

                            > Желание писать сразу все универсально приносит больше проблем, чем профита.
                            Всё - да. Места, в которых потом явно вылезут гейзенбаги, костыли и прочее говно - нет.
                            Ответить
                            • 1. ну ладно, давайте только те коллекции для которых есть публичный геттер :)
                              2. javadocи быстро устаревают
                              3. далеко не всегда можно знать где вылезут костыли
                              Ответить
                              • > javadocи быстро устаревают
                                По чьей вине? ;) Или они сами умирают от старости?

                                > далеко не всегда можно знать где вылезут костыли
                                Полностью согласен, но однопоточный синглтон без веской причины - отличный зародыш гейзенбага.
                                Ответить
                                • 1. Если у вас всегда есть время на правку документации - искренне рад за вас. Даже в публичных фреймворках частенько на методах висят старые джавадоки.
                                  2. Однопоточный синглтон - это такой большой красный флаг "Эй! Будь аккуратнее! Тут все надо проверить!". Согласитесь: если человек точно занет, что в будущем будем многопоточность так код не напишет. Значит он был уверен на 100% и даже в страшных снах не видел возможности многопоточного использования. С другой стороны, если он знал, что многопоточность будет внедряться, то и весь код будет написан таким образом, чтобы внедрение заняло минимум времени и было предельно простым.
                                  Ответить
                                  • Даже завидую вам немного, раз код вам всегда достается от адекватных программистов...

                                    А если он ни во сне ни на яву не видел потоков, и просто сляпал синглтон из того что знал?

                                    > Однопоточный синглтон - это такой большой красный флаг "Эй! Будь аккуратнее! Тут все надо проверить!"
                                    Это если есть исходники. А если либа досталась в бинарном виде? Декомпилировать и изучать? Или предполагать худшее и добавлять к этому неведомому синглтону свою синхронизацию?
                                    Ответить
                                    • Да, мне повезло, что после института я попал сразу в компанию, где у всех программистов был высокий уровень.

                                      Если код изначально был многопоточным, а программист сляпал такое - это уже другой разговор. Это действительно бомба.
                                      Ответить
                                    • Рассуждения выше относятся только к собственным разработкам.

                                      Если вы выпускаете библиотеку и отдаете ее людям - документация must have.
                                      Ответить
                                      • >>Если вы выпускаете библиотеку и отдаете ее людям - документация must have.

                                        Так бывает далеко не всегда.
                                        Ответить
                              • >2. javadocи быстро устаревают
                                У говнкокодеров - да. Поэтому лучше всего делать, как опенсорсники, т.е. не писать доки вообще.
                                Ответить
                        • к термину "однопоточный" не стоит относиться буквально.

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

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

                              Соответственно человек, пишущий свинговые приложения - гуру многопоточности. И в резюме он может смело писать - знаю многопоточность как свои 5 пальцев. Это как у уборщика в Шереметьево спрашивают: "Почему вы не смените работу?", а в ответ: "Да чтобы я бросил авиацию?! НИКОГДА!!!"

                              П. С. Не стоит воспринимать слова лишь с формальной точки зрения. Иначе любой диалог будет сводиться к абсурду.
                              Ответить
                              • А теперь объясните ARM'овскому процессору что он должен читать из переменной правильное значение, записанное туда другим потоком, если эти два действия не были связаны отношением happens before (к примеру освобождением-захватом лока или записью-чтением volatile переменной). А я посмотрю со стороны, посмеюсь, как вы будете это ему пояснять с неформальной точки зрения.

                                Если 2 потока используют общую переменную - это многопоточность. Хоть с формальной хоть с неформальной точки зрения. И если библиотека делает всю синхронизацию за вас (как например это делает андроидский AsyncTask), то это не повод называть такое приложение однопоточным.
                                Ответить
                                • наверное в этом треде многопоточность надо понимать в контексте целостности разделяемых данных.
                                  есть же случаи, когда двум потокам необязательно иметь актуальные данные.
                                  Ответить
                                  • В примере, который приводил tir, вполне так разделяемые данные - те которые передаются worker'у и те которые уходят от него обратно. Да, в случае с AsyncTask либа разруливает всю синхронизацию за нас, и если никуда больше не лезть из воркера - все будет ок, и мы даже не почувствуем, что прога была многопоточной... Но стоит воркеру полезть к глобальным переменным...

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

                                      У меня было все проще. Приложение посылает запрос на сервер "дай мне последние новости начиная с 7 марта" в ответ получает, например, json. И после этого в UI потоке (ибо так проще) конвертит этот json в объекты и обновляет интерфейс.

                                      То что здесь есть потоки - бесспорно. Но это их тривиальнейшее использование у меня язык не поворачивается назвать многоточностью. Хотя по всем законам это многопоточность.

                                      Вот если вы параллелите какие-либо вычисления, если у вас висят фоновые задачи, которые меняют данные и интерфейс вашего приложения в режиме реального времени их отображает - вот это я подразумеваю, когда говорю о многопоточности в этой ветке.
                                      Ответить
                                      • > Приложение посылает запрос на сервер "дай мне последние новости начиная с 7 марта" в ответ получает, например, json. И после этого в UI потоке (ибо так проще) конвертит этот json в объекты и обновляет интерфейс.
                                        Здесь мы видим 2 межпоточных передачи данных - на первой парамеры запроса ушли в worker, а на второй полученный json передали в UI поток, в котором его будут парсить и показывать. Да, не спорю, удобная либа скрыла от нас эти детали, и мы о них лишний раз не задумываемся...

                                        Но забывать о том, что это именно многопоточный сценарий все же не стоит. Здесь есть как минимум 2 разделяемых объекта - те самые запрос и ответ. А если воркер лезет к какому-нибудь не локальному объекту, например за куками, то добавятся и еще... И здесь шанс наступить на грабли, если забыть о многопоточности, ничуть не меньше, чем при параллельных вычислениях ;)

                                        Скажите честно, если бы вы поручили новичку реализовать данный сценарий, то сказали бы ему "не бойся, здесь нет никакой многопоточности", или все-таки перечислилили бы ему элементарные правила, о том что нельзя делать в таких ситуациях?
                                        Ответить
                                        • request - String, response - String.

                                          Задача воркера послать запрос передав String, подождать ответ и в чистом виде передать вернувшийся String в колбэк. Никуда вовне worker лазить не будет, никакие объекты сам обновлять не будет. Он только ждет.

                                          Мой сценарий по силам новичку.
                                          Ответить
                                          • > String
                                            Да, со стрингом, как и с любым другим иммутабельным объектом очень трудно что-нибудь забаговать, тут мне нечего возразить.

                                            > Мой сценарий по силам новичку.
                                            Который имеет базовые представления о многопоточности. Те самые "никуда вовне worker лазить не будет", "никакие объекты сам обновлять не будет" вы же не от балды придумали, а составили на основе своих знаний о потоках ;)
                                            Ответить
                                            • > а составили на основе своих знаний о потоках
                                              Сетевое взаимодействие часто прячется за фреймворком, который дает UI callback. Т. е. вроде как все работает из коробки, пусть и не оптимально. И многим этого хватает. Тут о многопоточности в моем понимании говорить не приходится.

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

                                              ПС. Кстати, если в моем конкретном примере кто-то из воркера решит куда-то слазить - я решу, что у него не все впорядке с головой :)
                                              Ответить
            • А если в PalletePath переопределен equals, но не hashCode (вполне вероятно, если судить по названию класса - pallete vs. palette), и код PalletePath недоступен?
              Ответить
    • http://govnokod.ru/3921
      точно так же когда-то написал.
      Ответить
      • Я тоже такое писал, когда только начинал работать программистом. Думаю, многие через это проходили.
        Ответить
    • O(n**2), как классно.
      Ответить

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