[prog.flame] На тему понятности кода

У меня достаточно специфическое отношение к такому занятию, как "программирование". Для многих разработчиков целью является лишь решение поставленной задачи. Грубо говоря, если нужно взять картинки вот отсюда, добавить на них вот это вот и отобразить вот там, и написанная программа все это делает, то цель полностью достигнута.

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

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

Изучаю демо-программу, входящую в состав одного очень известного открытого проекта.

Встречаю в коде функции длиной в 300+ строк вот такой вот фрагмент (все на чистой ламповой Сишечке):

pkt_in_play_range = ... /* Какое-то вычисление */; if (pkt->stream_index == is->audio_stream && pkt_in_play_range) {     packet_queue_put(&is->audioq, pkt); } else if (pkt->stream_index == is->video_stream && pkt_in_play_range            && !(is->video_st->disposition & AV_DISPOSITION_ATTACHED_PIC)) {     packet_queue_put(&is->videoq, pkt); } else if (pkt->stream_index == is->subtitle_stream && pkt_in_play_range) {     packet_queue_put(&is->subtitleq, pkt); } else {     av_packet_unref(pkt); }

И понимаю, что мне этот фрагмент не нравится. Что он избыточно сложный. Что сложности в него добавляет повторяющаяся проверка pkt_in_play_range, которая есть в каждом if-е.

Очевидная штука, которую бы я попытался сделать сразу -- это вынести проверку pkt_in_play_range в отдельный объемлющий if:

pkt_in_play_range = ... /* Какое-то вычисление */; if (pkt_in_play_range) {     if (pkt->stream_index == is->audio_stream) {         packet_queue_put(&is->audioq, pkt);     } else if (pkt->stream_index == is->video_stream                && !(is->video_st->disposition & AV_DISPOSITION_ATTACHED_PIC)) {         packet_queue_put(&is->videoq, pkt);     } else if (pkt->stream_index == is->subtitle_stream) {         packet_queue_put(&is->subtitleq, pkt);     } else {        av_packet_unref(pkt);     } } else {     av_packet_unref(pkt); }

Вероятно, автор фрагмента не стал писать так потому, что в итоге получается чуть больше кода и, главное, вызывать av_packet_unref теперь приходится два раза в двух разных ветках. Что в условиях отсутствия механизмов RAII не есть хорошо и может быть чревато в будущем, когда этот фрагмент потребуется расширить.

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

static void store_pkt_to_appropriate_queue_or_discard(     VideoStream * is,     AVPacket * pkt,     int in_play_range) {     if (in_play_range) {         if (pkt->stream_index == is->audio_stream) {             return packet_queue_put(&is->audioq, pkt);         } else if (pkt->stream_index == is->video_stream                    && !(is->video_st->disposition & AV_DISPOSITION_ATTACHED_PIC)) {             return packet_queue_put(&is->videoq, pkt);         } else if (pkt->stream_index == is->subtitle_stream) {             return packet_queue_put(&is->subtitleq, pkt);         }     }     av_packet_unref(pkt); }

С очевидным способом использования:

store_pkt_to_appropriate_queue_or_discard(is, pkg, ... /* Какое-то вычисление */);

Тем самым размер большой функции из 300+ строк уменьшился бы на 10 строчек, а ее читабельность несколько улучшилась бы.

Да, общий бы объем кода увеличился бы, но зато он стал бы понятнее. По крайней мере для меня.