Практика рефакторинга. Завистливые функции



Однажды наша команда обнаружила, что сильно проседает производительность системы при выполнении довольно простого SQL запроса:

select count(*) n from products where category_id = ?


Разумеется, встал вопрос, как его оптимизировать.

Подкованный читатель, возможно, сразу подумает об индексах, хинтах и прочих фичах СУБД. Но сегодня рассказ будет не о них. Да и вообще, не затронет тему оптимизации SQL запросов.

Сегодня речь пойдёт об очень простом методе рефакторинга, который в данном конкретном случае позволил значительно увеличить производительность системы.



Этот запрос находился в том старом коде, в который уже пару лет никто не лазил, в классе SQLConsts среди прочих других SQL запросов:

public class SQLConsts {
    public static final String PRODUCTS_SQL = "select count(*) n from products where category_id = ?";
    ...


А использовался он в другом классе – CategoryRepository:

public class CategoryRepository {
    ...
    private boolean isCategoryVisible(int categoryID)  {
        ResultSet resultSet = executeQuery(SQLConsts.PRODUCTS_SQL, categoryID);
        int n = resultSet.getIntegerFieldByName("n");
        return n > 0;
    }
    ...


Даже не очень опытный программист наверняка заметит, что излишне вычислять в запросе количество строк, если потом это количество просто сравнивается с нулём.

Как же появился этот очевидный эпикфейл? Анализ Git-логов показал, что изначально в методе isCategoryVisible была более сложная логика, которая использовала количество строк в своих вычислениях. Но потом от сложной логики отказались, и осталось только n > 0. Видимо, у того программиста, который делал эти изменения, просто не возник вопрос, что же такое n, и он не стал смотреть сам SQL запрос, тем более, что тот находился совсем в другом файле.

Теперь, когда эти два куска кода находятся рядом, оптимизация становится очевидной. В итоге метод isCategoryVisible был переписан: select count(*) заменили на конструкцию с where exists, что дало ощутимый прирост производительности на больших объёмах данных; а от класса SQLConsts впоследствии избавились.

public class CategoryRepository {
    ...
    private boolean isCategoryVisible(int categoryID)  {
        ResultSet resultSet = executeQuery(
            "select null from dual where exists (select null from products where category_id = ?)",
            categoryID
        );
        return !resultSet.isEmpty();
    }
    ...


Отсюда правило: «то, что изменяется одновременно, надо хранить в одном месте. Данные и функции, использующие эти данные, обычно изменяются вместе», – писал Мартин Фаулер в своей книге «Рефакторинг. Улучшение существующего кода» более десяти лет назад.

В нашем случае данные (SQL запрос) хранились в одном классе – SQLConsts, а функция isCategoryVisible, которая использовала эти данные – в другом: в CategoryRepository. Такие функции Фаулер называет завистливыми, потому что они больше интересуются не тем классом, в котором находятся, а каким-то другим. И чаще всего предметом зависти являются данные, как и в нашем случае: isCategoryVisible как бы завидует, что другой класс SQLConsts хранит SQL запрос, который этому классу, по сути, не нужен, но который необходим методу isCategoryVisible.

Еще раз: то, что изменяется одновременно, надо хранить в одном месте – повторяйте это как мантру, пока это правило не войдёт у вас в привычку. Когда вы уже перестанете о нём думать и будете следовать ему на подсознательном уровне, вы сами не заметите, как ваш код станет чище.

Функциональная зависть


Следует отметить, что в статье приведен неклассический пример завистливой функции. В оригинале завистливые функции называются «feature envy», что в дословном переводе означает «функциональная зависть». Т. о., полагаю, функциональная зависть по Фаулеру не ограничивается одними лишь функциями/методами и может также распространятся на целые классы, хотя сам Фаулер приводит в качестве примера только зависть методов.

Поэтому, возможно, следовало бы говорить уже о целом завистливом классе CategoryRepository, т. к. в реальности все методы этого класса использовали данные из SQLConsts, причем сам SQLConsts этими данными не пользовался.

Более подробно моё видение этого вопроса озвучено тут:
habrahabr.ru/post/220883/#comment_7547819

P. S.


А что, если бы переменная n называлась, к примеру, productCount, а константа PRODUCTS_SQLPRODUCT_COUNT_IN_CATEGORY? Тогда productCount > 0 подтолкнуло бы разработчика задуматься, нужно ли ему вычислять в запросе количество.

Поэтому второе правило: давайте понятные имена переменным, константам, методам и классам. Возможно, это правило даже важнее правила первого.

UPDATE


Небольшой ликбез для тех, кому не нравится exists.

Оператор exists вернет true, если хотя бы одна запись из подзапроса удовлетворяет условию category_id =?
Таким образом, СУБД не будет выбирать все строки из подзапроса: достаточно найти первую запись, удовлетворяющую условию.

Поэтому, эти два варианта будут одинаково эффективны:

select null from dual where exists (select null from products where category_id = ?)


select null from products where category_id = ? and rownum = 1


* rownum = 1 в Oracle тоже самое, что и limit 1 в MySQL.

А вот where NOT exists действительно приводит к перебору всех подходящих записей. Но следует заметить, что в этом случае уже не получится использовать rownum = 1.

UPDATE 2


Для тех, кто интересуется, использовался ли индекс для колонки category_id или нет, сообщаю, что ДА, использовался.

С точки зрения SQL оптимизации с исходным запросом уже ничего нельзя было сделать, не изменив логики его работы.
Поделиться публикацией

Похожие публикации

AdBlock похитил этот баннер, но баннеры не зубы — отрастут

Подробнее
Реклама

Комментарии 22

