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

    +74

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    6. 6
    protected void setPhones(ArrayList<MBPhone> phones) {
    if(phones!=null) 
        this.phones=phones;
    else 
        phones.clear();
}

    Код из одного западного вэб-сервиса для профессионалов в сфере недвижимости.
    Мораль: не надо игнорировать подсказки IDE. Автор на самом деле хотел очистить this.phones. И Intelij Idea подсказывает, что в этом месте может быть брошен NullPointerException. Увидеть эту проблему можно было только используя аннотацию @SuppressWarnings. Потому что варнингов так много, что различить среди них опасные очень сложно.

    Запостил: vladimir.loshchin, 10 Июня 2010

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

    • ava Анонимус 11.06.2010 00:05 # +1

      Вообще аргумент типа ArrayList в сигнатуре функции уже говорит о многом
      Ответить

    • ava morbid 11.06.2010 06:29 # 0

      Ну вообще то маразм совсем не в этом, а в том что используется не копирование элементов массива, а присвоение указателей. И даже если бы код был написан с this, то бага стала бы еще более неочевидной.
      Ответить

      • ava vladimir.loshchin 11.06.2010 06:33 # 0

        Что плохого в присвоении?
        А если бы вместе phones был List<Customer>, где Customer является persistence.entity имеющим связи с ещё пол сотней entity? Всё это дерево тоже нужно копировать?
        Ответить

        • ava morbid 11.06.2010 07:00 # 0

          тебе не надо делать deep-copy, просто делается копирование уровня addAll(); и после этого можно дальше использовать List<Customer> как угодно в вызвавшем методе.
          Ответить

    • ava Webkill 17.06.2010 04:12 # +2

      Ничё не напоминает? http://govnokod.ru/3070

      Говноплагиат!
      Ответить

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