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

Однажды наша команда обнаружила, что сильно проседает производительность системы при выполнении довольно простого 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 = ?";     ... 

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

public class ProductProcessor {     ...     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 ProductProcessor {     ...     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, которая использовала эти данные – в другом: в ProductProcessor. Такие функции Фаулер называет завистливыми, потому что они больше интересуются не тем классом, в котором находятся, а каким-то другим. И чаще всего предметом зависти являются данные.

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

P. S.

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

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