Еще чуть-чуть про code review, unit tests и тестовые задачки.

Семен все совершенно правильно расписал про Code Review. И хотя подобный способ несколько замедляет работу, он достаточно полезен при разработке и на 100% необходим в том случае, если речь идет про код, который не сегодня-завтра отправится на компы юзеров в виде патча или другого обновления.

Т.е. вообще за неделю другую до майлстоуна независимо от принятой у вас практики кодирования и коммитов полезно переходить на специальный режим работы, в котором запрещается добавление новой функциональности, все коммиты должны обособлено решать один четко описанный баг, ну а лид программер должен ревьюить всё, что поступающее поступает в main trunk.

Но вообще я хотел написать немного о другом. Несмотря на то, что меня можно причислить к яростным сторонникам code review как такового, честно признаюсь: я часто ленюсь анализировать код, заменяя это на активное (стрессовое) тестирование компоненты.
Буквально недавно была одна задачка (хотя я ее в свое время решал еще в Крейте) – создание пула для пакетов произвольного размера, которые хранятся в fixed size очереди. При переполнении буфера либо игнорируются новые пакеты, либо стираются самые старые пакеты, особождая место под новые данные.

Я считаю, это отличная задача для иллюстрации и способа стресс-тестирования данных взамен code review, и отличная тестовая задача для кандидатов на работу; подробности распишу ниже. Область применения этого буфера довольно широка – хранение TCP/IP тестовых или бинарных пакетов, хранение строк протокола для отображения в игровой консоли (чтобы не выделять память бесконечно, будем хранить только последние xxx строк, сколько влезет), другие временные буфера под очереди.

В общем-то, формулировка задачи уже есть, интерфейс к объекту описывается классическим push/pop/peek/count + возможно, итераторы + thread safe. Задача решается выделением буфера фиксированного размера и перемещением по циклическому буферу двух указателей – на голову и на конец списка. В том случае, если для вставки нового пакета не хватает памяти, второй указатель начинает удалять старые пакеты, перемещаясь по буферу и освобождая место под новые данные.

В чем главные особенности этой очереди?

  • достаточно аккуратная работа с указателями: переход через конец буфера на начало, добавление одного пакета может вызвать удаление нескольких старых (если старые по 10 байт, а новый – 100 байт), определение пустого или полностью заполненного буфера.
  • сложность визуального CR из-за большого количества сравнений указателей, особенно с аспектами “+-1″.
  • идеальная приспособленность для стресс-теста.

В чем особенности реализации:

  • необходимость выделить несколько ботлнеков, которые обеспечивают правильное перемещение указателей, чтобы не отлаживать все функции и преобразования указателей в них;
  • необходимость “перевязать” систему большим количеством ассертов на правильность указателей и согласованность их друг с другом и с остальными полями (например, полем m_count);
  • отработка граничных ситуаций (например, пакеты нулевого или очень большого размера);
  • аспекты создания итераторов и переборки пакетов в другом потоке;
  • создание стресс-теста на проверку корректности реализации. Стресс-тест может в рандомном режиме набрасывать данные в очередь, цинично сохраняя всю историю внутри себя, и постоянно выполнять сравнение корректности результатов (с точностью до стирания старых пакетов).

Собственная реализация заняла у меня около трех часов (вместе со стресс-тестом, общий размер кода – около 350 строк). Контрольный запуск выявил 3 ошибки:

  • в одном месте неправильно реализовывалось кольцевание указателя (бага с “+-1″).
  • при удалении пакета не всегда уменьшалось поле m_count, что вызвало ассерт на корректность (m_head == m_tell && m_count == 0);
  • через 3 минуты (!) после работы стресс-теста он ухитрился поймать ситуацию с совпадением m_head и m_tell после “хитрого” кольцевания буфера (тот же самый ассерт, что и в пункте 2);

Резюме:

  • Code Review хотя и полезен здесь в общих чертах, но поскольку стресс-тест в данной ситуации справляется быстрее, разумнее применить его.
  • Изначально надо продумать и спроектировать все bottle-neck точки, четко определить состояния “пустой”, “непусто” и “полный” буфер, определить логику thread-safe работы, особенно с итераторами.
  • Не забыть про граничные условия. Чтобы не думать долго, я тупо объявил и проверил, что размер пакета не может быть больше 1/3 размера очереди :)
  • Тщательно обвешивайте все ассертами.
  • Три с половиной часа на реализацию, из них 10 минут на стресс-тестирование.
  • CEMEH

    В принципе, вопрос, что делать раньше, Core Review или Unit Testing – в некоторой степени философский. Я тоже думаю, что сначала Unit Testing. И, разумеется, они дополняют, а не заменяют друг друга.

  • http://blog.murkt.org.ua/ Муркт

    > *четно* определить состояния “пустой”, “непусто” и “полный” буфер

    Здесь нет опечатки? Мне показалось, что должно было стоять слово «чётко».

  • dDIMA

    Ай спасибо. Хоть кто-то читает внимательно, а не по диагонали :). Поправил.

  • http://blog.murkt.org.ua/ Муркт

    Пожалуйста. В той же процитированной строке «непусто» выделяется из ряда – «непустой» звучит лучше. Хотел ещё сказать, что не нашёл кнопочки «Комментарии на почту», но вовремя заметил ссылку внизу «Comments (RSS)».

  • CEMEH

    Кстати, может, литературный review вообще на блоге – это неплохая идея?

  • DigiMind

    > Кстати, может, литературный review вообще на блоге – это неплохая идея?

    Уже, кстати, мысль вслух возникала (после статьи Бориса про интервью).
    Крайнего правда не нашли :)

  • caujka

    Думаю, хвост корректней назвать m_tail.
    “непусто” -> “не пусто”.
    А 3.5 часа на 350 строк, дебаг и тестирование – респект!