О проблемах с code reviews

Кросс-пост с персонального как обычно...
--- 

Да-да, знаю... Очень необычно ругаться на code reviews (ревизии кода), особенно в мире где они воспринимаются чуть ли не как одиннадцатая заповедь, за неуважение к которой легко угодить на костер... Так что, потерпите немного ереси, я все обьясню!

Итак... Я не говорю, что ревизия кода – это плохо. Просто все в нашем грешном мире имеет свои преимущества и недостатки. Или как говорили утомленные мудростью греков римляне – cons et pros. Так вот, я хотел бы обратить ваше внимание на некоторую con ревизии кода, которая обычно не упоминается вслух...

Представьте себе, вы работаете в небольшой команде и вы все жутко заняты создавая новый продукт. Обратите внимание: «жутко заняты». И – удивительно, не правда ли? – как и в любом другом продукте, у вас есть баги. А баг – это такая штука, которую надо чинить. Без дураков, не шучу....

Теперь, вопрос на засыпку. Как вы будете чинить баг? Насколько я знаю, есть только две философии как это делать: заплатки и рефакторинг. Ну, да, да, есть еще идиотское «просто почини его», но мы не будем опускаться так низко, правда? Идиоты, которые не понимают о чем я говорю, могут прогуляться и не лезть в наши разговоры. А для нас – оставшихся – выбор все-таки есть. Итак, заплатки или рефакторинг?

Так, как вы думаете, что является правильным способом исправления багов? Да-да, есть случаи, когда заплатки – это верное решение. Например, Quick Fix Engineering – когда нужно доставить заплатку критическому пользователю с несколькими тысячами копий вашего софта. Другой пример – когда продукт уже давно сделан, и все что вы хотите – это трогать его как можно меньше. Ну, и, наконец, ситуации, когда каждый фикс – это очень большой риск. Скажем, когда вы исправляете софт для космического аппарата на орбите Юпитера... real time… и любой баг оставит ваших заказчиков с куском мертвого железа, стоящего много-много миллионов долларов на ... той самой орбите Юпитера.

Однако в большинстве случаев я бы поспорил, что в терминах «хорошего инжиниринга» рефакторинг обычно превосходит заплатки. Well… не просто превосходит... а как бы это сказать... «как бык овцу»! Не, правда. Дайте обьяснить на примере.

Учитывая короткий размер статьи, мне придется привести довольно примитивный пример, но все-таки, весьма наглядный, как мне кажется. Итак, представьте себе, что у вас есть метод ЗагрузкаЗакончена(), которая вызывается в момент, когда закончена загрузка куска медиа, и которая означает, что вы можете начать загрузку следующего куска. И тут вы заметили, что один из ваших коллег вставил в нее какой-то совершенно идиотский код. Нет, будем справедливы, код был бы совершенно неидиотским, если бы он бы вставлен не в ЗагрузкаЗакончена(), а в ЗагрузкаЗаконченаНаФиг(). Ну, что поделать, неудачный идентификтор. Но в результате, у вас выбор из двух опций:

1. Просто перенести код из ЗагрузкаЗакончена() в ЗагурзкаЗаконченаНаФиг(). Баг будет исправлен. Исправление занимает несколько строк перенесенных чуть-чуть вниз в файле.

2. Сделать (1) и переименовать ЗагрузкаЗакончена() в РазрешитьЗагрузкуСледующегоФайла(), чем, собственно, эта функция и является, тем самым предотвратив подобыне ошибки раз и навсегда. Исправление заденет дюжину-другую файлов.

Теперь, не забывайте, ваша команда действительно занята. Каковы ваши шансы получить code review (ревизию кода) быстро, при выборе (1) или при выборе (2)? Ага. Ну, да. Точно. 1. При попытке (2), парни быстро взглянут на список измененных файлов и тут же выпадут в осадок. Причем по хорошей причине. А вам совершенно не в кайф держать код черт-те-сколько на своей машине и тянуть с чекином, верно? Вот-вот.

А каков результат? Результат в том, что ваш продукт получит заплатку, а не рефакторинг. И через неделю другую, кто-то другой опять вляпается в этот самый ЗагрузкаЗакончена(), и вы будете править следующий баг-близнец... Хорошо, если такой же, а то и похуже...

Не знаю, честно, как это выглядит? Я и вправду о чем-то серьезном говорю, или мне мерещится?