    +2
    А не слишком ли не эффективно выбирать все продукты по категории? Не будет ли быстрее в вашем случае что-то вроде
    «select 1 from products where category_id =? limit 1»
    ?
      0
      Оператор exists вернет true, если хотя бы одна запись из подзапроса удовлетворяет условию category_id =?
      Таким образом, СУБД не будет выбирать все строки из подзапроса: достаточно найти первую запись, удовлетворяющую условию.

      Поэтому, эти два варианта будут одинаково эффективны:

       select null from dual where exists (select null from products where category_id = ?)
      


      select null from products where category_id = ? and rownum = 1
      


      Вот тут есть подробности:
      stackoverflow.com/questions/19125712/oracle-exists-clause-vs-rownum-1

      P.S. используем rownum, т. к. запросы для Oracle.
        0
        Про Oracle ничего не скажу, но сталкивался с описаниями под mysql, где where [not ]exists приводило к перебору всех подходящих записей. Этакий WHERE (SELECT COUNT(*) ,...) > 0 или WHERE (SELECT) IS NOT NULL. Сам, правда, не пользовался.

        Смутило, зачем для простой проверки наличия хотя бы одной строки, усложнять запрос.
          0
          where NOT exists действительно приводит к перебору всех подходящих записей, но не where exists.

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

          Полагал, что не для всех будет понятно, что именно делает rownum = 1, поэтому использовал классический exists.
      0
      ДА!
        +3
        Начало недели очевидностей на хабре?
          –3
          Хорошо, что вам это кажется очевидным. Если бы все следовали этим правилам на интуитивном уровне, подобные ситуации не возникали бы, и эта статья не появилась бы на свет.
            0
            Я думаю, тут это большинству покажется очевидным.
            Суть всей вашей статьи в банальном «не плоди сущностей сверх необходимого»
            А про Фаулера вы тут немного не в тему написали.
              0
              Жаль, что не смог донести до вас суть статьи: то, что изменяется одновременно, надо хранить в одном месте.

              А про Фаулера вы тут немного не в тему написали


