[prog.c++] Пример очень простого рефакторинга для простого, но объемного кода

Ув.тов.asfikon в своем блоге eax.me ведет серию постов, посвященных изучению работы с OpenGL (вот самый свежий пост из этой серии). Для работы с OpenGL используется C++. Код живет на GitHub, где его можно посмотреть и пощупать руками.

Любопытства ради заглянул, глянул краем глаза. Код как код, очень простой, разобраться в нем труда не составляет. Смело могу сказать, что доводилось видеть намного более страшные образчики C++ного кода. Однако...

Однако, чаще всего подобный код пишут новички, которые еще только изучают C++, или же математики/физики и пр. ученые, для которых программирование далеко не основное занятие и C++ ими используется для выполнения расчетов в качестве этакого современного Фортрана. Т.е. очень похожий стиль показывают люди, которые либо еще не освоили идеологию C++, либо же не имеют возможности это сделать. Отсюда и получается код, который прост, но, к сожалению, местами слишком объемен.

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

Именно поэтому мой взгляд зацепился за реализацию работы с отображаемыми в память файлами (речь про utils/fileMapping.h и utils/fileMapping.cpp).

В заголовочном файле приведены описания функций для работы с отображаемым в память файлом:

#ifndef AFISKON_FILEMAPPING_H #define AFISKON_FILEMAPPING_H struct FileMapping; FileMapping * fileMappingCreate(const char* fname); unsigned char* fileMappingGetPointer(FileMapping * mapping); unsigned int fileMappingGetSize(FileMapping * mapping); void fileMappingClose(FileMapping * mapping); #endif //AFISKON_FILEMAPPING_H

Тут видно, что автор кода решил использовать стиль, который более характерен для Go и plain old C, нежели для C++. Вместо класса, который реализует идиому RAII и предоставляет несколько методов для работы с отображенным в память файлом, вводятся свободные функции, оперирующие некоторым непрозрачным для пользователя хендлом (в данном случае указателем на структуру FileMapping, определение которой скрыто от пользователя). Соответственно, т.к. RAII нет, пользователь должен вручную закрывать хендл, т.е. вызывать fileMappingClose для каждого успешного вызова fileMappingCreate.

Подход как подход. Вполне имеет право на жизнь. Особенно в случаях, когда пишется библиотека, выставляющая наружу plain old C интерфейс, чтобы ее можно было использовать из разных языков, включая Python/Ruby/Erlang и т.д.

Дабы не иметь проблем с вызовом fileMappingClose, ув.тов.asfikon использует библиотеку deferxx, которая имитирует инструкцию defer из языка Go. В коде это выглядит приблизительно так:

FileMapping* mapping = fileMappingCreate(fname);   if(mapping == nullptr) return false;   defer(fileMappingClose(mapping));   unsigned char* dataPtr = fileMappingGetPointer(mapping);   unsigned int dataSize = fileMappingGetSize(mapping);

Думаю, что для Go-шных разработчиков этот подход выглядит как родной и проблем с написанием и пониманием такого кода у них нет.

Тем не менее, реализация fileMappingCreate/fileMappingClose мне лично показалась хорошим местом, на котором можно продемонстрировать, как средствами C++ (особенно современного C++) можно уменьшить объем кода и упростить себе жизнь.

Для начала продемонстрирую, как выглядел оригинальный вариант работы с FileMapping для одной платформы, Win32:

