HW #3

[Egor Suvorov]

Это уже вторая попытка, к сожалению. Я не уверен, что имеет смысл её проверять прямо сейчас. Поменяйте обратно на "ожидается проверка", если вы действительно хотите потратить вторую попытку (например, у вас должны выполняться требования к промежуточной попытке, чтобы получить третью).

[Brilliantov Kirill]

У меня только не до конца напсианы тесты, а все остальное готово, поэтому, наверное, нет смысла ждать четверга...

[Egor Suvorov]

Проверялась ревизия 4014 или ранее.

Третья попытка разблокирована.

Общее впечатление: стереть половину кода/функций/методов, распилить HuffmanArchiver.h и Utility.h` и станет сильно хорошо.

Корректность 7/9

• Регулярно ошибается на +- 1 в размере сжатого файла. На пустом файле, например. Иногда не ошибаюется, впрочем.
• Сейчас у вас нельзя обработать файл с именем -foo.txt, а зря.
• Если произошла ошибка, код возврата должен быть ненулевой. У вас, судя по main, это не так ни в одном из трёх случаев (в одном вообще TODO стоит).

Тесты 5.5/8

• Не протестирован CLI
• Не протестирован подсчёт статистики (у вас как раз тут баг)
• Не протестирован битовый ввод-вывод отдельно от всего остального
• TestHuffmanTree.cpp
  • Дублирование кода: isLeaf дублирует метод вершины, onlyOne — это просто ^ или != на выбор.
  • Каждый из existingLeafs — это отдельный CHECK.
  • Лучше тестировать белый ящик: вы полностью знаете алгоритм и в каком порядке будут вершины. Для этого лучше использовать stable_sort. Тогда уйдёт вообще вся логика из тестов. Тем более что test generating codes у вас и так предполагает что-то серьёзное про алгоритм.
• TestHuffmanArchiver.cpp
  • Вместо чтения из stringstream и хитрых циклов спросите содержимое целиком при помощи .str() и сравните с известным массивом.
  • Вообще неясно, что делает цикл в конце write weight — вы считали данные, но не проверили. Это странно.

Архитектура 1.5/5

• Вместо двух вложенных namespace лучше писать сразу namespace hw_03::huf_cli. А ещё лучше hw_03::cli, всё-таки huffman следует из hw_03.
• Неясно, зачем ограничивать move у CLI/IOStatistic и ещё много кого. Вообще move обычно полезен. Copy ещё можно семантически ограничивать (если прям нельзя придумать "что такое копия"), а вот move обычно не стоит. Да и копия у вас практически везде делается очевидным образом и может иметь смысл.
• CLI
  • Сеттеры лучше закрыть. И неясно, почему они принимают const char*, а не хотя бы string_view (а ещё лучше string по значению, потому что в поле всегда будет записываться string).
  • ACTION скорее принадлежит к CLI, а не к Utility
• ArgumentsFormatException лучше отнаследовать от runtime_error/logic_error и сделать using конструкторов.
• Utility.h — сразу по названию понятно, что тут сборка всего подряд. Но, к сожалению, не по делу:
  • Action можно перенести
  • Обобщение HuffmanByte можно убрать, если сказать, что у нас нет спецсимвола для конца потока. Например, как-то по-другому понять длину потока (входного или выходного, смотря что проще).
  • HuffmanByteLength используется только в одном .cpp.
  • HuffmanBit лучше убрать и использовать честный bool, напоретесь на vector<bool> — нестрашно. Сейчас код сильно раздувается и обобщается, сложно читать. Лучше уж HuffmanCode замените сразу на что-то ещё. А бит — всегда bool.
  • LeftChildBit/RightChildBit — тоже лишнее обобщение, не нужно, это вряд ли будет меняться, лучше захардкодить, а не обобщать.
  • getBit(HuffmanWeight, size_t) — зачем получать i-й бит у _веса_ вершины? O_O А ещё это снова приватная для HuffmanArchiver.cpp функция. И наверняка ещё можно заинлайнить, она не ахти сложная, но код сильно раздувает, если вынести.
  • Категорически неясен смысл ByteMap. Чем обычный map или массив не угодил? А так мне придётся для чтения кода абсолютно новый интерфейс с какими-то лямбдами учить. А ещё не работает list-initialization (в тестах бы пригодилось).
• IOStatistic — не show(), а operator<<. Но респект за подсчёт бит (правда, там косяк, смотри корректность).
• Где-то CHAR_BIT, а где-то 8. Если уж знаете про CHAR_BIT, то поставьте просто static_assert на его размер, не надо подгадывать под древние машины с не тем размером байта.
• Huffman
  • using TreeNode сослужил вам плохую службу: маскирует кучу стилистических замечаний (смотри ниже) плюс постоянно, постоянно путается с HuffmanNode. Лучше явно написать unique_ptr.
  • Неясно, зачем отделять merge и конструктор от двух вершин.
  • HuffmanTreeTraverser
    • Не выполнено правило пяти. А лучше — правило нуля.
    • Лучше не влево/вправо, а по значениям бит. И неясно, зачем getLeftChildCursor(), если класс копируемый. Лучше публичных методов по минимуму. Да и canGoLeft/canGoRight наверняка убиваются при помощи assert внутри goLeft.
    • Неясно, зачем куча operator->, get()... Кстати, тогда уж и operator* надо, раз под умный указатель мимикрия. Но лучше не надо.
    • Кажется, что если полностью выкинуть HuffmanTreeTraverser и оставить только вершину с геттерами, будет примерно то же самое.
  • HuffmanTree
    • Конструктор по умолчанию не инициализирует поля. Будет UB.
    • Неясно, зачем отдельный конструктор и отдельный метод build().
• HuffmanArchiver — снова надо распилить минимум на два файла.
  • Тут побайтовый/побитовый ввод-вывод не по делу. Это отдельные сущности.
  • Аналогично, createFrequencies точно выносится. А ещё эти группы функций в конце HuffmanArchiver.h тонко намекают, что, возможно, они на самом деле говорят про какой-то объект.
  • shared_ptr не по делу. По факту IOStatistic всегда живёт не меньше, чем соответствующий читатель/писатель.
  • BitReader::getWeight() — это чтение не веса, а числа определённого размера. Как минимум стоит назвать getHuffmanWeight(), а ещё лучше — read (да и остальные get тоже). А ещё неясно, зачем эти функции, когда уже есть operator>>.
  • BitReader::flush() — шта.
  • Кажется, что BitReader стоит разбить как минимум на битовый-ввод и всё остальное (типа чтения байтов, кодов, весов).
  • HuffmanArchiver — кажется, что getStats() лучше заменить на возвращаемое значение compress/extract, будет очевиднее семантика "что за статистику получили".

Стиль 2.5/8 (болдом выделены очень существенные замечания, каждое на -1 балл сразу)

• Не используйте альтернативные названия операторов: !, не not. Это C++, не Pascal, не Python. Альтернативные названия были введены как триграфы — для клавиатур, где нет почти ничего, кроме латиницы — не наш случай.
• Не используйте оператор new: используйте умные указатели и операции вроде make_shared.
• Не хватает слов final и noexcept.
• В Makefile для автогенерации зависимостей через gcc ещё хорошо бы давать ключ -MP, чтобы при удалении заголовочного файла не было проблем.
• CLI
  • Практически везде вместо char* надо написать std::string_view.
  • ACTION
    • Лучше Action, это тип
    • Скорее принадлежит к CLI.h, а не к
  • struct Key
    • деталь реализации, должен быть в CLI.cpp
    • Лучше делать не два вида опций, а сразу сделать set<string> из возможных альтернатив. Раз уж там полные синонимы.
  • Вместо compare/equal лучше ==, это не Java, тут принято перегружать операторы.
  • parseNextArg — бесполезная функция, используется ровно один раз и требует вообще все переменные из конструктора CLI. Лучше заинлайнить.
    • find_if — в лямбде лучше сделать захват по ссылке всего [&], чтобы обозначить, что эта лямбда вызывает вот прямо сейчас и немедленно.
• Незачем явно удалять конструктор по умолчанию, если вы уже задали какие-то другие конструкторы.
• IOStatistic
  • Не используйте тернарный оператор вместо if.
  • Шесть приватных функций используются ровно один раз и имеют смысл только в контексте публичных. Никогда без них использоваться не будут. Лишние, заинлайнить.
  • IOStatistic --> IOStatistics
• Huffman
  • Принимайте unique_ptr по значению, а не по rvalue-ссылке.
  • Не возвращайте константную ссылку на unique_ptr, это бесполезно.
  • Странно, что у HuffmanNode есть конструктор с weight = 0.
  • Неясно, зачем вынесена функция isFinish (сильно больше кода, но он теперь не такой локальный).
  • extractMin лучше лямбдой с захватом.
    • А ещё там res не используется.
    • А ещё его можно написать в один иф с не очень сложным условием, а не три. К тому же сейчас четыре return идут на разных уровнях.
• CHECK_EQ(a, b) --> CHECK(a == b). doctest разберётся.
• TestHuffmanTree.cpp
  • Дублирование кода: isLeaf дублирует метод вершины, onlyOne — это просто ^ или != на выбор.
  • Каждый из existingLeafs — это отдельный CHECK. Тут действительно однообразный код, не надо пытаться это замаскировать. Тесты должны быть максимально дубовые.
• TestHuffmanArchiver.cpp
  • Вместо 128 лучше битовый литерал: 0b1'000'0000
  • Название теста должно говорить, что именно тестируется/не работает. В частности, неясно, чем отличаются write bit 1 и write bit 2. Аналогично case1/case2`.
• Лучше не std::abort(), а assert.
• Строковые константы лучше не const char*, а const char[], чтобы можно было спрашивать sizeof, если потребуется.