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

    +70

    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
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    for (AllResponseCache actionResponse : allResponses) {
                if (null != actionResponse.getResponseStatus() && actionResponse.getResponseStatus().length() > 0) {
                    for (ResponseSubjectCache subj : actionResponse.getSubjects()) {
                        // find needed element
                        if (subj.getClaims() != null) {
                            for (ClaimCache claimCache : subj.getClaims()) {
                                Seller seller = getSellerByPersonMatched(pool, claimCache);
                                if (seller != null) {
                                    if (mapToSyncronize.get(seller) == null) {
                                        mapToSyncronize.put((SellerrEntity) seller, new LinkedList<ReportResponseCache>());
                                    }
                                    mapToSyncronize.get(seller).add(actionResponse);
                                }
                            }
                        }
                    }
                }
            }

    Индусы и "for-if"-ы.

    Я уж думал будет хронология как в России с "президентами" - "лысый, волосатый, лысый, волосатый" и так далее.
    А тут "for, if, for, if" но в конце всё-таки 2 иф-а!

    Запостил: Dimedrol, 25 Октября 2011

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

    • ava gegMOPO4 25.10.2011 16:32 # 0

      Ну и что?
      Ответить

    • ava roman-kashitsyn 25.10.2011 16:54 # +2

      Обычный тоскливый Java-код. Что тут такого?
      разве что в функциональном языке это, вероятно, делается в две-три строчки
      Ответить

      • ava Lure Of Chaos 25.10.2011 17:20 # +2

        у меня смутное подозрение, что даже на Java можно эту операцию написать гораздо красивее.
        только я не вьеду до конца, что оно тут делает.
        Ответить

        • ava roman-kashitsyn 25.10.2011 17:25 # 0

          Проходит по отчётам с жалобами, извлекает все жалобы, создаёт запись в мапе Продавец => Список Отчётов и заполняет этот список.
          Ответить

          • ava Lure Of Chaos 25.10.2011 17:30 # 0

            выглядит как ORM. Думаю, что через грамотный запрос можно добиться того же эффекта без циклоциклов
            Ответить

      • ava 3.14159265 03.11.2011 21:44 # 0

        Может тут имело смысл породить методы для борьбы с null
        static List<T> noNull(List<T> l){
        return (l==null ? Collections.emptyList() : l);
        }
        static List<T> noNull(Map<T> m){
        return (l==null ? Collections.emptyMap() : m);
        }
        итд...
        Ответить

      • ava 3.14159265 03.11.2011 21:50 # −1

        + написать или использовать в готовой либе метод который реализует логику.
        static <K,V>void safePut(Map<K,V> m,  K key, V value)
{
if (key != null) 
if (map.get(key) == null) //хотя прозреваю что подразумевался contains
map.put(key,value);
}

        Ибо без сомнений такое и в других местах по проекту встречается.

        В идеале такой вот код 
        Seller seller = getSellerByPersonMatched(pool, claimCache);
if (seller != null) {
    if (mapToSyncronize.get(seller) == null) {
        mapToSyncronize.put((SellerrEntity) seller, new LinkedList<ReportResponseCache>());
    }
    mapToSyncronize.get(seller).add(actionResponse);
}

        Выглядел бы так
        MapUtils.safePut(mapToSyncronize,
      getSellerByPersonMatched(pool,claimCache), Arrays.asList(actionResponse)
)
        Ответить

        • ava gegMOPO4 25.02.2012 17:55 # 0

          Нет, этот код делает совсем не то. Нужен где-то такой safeGet:
          static V safeGet(Map<K,V> map,  K key, Class<V> aClass)
{
    V value = map.get(key);
    if (value == null)
    {
        value = aClass.newInstance();
        map.put(key,value);
    }
    return value;
}

          Использование:
          safeGet(mapToSyncronize, seller, LinkedList<ReportResponseCache>.class).add(actionResponse);


          Используемая здесь идиома так распространена, что в Питоне для этого есть специальный тип — defaultdict.

          if (seller != null) здесь вообще не при делах, это особенность конкретного кода.
          Ответить

    • ava absolut 25.10.2011 17:01 # +1

      Вложенные объекты через вложенные циклы с проверкой необходимых условий. Вердикт : не говнокод!
      Ответить

    • ava guest 25.10.2011 17:08 # +2

      Это - говнокод, потому что:
      1. Это невозможно читать (человеку).
      2. Это невозможно тестировать.
      Ответить

    • ava odmin 25.10.2011 17:55 # 0

      А как надо-то?)
      Ответить

    • ava guest 25.10.2011 19:29 # 0

      Многие if (condition) { ... } внутри циклов можно заменить на if (!condition) continue;
      Должно лучше читаться
      Ответить

    • ava Dimedrol 25.10.2011 22:34 # 0

      (это я сам был 1-м quest-ом.)

      Я хотел сказать именно то, что высказал "Irdis".
      Эту кашу нужно отрефачить на несколько методов у которых не будет вложенных циклов и кучи if-ов.

      Почему никто их комментаторов не говорит о тестировании?!
      Вы попробуйте unit test к этому уЖОсу написать!

      Если человек знает что такое unit-тестирование и сам пишет тесты на свой код, - он не может написать вышепоказанный код по определению!
      Ответить

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