struct FileMapping {   HANDLE hFile;   HANDLE hMapping;   size_t fsize;   unsigned char* dataPtr; }; FileMapping * fileMappingCreate(const char* fname) {   HANDLE hFile = CreateFile(fname, GENERIC_READ, 0, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);   if(hFile == INVALID_HANDLE_VALUE) {     std::cerr << "fileMappingCreate - CreateFile failed, fname =  " << fname << std::endl;     return nullptr;   }   DWORD dwFileSize = GetFileSize(hFile, nullptr);   if(dwFileSize == INVALID_FILE_SIZE) {     std::cerr << "fileMappingCreate - GetFileSize failed, fname =  " << fname << std::endl;     CloseHandle(hFile);     return nullptr;   }   HANDLE hMapping = CreateFileMapping(hFile, nullptr, PAGE_READONLY, 0, 0, nullptr);   if(hMapping == nullptr) { // yes, NULL, not INVALID_HANDLE_VALUE, see MSDN     std::cerr << "fileMappingCreate - CreateFileMapping failed, fname =  " << fname << std::endl;     CloseHandle(hFile);     return nullptr;   }   unsigned char* dataPtr = (unsigned char*)MapViewOfFile(hMapping, FILE_MAP_READ, 0, 0, dwFileSize);   if(dataPtr == nullptr) {     std::cerr << "fileMappingCreate - MapViewOfFile failed, fname =  " << fname << std::endl;     CloseHandle(hMapping);     CloseHandle(hFile);     return nullptr;   }   FileMapping* mapping = (FileMapping*)malloc(sizeof(FileMapping));   if(mapping == nullptr) {     std::cerr << "fileMappingCreate - malloc failed, fname = " << fname << std::endl;     UnmapViewOfFile(dataPtr);     CloseHandle(hMapping);     CloseHandle(hFile);     return nullptr;   }   mapping->hFile = hFile;   mapping->hMapping = hMapping;   mapping->dataPtr = dataPtr;   mapping->fsize = (size_t)dwFileSize;   return mapping; } void fileMappingClose(FileMapping* mapping) {   UnmapViewOfFile(mapping->dataPtr);   CloseHandle(mapping->hMapping);   CloseHandle(mapping->hFile);   free(mapping); } unsigned char* fileMappingGetPointer(FileMapping * mapping) {   return mapping->dataPtr; } unsigned int fileMappingGetSize(FileMapping * mapping) {   return (unsigned int)mapping->fsize; }

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

Сразу отмечу, что если ставить задачу переписать код в идиоматическом для C++ стиле, то нужно проводить серьезный рефакторинг: создавать класс FileMapping, реализующий RAII и, скорее всего, использующий исключения для информирования об ошибках. Но это слишком серьезная переделка. Более того, зачастую при написании quick-and-dirty прототипов и тестовых программ не всегда есть время для проектирования классов и их реализации "по всем правилам". Поэтому попробую изменить код ув.тов.asfikon-а без существенного изменения принципов: точно так же останется некий непрозрачный для пользователя хендл + несколько функций для работы с ним.

Тем не менее, все равно будет присутствовать какой-то объект, который будет чистить ресурсы у себя в деструкторе. Т.к. пара из fileMappingCreate/fileMappingClose -- это слишком серьезный источник потенциальных проблем, даже при использовании defer-а. Поэтому первая задача №1: оставить fileMappingCreate, fileMappingGetPointer и fileMappingGetSize, но избавиться от fileMappingClose и необходимости ее ручного вызова.

Как это сделать?

Пойдем простым путем и воспользуемся готовым классом std::unique_ptr из стандартной библиотеки C++11. Может быть кто-то не в курсе, но шаблон unique_ptr имеет два параметра. Второй параметр -- это deleter, отвечающий за уничтожение находящегося под контролем unique_ptr ресурса. По умолчанию это std::default_deleter, который вызывает delete. Но в качестве deleter-а можно использовать и указатель на функцию. Чем и воспользуемся. В результате содержимое fileMapping.h примет вот такой вид:

#ifndef AFISKON_FILEMAPPING_H #define AFISKON_FILEMAPPING_H #include <memory> struct FileMapping; using FileMappingUniquePtr = std::unique_ptr< FileMapping, void (*)(FileMapping *) >; FileMappingUniquePtr fileMappingCreate(const char* fname); const unsigned char* fileMappingGetPointer(FileMapping & mapping); unsigned int fileMappingGetSize(FileMapping & mapping); #endif //AFISKON_FILEMAPPING_H

Итак, теперь fileMappingCreate возвращает не голый указатель на FileMapping, а unique_ptr. Соответственно, надобность в публичной fileMappingClose отпадает. И использование обновленного интерфейса fileMapping будет выглядеть вот так:

auto mapping = fileMappingCreate(fname);   if(!mapping) return false;   const unsigned char* dataPtr = fileMappingGetPointer(*mapping);   unsigned int dataSize = fileMappingGetSize(*mapping);

Отличие от оригинального варианта использования fileMappingCreate всего в одной строке. Но, на основании своего многолетнего опыта работы с C++ могу уверенно утверждать, что это очень серьезное отличие.

Теперь можно взяться за переделку внутренностей fileMapping.

Самая первая вещь, от которой хочется избавиться -- это ручная очистка результатов предыдущих действий при возникновении ошибок на очередной операции. Т.е. все те самые CloseHandle() внутри if-ов.

Как это сделать?

Ответ очевиден: типичным для C++ способом. А именно: перекладыванием ответственности за очистку ресурсов на чей-то деструктор. В данном случае понятно чей именно -- это деструктор структуры FileMapping. Поэтому соответствующим образом переделываем эту структуру:

struct FileMapping {    HANDLE hFile = INVALID_HANDLE_VALUE;    HANDLE hMapping = nullptr;    unsigned int fsize = 0;    const unsigned char* dataPtr = nullptr;    FileMapping() {}    ~FileMapping() {       if(dataPtr) UnmapViewOfFile(dataPtr);       if(hMapping) CloseHandle(hMapping);       if(INVALID_HANDLE_VALUE != hFile) CloseHandle(hFile);    }    FileMapping(const FileMapping&) = delete;    FileMapping(FileMapping&&) = delete; };

Здесь сам деструктор FileMapping будет очищать все ресурсы, выделение которых завершилось успешно. А начальные значения для атрибутов FileMapping позволяют определить, что именно успели сделать, а что нет.

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

После таких изменений fileMappingClose вырождается в тривиальный однострочник, внутри которого вызывается delete для указателя на FileMapping. Адрес fileMappingClose будет передаваться в FileMappingUniquePtr в качестве deleter-а.

Теперь, получив автоматическую очистку ресурсов прямо внутри FileMapping, а так же автоматическое удаление самого объекта FileMapping, можно записать реализацию fileMappingCreate в виде простой последовательности if-ов с простыми return-ами в случае неудачи.

Однако, в оригинальном коде есть еще один момент, от которого хотелось бы избавиться -- это повторение очень похожих печатей сообщений об ошибках в std::cerr. Действия эти совершенно однообразные, отличающиеся только именем проблемной функции внутри сообщения. Поэтому, дабы устранить их дублирование, воспользуемся фичей C++11, позволяющей иметь в C++ локальные функции: объявим лямбду внутри fileMappingCreate:

auto logErrorThenReturnNull = [fname](const char * what) {       std::cerr << "fileMappingCreate - " << what << " failed, fname = " << fname << std::endl;       return FileMappingUniquePtr{ nullptr, &fileMappingClose };    };

Эта лямбда не только печатает сообщение об ошибке, но еще и возвращает нулевой FileMappingUniquePtr. Что позволяет под if-ом писать просто return logErrorThenReturnNull.

В итоге, вся реализация fileMapping для платформы Windows принимает вот такой вид:

struct FileMapping {    HANDLE hFile = INVALID_HANDLE_VALUE;    HANDLE hMapping = nullptr;    unsigned int fsize = 0;    const unsigned char* dataPtr = nullptr;    FileMapping() {}    ~FileMapping() {       if(dataPtr) UnmapViewOfFile(dataPtr);       if(hMapping) CloseHandle(hMapping);       if(INVALID_HANDLE_VALUE != hFile) CloseHandle(hFile);    }    FileMapping(const FileMapping&) = delete;    FileMapping(FileMapping&&) = delete; }; static void fileMappingClose(FileMapping* mapping) {    delete mapping; } FileMappingUniquePtr fileMappingCreate(const char* fname) {    auto logErrorThenReturnNull = [fname](const char * what) {       std::cerr << "fileMappingCreate - " << what << " failed, fname = " << fname << std::endl;       return FileMappingUniquePtr{ nullptr, &fileMappingClose };    };    FileMappingUniquePtr result{ new FileMapping, &fileMappingClose };    result->hFile = CreateFile(fname, GENERIC_READ, 0, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);    if(result->hFile == INVALID_HANDLE_VALUE)       return logErrorThenReturnNull( "CreateFile" );    DWORD dwFileSize = GetFileSize(result->hFile, nullptr);    if(dwFileSize == INVALID_FILE_SIZE)       return logErrorThenReturnNull( "GetFileSize" );    result->hMapping = CreateFileMapping(result->hFile, nullptr, PAGE_READONLY, 0, 0, nullptr);    if(result->hMapping == nullptr) // yes, NULL, not INVALID_HANDLE_VALUE, see MSDN       return logErrorThenReturnNull( "CreateFileMapping" );    result->dataPtr = static_cast< const unsigned char* >(MapViewOfFile(result->hMapping, FILE_MAP_READ, 0, 0, dwFileSize));    if(result->dataPtr == nullptr)       return logErrorThenReturnNull( "MapViewOfFile" );    result->fsize = static_cast<unsigned int>(dwFileSize);    return result; } const unsigned char* fileMappingGetPointer(FileMapping & mapping) {   return mapping.dataPtr; } unsigned int fileMappingGetSize(FileMapping & mapping) {   return mapping.fsize; }

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

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

PS. Очень не хотелось бы обсуждать вопросы вроде "а зачем нужно было делать fileMapping вручную вместо того, чтобы взять готовую реализацию из моей любимой библиотеки?" Вопрос здесь вовсе не в конкретном API, над которым приходится делать собственную обертку. На все случаи жизни готовых оберток не наберешься. Время от времени приходится вручную чистить какие-то специфические ресурсы. И тогда возникает далеко не празный вопрос: "Выгоднее ли работать с ресурсами в стиле plain old C, в стиле Go или же использовать фичи C++?"

PPS. Продолжение темы уже в стиле наворотов а-ля Андрей Александреску :)