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

    +148

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    6. 6
    public List<OrderEntity> getOrders() {
        if (orders == null) {
            orders = new ArrayList<OrderEntity>();
        }
        return orders;
    }

    Потокобезопасность? Не, не слышал.

    Запостил: roman-kashitsyn, 23 Сентября 2011

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

    • а вы всегда сразу пишете потокобезопасный код?
      Ответить
      • Стараюсь. Вообще стал много думать над тем, что пишу.
        Ответить
        • а я такие вещи оставляю на "второй проход". хотя, с опытом, уже сразу начинаешь замечать наиболее типичные глупости )
          Ответить
          • Тут таких кусков кода штук 200. Второго прохода не будет.
            Ответить
            • судя по активности сайта, хороших программистов не так уж много. действительно хороших, тех, кто еще заботится о потокобезопасности, пока гром не грянет, еще меньше.
              Ответить
              • заботиться о потокобезопасности надо там, где много потоков.
                КЭП
                Ответить
                • в Java ВСЕГДА много потоков
                  КЭП №2
                  Ответить
                  • ArrayList не потокобезапосный OpenJDK говно?
                    Ответить
                    • его можно сделать потокобезопасным, или пользовать Vector
                      Ответить
                      • да.... переписав, например, OpenJDK
                        Ответить
                        • там нет Collections.synchronizedList()?
                          Ответить
                          • synchronizedList сделает объекты класса ArrayList потокобезопасными?
                            Ответить
                            • не все, а лишь оборачиваемый.
                              вы меня троллите?
                              Ответить
                              • да что-то действительно стало походить на троллинг...
                                короче, я это говнокодом совершенно не считаю. Может быть из контекста было бы понятно в чем проблема. (Автор несёт в треде что-то подозрительное)
                                Но если подходить ко всему с подходом не "потокобезопасно", то и i++ надо бы помещать под локи.
                                Ответить
                                • потоконебезопасность метода в том, что два потока, обратившись к нему (на том же объекте) одновременно впервые, рискуют получить по собственному пустому экземпляру, а эффективен будет только один. Самое простое решение проблемы -- сделать метод синхронизированным.

                                  > то и i++ надо бы помещать под локи
                                  не надо, поскольку это локальная переменная, которую два потока совместно использовать никогда не смогут.
                                  Ответить
                                  • >>>это локальная переменная
                                    кто сказал.
                                    \\OLOLOLO
                                    public void getOrders() { i++; }
                                    Ответить
                                    • private int i=0; //ничего страшного
                                      Ответить
                                      • как нет!!! это же суть примера roman-kashitsyn'а
                                        поговнокодю ка я на шарпе
                                        Тут всем понятно, что GetOrder говнокод. Но ничего не понятно из примера roman-kashitsyn'а.
                                        internal class Bar
                                            {
                                                private int i = 0;
                                        
                                                public void GetOrder() { i++; }
                                            }
                                        
                                            class Program
                                            {
                                                static void Main(string[] args)
                                                {
                                                    Bar b = new Bar();
                                                    new Thread(b.GetOrder).Start();
                                                    new Thread(b.GetOrder).Start();
                                                }
                                            }
                                        Ответить
                                        • а, ну в данном случае либо GetOrder нуждается в модификаторе synchronized, либо (если на java) i делать AtomicInteger
                                          Ответить
                                          • may be.
                                            но я бы писал так
                                            internal class Bar
                                                {
                                                    private int i = 0;
                                            
                                                    public void GetOrder() { i++; }
                                                }
                                            
                                                class Program
                                                {
                                                    static void Main(string[] args)
                                                    {
                                                        var b = new Bar();
                                                        Action foo = () =>
                                                                         {
                                                                             lock (b)
                                                                             {
                                                                                 b.GetOrder();
                                                                             }
                                                                         };
                                                        new Thread(new ThreadStart(foo)).Start();
                                                        new Thread(new ThreadStart(foo)).Start();
                                                    }
                                                }
                                            Ответить
                                            • а почему через Action?
                                              Ответить
                                              • Привычка.
                                                foo.BeginInvoke(null, null);
                                                foo.BeginInvoke(null, null);
                                                Можно написать так (типо короче).
                                                Ответить
                              • + дико бесит когда стоит не нужный лок. Он сильно сбивает столку.
                                Ответить
                                • по-хорошему бы надо делать класс потокобезопасным всегда, либо явно в доке (причем javadoc, а не сопроводительная книжечка) указывать: non-threadsafe, losers!
                                  Ответить
                                  • Почитал эту перепалку, что-то я не понимаю позицию "Lure Of Chaos". О производительности по вашему вообще думать не надо? Synchronized гораздо хуже критических секций на C++ и серьезная потеря производительности. Неоправданный synchronized - говнокодерство. В нормальном проекте кол-во классов которые требуют synchronized от силы 1%. Помечать 99% классов в Javadoc как non-threadsafe безумие, проще априори считать, что все классы не рассчитаны на threadsafe. А если рассчитаны, то либо давать говорящие имена, либо гордо писать в Javadoc threadsafe. Типичный говнокод это использовать StringBuffer, там где можно использовать StringBuilder.
                                    Ответить
                                    • synchronized надо тоже с умом использовать, или вообще не использовать, дан целый пакет java.util.concurrent.
                                      > проще априори считать, что все классы не рассчитаны на threadsafe
                                      как правило, большинство кодеров вряд ли рассматривает этот вопрос, а проблемы начнутся, есть небезопасный использовать, полагая, что он безопасен - нежели наоборот...
                                      Ответить
                                      • >как правило, большинство кодеров вряд ли рассматривает этот вопрос

                                        Говнокодеров, которые даже не знали, что их взяли в проект с многопоточностью.
                                        Ответить
                      • ArrayList появился только в 1.2, в то время, как уже был всегда Vector. Зачем?
                        Ответить
                        • чтобы иметь и медленную потокобезопасную, и быструю потоконебезопасную версию массива
                          Ответить
                    • потокобезопасность ArrayList тут совершенно не причём. Если два потока независимо вызовут getOrders(), они могут получить два совершенно разных списка. А полем класса будет только последний из них. Первый накидает элементов в список, который потом аккуратно удалит сборщик мусора.
                      Ответить
                      • >>>ArrayList тут совершенно не причём
                        ArrayList тут как пример, что не потокобезопасные совершенно не говно.
                        Ответить
    • orders static ?
      Ответить
      • нет, обычное поле.
        Ответить
        • Тогда интересно, что мешало в конструкторе проинициализировать?
          Ответить
          • Вот и мне очень интересно. Видимо, экономят память.
            Ответить
          • Элементарный пример - это класс модели, и значения полей заполняются Hibernate при access="field", т.е. Hibernate не вызывает setOrders(...), а прямо использует поле orders через reflection. Тогда, если нет ни одного ордера, значение поля orders будет null. Вполне нормальный подход, имхо.
            Ответить
            • access=field уже говнокод :)
              Ответить
              • Ээээ... почему?
                Ответить
                • Ну может и не сильно говнокод, но все равно сам Hibernate рекомендует accessoры, а что не соблюдать рекомандации нужны веские аргументы против них.

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

                    На самом деле я всегда использую access=field, чтобы иметь возможность создавать immutable объекты.
                    Ответить
                  • просто гибер хреново работает с коллекциями при field access'е. Поэтому используя гибер, к коллекциям даже внутри класса необходимо обращаться только через геттер.
                    Ответить
    • А сам класс, где это встречается - он static или используется в разных потоках?
      Ответить
      • Веб-приложение, а это класс модели. Соответственно, обращение из нескольких потоков более чем возможно.
        Ответить
        • Именно потому, что это класс модели в веб-приложении, обращение из нескольких потоков невозможно, точнее, разработчику пришлось бы специально задаться целью разрешить обращение из нескольких потоков.
          Ответить
        • А что понимается под моделью? Например MVC не подразумевает обращение к модели из нескольких потоков, это просто объект с данными, который надо отрендерить. Область его жизни равна области жизни запроса.

          Если это некий статический объект, содержащий всю конфигурацию и данные приложения, то что-то мне подсказывает, что там есть архитектурный говнокод. Если нужен кеш, то во всяких там Hibernate есть кеш. Есть кеши на уровне Application сервера. Причем все эти решения поддерживают репликацию кеша между несколькими нодами/системами. И никаких рисков внести потоко-небезопасный код нет.

          Юзайте Spring MVC, он избавит вас от неверных архитектурных решений :)
          Ответить
          • +100500
            Ответить
          • Я бы и рад использовать любимый Spring MVC, но архитектуру придумали за меня. Тут JSF + собственная система компонентов, куча данных хранится в сессии. Переписывать работающее приложение, исходники которого занимают 400Mb, никто не будет.
            Ответить
        • Зачем потокобезопасноcть, если scope = request
          Ответить
    • Да, наверное, погорячился. Посыпаю голову пеплом. Думал, что один экземпляр модели может использоваться в нескольких потоках. Всем спасибо за сеанс просветления.
      Ответить
    • кстати и для однопоточного кода - нормально написано.
      сталкивался на практике с ситуацией, когда таких вот коллекций в объекте было штук 5 а то и больше, а объектов в памяти было 500к+. оверхед по памяти нехеровый выходит, если сразу инициализировать.
      Ответить
    • а как правильно этот код написать для работы в многопоточном окружении, при условии, что коллекцию инициализировать в конструкторе нельзя?
      Ответить
      • Не претендую на правильность, но первое, что приходит в голову - использовать synchronized-блок:
        public List<OrderEntity> getOrders() {
            synchronized(this) { // можно ещё использовать какой-нибудь внутренний объект - лок
                if (orders == null) { orders = new ArrayList<OrderEntity>(); }
            }
            return orders;
        }
        . Можно ещё попробовать прикрутить AtomicReference<List<OrderEntity>>, но тут возникают некоторые трудности, методов борьбы с которыми я не знаю.
        Ответить
        • можно ещё замутить хитрость, чтобы вход в synchronized блок осуществлялся только если orders==null:
          public List<OrderEntity> getOrders() {
              if (orders == null) {
                  synchronized(this) {
                      if (orders == null) { orders = new ArrayList<OrderEntity>(); }
                  }
              }
              return orders;
          }
          Ответить
          • if (orders == null) {
                    synchronized(this) {
                        if (orders == null) ...
            так ведь можно и зациклиться.
            Ответить
          • orders должен быть volatile. DCLP работает только с жавы 1.5. В более ранних версиях нет гарантии что это сработает из-за бага в JVM (Sun'овская)
            Ответить
            • Да, об этом написано у Блоха. у него же описан более хитрый приём, чтобы можно было не писать volatile.
              Ответить
              • Если правильно помню, у него на примере singleton'ов, а это немного другое.
                Ответить
                • А какая разница в организации доступа к лениво-инициализируещемуся полю сингтона и обычного объекта?
                  Ответить
                  • class Singleton {
                       private static class Holder {
                           private static final Singleton INSTANCE = new Singleton();
                       }
                    
                       public Singleton getInstance() {
                          return Holder.INSTANCE;
                       }
                    }

                    В этих паттернах очень много построено на том, что поле является static.

                    Как подобный паттерн можно применить в случае обычного объекта?
                    Ответить
                    • Нет, у него там есть double-check idiom:
                      // Double-check idiom for lazy initialization of instance fields.
                      private volatile FieldType field;
                      FieldType getField() {
                          FieldType result = field;
                          if (result == null) { // First check (no locking)
                              synchronized(this) {
                                  result = field;
                                  if (result == null) // Second check (with locking)
                                      field = result = computeFieldValue();
                              }
                          }
                           return result;
                      }
                      Ответить

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