Это не я! История одного рефакторинга

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

    Началось с того, что экстренно потребовалось собрать форк приложения Aword, для которого уже больше года не выходило новых версий. По сути, сильно урезанную версию основного приложения Skyeng: в форке должны были быть тренировки, фид с контентом (сториз, видео и статьи), словарь и аудирование.

    С многомодульностью такие задачи решаются очень просто, и форк мы собрали в течение буквально нескольких часов. Успели порадоваться тому, что не стали когда-то тянуть с рефакторингом проекта. Записали в ноушн возникший по ходу дела техдолг. 

    Но одному модулю было нехорошо.

    Аудирование.

    В основном приложении его нет, поэтому мы оттягивали момент рефакторинга достаточно долго. И написанный в незапамятные времена модуль listening никто не трогал, не подключал и не тестировал уже год. За это время все остальные модули проекта, в том числе те, от которых зависел listening, успели сильно измениться.

    Вот что предстояло воскресить.
    Вот что предстояло воскресить.

    Аудирование — вполне самостоятельная и не то чтобы маленькая фича: список аудиофайлов с пагинацией, экран с плеером и субтитрами (и сервис для плеера), экраны для упражнений и тестов после прослушивания, свои, отдельные от остального приложения, настройки, фильтры по тегам и категориям…

    - Мы никак не можем обойтись без листенинга в этом приложении?

    - Никак.

    - Его давно никто не собирал… Доков нет. Растительности нет. Населена роботами.

    - Ага.

    - А кто его последний трогал?

    - Я и трогала. Был прошлой весной один баг в субтитрах…

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

    Типичный диалог того времени выглядел так:

    - Как ты себя чувствуешь?

    - Я рефакторю листенинг вторую неделю. Не очень. А кто его писал, кстати?

    - Это не я!

    И вот почему.

    Документация и тесты

    Документации в модуль аудирования не завезли — в отличие от остального проекта. (Единственным видом комментариев был копирайт. И благодаря ему я знала, кого хочу убить раньше, чем до меня доберутся те, кто вынужден поддерживать мой код на прошлых местах работы) 

    Тесткейсы. Чаще всего у нас они имеют названия, лаконично обрисовывающие сценарий использования. Если сущности неудачно названы, javadoc отсутствует или код не поддаётся беглому прочтению — они приходят на помощь, что делает тесты ничуть не менее важной частью доков, нежели комментарии или вики. В данном случае мне этого остро не хватало с самого начала.

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

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

    Код в модуле

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

    public abstract class LceActivityWithTracking <T, V extends LceView<T>, P extends LcePresenter<T, ?, ? extends RxUseCase<T, ?>, V>>
            extends LceActivity<T,V,P>

    Честно сказать, я далеко не сразу поняла, что принимает описанный выше класс, и в какой последовательности. В какой-то период я тоже стремилась использовать дженерики буквально повсюду, усмотрев в них хороший способ фиксировать важные ограничения. Но теперь знаю, что дженерики такой сложности и вложенности хороши лишь тогда, когда их пишешь. А не читаешь.

    Скорее всего, написанный код был крайне очевидным для его автора. Для меня он очевидным не был. Зато навёл на не менее очевидную мысль, что лучше бы не забивать на доки, даже если всё кажется очевидным. Возможно, стоит даже писать javadoc ко всему, что вызвало вопросы на код-ревью — но это плавный и непрерывный процесс, который сложно исправить одним волевым усилием. Особенно если надо на какое-то время срочно законсервировать модуль.

    Или вот, например. Маленький класс, в котором всего было около пятидесяти строчек:

    public class GetSubtitlesUseCase extends SerialUseCase<List<SubtitleItem>, SubtitlesIds>

    Всё вроде хорошо, правда? Но SerialUseCase скрывал под собой ещё четыре уровня наследования, и все они были выпилены не меньше полугода назад за полной невостребованностью. Пришлось раскапывать.

    Примерно везде меня ждали пагинация, кэширование, lce-паттерны и утилитные методы, спрятанные под капотом ныне несуществующих классов.

    К счастью, я достаточно активно ввязалась в рефакторинг проекта, и многие из попавшихся базовых классов выпиливала и переписывала тоже я — это принесло некоторое представление о том, как они были устроены.

    И это подтолкнуло к выводу: если есть вероятность того, что выключенный из проекта код долго не будет пересобираться, проверяться и использоваться — лучше сразу положить в него пакет с задействованными в нём базовыми классами. Их может внезапно не оказаться после рефакторинга. А может не оказаться ни документации, ни людей, которые помнят, что там в принципе было)

    Переписывая экраны

    На моё счастье, модуль очень слабо зависел от основного проекта в плане информации — всего две входящие зависимости. Поскольку он не нуждался практически ни в чём от окружающего мира, было достаточно легко восстановить всё, связанное с dagger.

    С навигацией было немного посложнее. Подход с single-activity ещё не успел войти в моду, и на отдельных активити были написаны даже самые элементарные экраны, вроде голосовалки, понравился ли учебный материал.

    Потом я стала потихоньку раскомменчивать и переписывать экраны на kotlin, сверяясь с версией с прода и картой функциональности, а иногда и просто выкидывать старый код. Сначала — главная с выбором материала. Потом — экраны попроще. Потом — посложнее. Некоторые классы просто остались болтаться, как лишние детали после пересборки.

    Чтобы не сойти с ума, мы созванивались с тимлидом и дробили задачи на более мелкие с более понятным definition of done. Интересные задачи приносили фан сами по себе, с неинтересными и нудными я придумывала челленджи. Жизнь на тот момент так или иначе состояла в основном из четырёх стен, кота и кода. А большую часть времени в четырех стенах естественным образом занимала работа: как деятельность с наибольшей и гарантированной эмоциональной отдачей. 

    В конце концов модуль, доведённый до примерно рабочего состояния, влился в мастер, мы выдохнули. А затем я сделала для ребят внутренний доклад про этот опыт. Ну, как доклад. Мне просто хотелось пожаловаться и не держать это в себе. 

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

    И тогда тот, кто столкнётся с законсервированным модулем, будет чуть меньше хотеть отыскать ваш адрес и прийти к вам с топором. Или добыть ваши волосы для ритуала вуду… так, что-то меня понесло.

    p.s. И да, чувак, которому не повезёт заглянуть в этот модуль следующим. Listening писала не я. Я его рефакторила.

    UPD: убран небольшой эпизод про созвон команды, на техническую сторону поста он не влияет.

    UPD-2: историю коммитов несколько лет назад схлопнули — она была большой, глючил поиск по коммитам, предстоял глобальный рефакторинг всего, и коллеги решили, что так надо. Везде осталось только имя разработчика, который это сделал. Весь код, о котором говорится в статье, был написан до этого момента, и с тех пор практически не затрагивался.

    Skyeng
    Крупнейшая онлайн-школа Европы. Удаленная работа

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

      +2
      Готовый материал для сценария :). Комедия ужасов с хэппиэндом и онлайн-оргией в титрах :).
        +1

        Какая-то чушь, начиная от того "кто это писал?"(вы что, VCS не пользуетесь?) и до выпиливания "ненужных базовых классов", после которого остались какие-то ошмётки, всё ещё зависящие от этих классов. Сначала думал, что это какой-то пхп, где такое возможно, но нет, это джава, развитые инструменты разработки, рефакторинга. Как вообще можно было довести проект до такого состояния?

          0
          Вы с легаси не работали, видимо, так ещё не такой адок бывает =)
            0
            Работал, и в такой примерно ситуации бывал. И годами мы с командой выпиливали плохие решения, и отдельные активити с легаси у нас сохранялись очень долго. Но бездумно выпиливать какие-то классы, не глядя на зависимости? Нет.
              +1

              Я так понял, к моменту разгребания конюшен, классов уже не было. Если не так понял, то это автор зря, конечно.

            +3
            вы что, VCS не пользуетесь?

            Важный момент, о котором стоило упомянуть: историю коммитов несколько лет назад схлопнули — она была большой, глючил поиск по коммитам, предстоял глобальный рефакторинг всего, и коллеги решили, что так надо. Везде осталось только имя разработчика, который это сделал. Весь код, о котором говорится в статье, был написан до этого момента, и с тех пор практически не затрагивался.

            ошмётки, всё ещё зависящие от этих классов

            Модуль очень долго был отключён, не запускался и не тестировался — ни на ci, ни где-либо ещё. Остальной проект уже какое-то время счастливо жил на новых базовых классах, а рефакторинг модуля, который неизвестно когда понадобится и неизвестно, будет ли жить, тянул дополнительные издержки, и мы его откладывали.
              +1
              Кажется, вы судите по специально выбранному случаю о состоянии всего проекта.

              Но вы правы в одном: у пхп-разрабочиков и правда бывает куда жестче в плане легаси)
                0
                Хорошо если на стадии жизни проекта VCS было не больше двух…
                А так некоторые проекты переживали три перехода в разные VCS :-)
                  0

                  Это всё понятно, но сейчас примерно все начинают проекты в гите. И вряд ли андроид проект ездил из одной VCS в другую.

                +1

                У вас разве не все модули заведены в один проект? И не все разом собираются на ci?

                  +1
                  У вас разве не все модули заведены в один проект? И не все разом собираются на ci?

                  Да, всё так, но конкретный модуль был отключён.
                    +1

                    Блин, вы этим постом и комментариями в корпблоге капец антирекламу делаете:) с такими процессами я бы 100 раз подумал прежде чем к вам идти.

                      0
                      Ну вот видите, вы уже о нас думаете) В любой компании, выросшей из стартапа и команды разработки в пару человек, будет страшное что-то, доставшееся новой команде в наследство от людей, которых особо не помнят.

                      Процессы не рождаются сами по себе, они строятся людьми. Сейчас ребятам удалось построить процессы так, что они могут развивать линейку приложений, пилить фичи и при этом в сроки попадать (даже при учете таких ситуаций). Они молодцы в этом плане — просто вы, возможно, не видите этого и не захотите об этом читать.

                      А как у вас в команде процессы построены, кстати?
                  +1
                  Единственным видом комментариев был копирайт. И благодаря ему я знала, кого хочу убить раньше, чем до меня доберутся те, кто вынужден поддерживать мой код на прошлых местах работы)

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


                  А иногда стыдно сразу и приходится оставлять комментарий типа:
                  // залипуха жуткая. Но сроки жмут… надеюсь никогда не придется этот кусок развивать. Извините.

                  Увы… жизнь…
                  Но, пальцем иногда хочется потыкать в копираторов с гордостью публикующих свое ФИО в коде.


                  Например крипто библиотека одобренная ФСБ(!) где данные в TCP/IP летают открыто, а пароль(!!!) для "аутентификации" (!) запроса на подпись/шифрование отправляется вместе с данными "зашифрованный" XOR по константному байту (!!!).


                  Ой… надеюсь никогда до такого позора не докачусь. Но на всякий случай свой копирайт не вставляю :)

                    +1
                    I feel your pain, bro!.. ©
                      +1
                      Bro, she's a sis!
                        0
                        You fool! It's pain that matters, not gender…

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

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