Что действительно делает этот код плохим

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

FileTableEntry * FilerManager::GetFileTableEntry(uint32_t i) const { 
    return &(GetFileTable()[i]); 
} 

… 
for (uint32_t i = 0; i < container_.size(); ++i) { 
    *GetFileTableEntry(i) = FileTableEntry(); 
    // GetFileTableEntry ultimately accesses a std::vector in FileManager 
}

Мои основные аргументы:

Этот код очень непрямым и вводит в заблуждение, он не должен использовать поглотитель для initiliaze (часть FileManager), но не менее сеттер : будучи более прямым, код легче понять.
Геттер полностью теряет внутреннее состояние FileManager, поэтому инкапсуляция для FileManager в этот момент ничего не значит. Хуже того, геттер обещает быть применимым к объектам const, но он просто используется для изменения внутреннего состояния FileManager. Нарушение инкапсуляции - это верный путь к более эффективному рефакторингу.

Есть ли другие аргументы в пользу того, что вы не пишете такой код, который мне не хватает?

источник

2015-07-07 Frank

Если 'GetFileTable' возвращает' vector', то предоставление пользователю ссылки на позицию в этом векторе может оказаться плохим, если какая-либо вставка или удаление происходит с вектором (из-за изменения размера). Пользователь просто должен быть осторожным; возвращение итератора является лучшим предупреждением для потребителя. – AndyG

Технически, с кодом не так уж плохо, но да, это не совсем «хороший стиль» либо [при условии, что он возвращает ссылку на запись, а многопоточное выполнение не позволяет вектору расти/сокращаться и перераспределяться ] –

Возможно, вы могли бы предоставить немного больше кода, например, реализацию. 'GetFileTable' и как он связан с контейнером. Anxway, первое, что приходит мне на ум, - это то, почему таблица файлов должна быть инициализирована/сброшена в цикле вообще? Не будет ли более подходящим конструктор или 'std :: fill'? Другой вопрос, который у меня будет, это то, почему 'GetFileTableEntry' возвращает указатель вместо ссылки. – MikeMB

Другим аргументом против этого кода является то, что подпись GetFileTableEntry возвращает указатель в ситуациях, когда объект всегда присутствует. Это требует ссылки, а не указателя:

FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const { 
    return GetFileTable()[i]; 
}

Это должно касаться вашего первого пункта. Ваша вторая точка может быть решена путем создания эталонного const:

const FileTableEntry& FilerManager::GetFileTableEntry(uint32_t i) const { 
    return GetFileTable()[i]; 
}

Это запрещает звонящие от изменения внутреннего состояния, возвращенного GetFileTableEntry.

Примечание: Чтобы сделать функцию GetFileTableEntry полезной, следует добавить проверку границ проиндексированного индекса для раннего обнаружения ошибок.

источник

2015-07-07 22:31:04 dasblinkenlight

По крайней мере, при отладке 'GetFileTableEntry' должно отображать сообщение об ошибке и останавливать выполнение/создавать файл дампа, если' i' находится за пределами допустимого диапазона. – Yakk

Что действительно делает этот код плохим

ответ

Смежные вопросы