              Можете пояснить свою мысль? Т. к. статья про завистливые функции, которые описаны как раз в книге Фаулера.
                0
                Думая о «завистливых» функциях, вы все усложняете, да и приводите вообщем-то не совсем корректный пример.
                Суть вашей проблемы намного проще — константа PRODUCTS_SQL в другой сущности просто не нужна, а скорее всего даже нет необходимости в сущности SQLConsts. И это объясняется простым, общеизвестным правилом «не плоди сущностей сверх необходимого»
                  0
                  Не могли бы вы пояснить, почему вам кажется данный пример некорректным.
                    0
                    Вы лезете в дебри ООП, не до конца разобравшись с основами, оперируете довольно сложными понятиями, пытаясь объяснить их другим, при этом очень категоричны в своих выводах. В результате вы путаете причины и следствия, а также запутываете тех кто читает.
                    >>давайте понятные имена переменным, константам, методам и классам
                    взяли банальную фразу из учебника, попытались объяснить:
                    ProductProcessor->isCategoryVisible
                    Почему processor? а причем тут product, если у нас видимость категории? Странные какие-то сущности.
                    >>то, что изменяется одновременно, надо хранить в одном месте
                    опять очень-очень общая фраза из учебника, странный пример, где мы сначала создали новую сущность а потом прибили.
                    Ни капли объяснений почему так делать не стоило изначально, в чем минусы, как понять почему так делать не стоит.

                    И далее про Фаулера и его «завистливые» фукнции, мы ничего не объяснили, просто нагнали таинственности:
                    >>Такие функции Фаулер называет завистливыми, потому что они больше интересуются
                    >>не тем классом, в котором находятся, а каким-то другим.

                    В чем суть статьи вообще? Вы увеличили производительность своего кода и решили поделится?
                    Ну так ок — была лишняя сущность ее не стало, вот весь вывод.
                      0
                      Почему processor? а причем тут product, если у нас видимость категории? Странные какие-то сущности.

                      Полагаю, что не стоит делать выводы о названии класса, по названию всего одного приватного метода, не видя при этом ни одного публичного метода этого класса.

                      Если бы вы знали, что за многоточием скрывается метод processProduct, который в зависимости от видимости категории по разному обрабатывает продукты, у вас бы уже не возникали подобные вопросы?

                      И далее про Фаулера и его «завистливые» фукнции, мы ничего не объяснили

                      Полагал, что такого объяснения будет достаточно:

                      В нашем случае данные (SQL запрос) хранились в одном классе – SQLConsts, а функция isCategoryVisible, которая использовала эти данные – в другом: в ProductProcessor.


                      Поэтому isCategoryVisible можно назвать завистливой функцией. т. к. она как бы завидует, что другой класс SQLConsts хранит SQL запрос, который этому классу, по сути, не нужен, но который необходим методу isCategoryVisible.

                      Думаю, вы правы в том, что из моего объяснения было не совсем понятно, почему именно «завистливые» – попробую расширить объяснение в статье.

                      В чем суть статьи вообще?

                      Хотелось рассказать о завистливых функциях на простом примере, т. к. у того же Фаулера они описаны, на мой взгляд, не очень понятно.

                      В любом случае, спасибо, что потратили свое время и озвучили свое мнение.
          0
          не в ту ветку написал, а удалить коммент никак…
            +1
            Не думаю что она завидует. Она просит себе свою константу от класса который занимается хранением констант.
            Выделяли первоначально скорее всего чтобы уменьшит проблемы при адаптации к другому sql серверу.
            А вот параметр categoryID наводит на мысль что кто нить напишет код по переборке категорий в цикле для определения видимых что разом убьет производительность кучей запросов.
              0
              Не думаю что она завидует

              Хорошо, тогда ваше понимание завистливых функций в студию, плиз!

              А вот параметр categoryID наводит на мысль что кто нить напишет код по переборке категорий в цикле для определения видимых что разом убьет производительность кучей запросов.

              Согласен, однако, т. к. это приватный метод, контролировать его правильное использование довольно просто.
                0
                Ну для зависти можно добавить в isCategoryVisible какуюнить хитрую инициализацию или влияние на работу SQLConsts или дерганье его методов для получения хитрого sql, вообщем создать ощущение isCategoryVisible не столько проверяет видимость категории сколько манипулирует состоянием SQLConsts.
                  0
                  Согласен, это был бы хороший пример для классического фаулерского описания завистливых функций.

                  Все почему-то зациклились на том, что метод должен вызывать не меньше «полдюжины методов доступа к данным другого объекта», чтобы его можно было считать завистливым.

                  Давайте, в качестве примера, рассмотрим такой метод:

                      private void doSomething()  {
                          ...
                          ResultSet resultSet = executeQuery(SQLConsts.getProductsSQL(), categoryID);
                          int n = resultSet.getIntegerFieldByName("n");
                          boolean isCategoryVisible = n > 0;
                  
                          if (isCategoryVisible) {
                              resultSet = executeQuery(SQLConsts.getVisibleCategorySQL());
                              ...
                          } else {
                              resultSet = executeQuery(SQLConsts.getNotVisibleCategorySQL());
                              ...
                          }
                          resultSet = executeQuery(SQLConsts.getAnotherSQL(), ...);
                          ...
                          resultSet = executeQuery(SQLConsts.get...SQL(), ...);
                          ...
                          resultSet = executeQuery(SQLConsts.get...SQL(), ...);
                          ...
                      }
                  


                  Видно, что метод вызывает шесть методов доступа к данным другого класса, и очевидно, что он «больше интересуется не тем классом, в котором он находится, а какимто другим» – получается, этот метод смело можно назвать завистливым. И, кстати, можно заметить, что Фаулер ничего не говорит о том, что завистливый метод обязательно должен менять состояние объекта, которому он завидует.

                  А теперь вопрос: что изменится, если вынести код, вычисляющий isCategoryVisible, в отдельный метод? Разве метод doSomething не останется все еще завистливым?

                  А если все кроме одного запроса к базе вынести в отдельные методы? Т. е. в методе doSomething останется всего одна конструкция resultSet = executeQuery(SQLConsts.get...SQL(), ...) – разве мы избавим метод от зависти?

                  И если геттеры заменить на константы? Т. е. вместо SQLConsts.getProductsSQL() будет SQLConsts.PRODUCTS_SQL.

                  Очевидно, что все это не спасет ситуацию.

                  По поводу примера, приведенном в статье, возможно, следовало бы говорить уже о целом завистливом классе, т. к. можно предположить, что многие, если не все, методы класса CategoryRepository используют данные из SQLConsts, причем сам SQLConsts этими данными не пользуется.

                  Возможно, стоит также упомянуть, что в оригинале завистливые функции называются «feature envy», что в дословном переводе означает «функциональная зависть». Т. о., полагаю, Фаулер не ограничивается одними лишь функциями/методами, хотя и приводит в качестве примера только зависть методов.

                  В любом случае, спасибо за дискуссию.
              0
              А если вдруг в базе 1 млн записей, и пока БД своим курсором дойдет хотя бы до 1 записи с такой категорией, она может пройтись по тысячам записей.? В случае если по category_id нет индекса… Тогда я думаю запрос может выполняться очень долго.
                0
                В случае с rownum = 1 СУБД так же придется пройтись по 1000 записей, пока она не найдет первую подходящую запись, если нет индекса.
                Поэтому, как я уже писал, разницы в производительности для exists и rownum = 1 не будет никакой.

                Хотя, думаю, вы со мной согласитесь, что было бы неразумным не использовать индекс для category_id в данной ситуации, из чего можно с большой уверенностью предположить, что индекс все таки используется.

                В статье ничего не сказано про индексы, т. к. речь идет не об SQL оптимизации (ибо это отдельная и очень объемная тема).
                Но, в любом случае, exists и rownum = 1 будут намного быстрее, чем использовать count(*).

                Единственным исключением, если все же допустить, что индекса нет, будет случай, когда подходящая запись физически находится в самом конце таблицы. Только в этом случае СУБД придется просканировать всю таблицу целиком, и exists или rownum = 1 затратят столько же времени, что и count(*).
                  0
                  Однако же в статье это выглядит как решение проблемы с count(*), но это далеко не решение проблемы без индекса, особенно на огромных таблицах. Даже проход в 100 000 элементов стоит дорого.
                    0
                    Спасибо, что указали на это – добавил апдейт в конец статьи.

              Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

              Самое читаемое