HW #3

[Egor Suvorov]

Первая и последняя посылка — по акции.

[Egor Suvorov]

А ещё, как я понимаю, это всё-таки Хаффман.

[Egor Suvorov]

Первая и последняя посылка — по акции, с весом 0.5. Проверялась единственная ревизия 4055.

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

• test_hw_03 должен лежать рядом с hw_03, а не в подпроекте
• На ваших же тестах Valgrind ловит кучу неинициализированных чтений (это UB и FATALITY) в binary io end-to-end/small case, big numbers.
• Не работает под Windows: файлы надо открывать в бинарном режиме.
• Падает на пустом тесте.
• __CHAR_BIT__ --> CHAR_BIT, иначе это что-то внутреннее конкретного компилятора.
• Не работает с файлом с именем, например --file.
• Не работает с файлами размером больше 4 ГБ (потому что размер файла в uint32_t не хранят).

Тесты 5/8

• Тесты должны быть реализованы не в TestMain.cpp, а в отдельных файлах. Это сильно усложняет чтение.
• Не проверен парсинг аргументов командной строки вообще никак.
• Не проверяется посчитанная статистика.

Архитектура 0.5/5 — идеи правильные, но очень перегружено и намешано

• Лучше поставить один раз static_assert(CHAR_BITS == 8) и дальше честно хардкодить число 8. Всё равно наверняка не получится работать, если в байте не 8 бит. Например, вы всё выражаете через write(uint8_t)/read(uint8_t), это не сработает, если в байте, скажем, 16 бит. Или вот в read<uint8_t> вы сравниваете не с 8 битами, а с CHAR_BIT.
• Странно, что битовые "потоки" жёстко завязаны на вектор, а не поддерживают произвольные итераторы.
• Не стоит помечать noexcept функции, которые могут веруть что-то сложное по значению. Им надо вызвать конструктор копирования, а он может кинуть.
• Bytes
  • Неясно, зачем подключать Archiver.h.
  • Нехорошо, что write шаблонная, хотя на самом деле у неё ровно три вида. Лучше три перегрузки, будет точнее выражаться намерение.
  • writeBytes должен принимать const Bytes&
  • Нехорошо выносить поля в общий базовый класс BitsStream, если всю логику всё равно реализует каждый наследник отдельно. Это поля, подозреваю, имеют совершенно разный смысл в Istream/Ostream. Плюс в каждом из наследников дополнительно возникают какие-то _bitsFilled/_bitsLeft/_firstBatch.
  • Странный flush(). Особенно в сочетании с тем, что указатель приходится всё равно руками двигать.
  • Странно, что вы выразили чтение "не больше 8 бит", а потом всё остальное. Получилось две сложные функции. В итоге код разбора на биты перемешан с кодом считывания из итератора. Лучше было бы сделать чтение одного байта, потом чтение одного бита и потом чтение N бит, получилась бы две простых функции и одна посложнее.
  • Аналогичная претензия к записи: "могу записать не больше 8 бит" — какое-то довольно произвольное ограничение, плюс код усложняет.
  • Очень много где вам требуется явно указывать шаблонный параметр — это плохо.
• CLI
  • triggers() возвращает строчку, которая используется ровно для одной цели: сразу сделать split() (что само по себе нетривиальная функция, которой хорошо бы принимать string_view, кстати). Причём возвращаемая строчка обычно захардкожена на этапе компиляции. Это очень, очень плохо: ни типы не подсказывают о происходящем, ни упрощения кода нет. Пусть лучше возвращает vector<string_view>.
  • addFlagHandler(flag) может казаться, что можно написать addFlagHandler(FlagHandler), хотя это не так. Понятно, почему: вы не берёте владение параметром. Но это странно. Было бы гораздо удобнее не заводить отдельные переменные к классу CLI. Пусть лучше принимает, скажем, unique_ptr<FlagHandler> и вызывается как make_unique<>.
  • char **argv — плохая идея, прибивает гвоздями устаревший сишный интерфейс из указателей. Лучше сконвертировать типы в, скажем, vector<string_view>/span<string_view> и работать с ним. Заодно можно будет избавиться от костыльного int flagInd и передавать, скажем, сразу суффикс аргументов. И возвращать суффикс аргументов - без обработанного.
  • Типы flagHandlersMap/flagHandlersList (второй, кстати, ещё и не list) — явно детали реализации, не надо добавлять эти using. А если и добавлять, то называть консистентно с типами вроде FlagHandlers, с большой буквы.
  • logic_error — это для ошибок программиста. А "флагов не хватило" — это ошибка пользователя. И вообще для ошибок парсинга, кажется, лучше отдельный класс завести.
• Huffman
  • NodesComparator — точно деталь реализации, нужна только в .cpp
  • У Node слишком большой интерфейс, слишком много всего хранит. Например, _terminal явно вычисляется, равно как и _code.
  • При этом у самого HuffmanTree мы почти ничего сделать не можем. Только ссылку на корень взять. Это довольно странно.
  • followingNode должен принимать bool. Откуда uint8_t — непонятно.
  • fillByteCodes — какая-то сложная штука, которую сложно вызывать и начинать надо с определённого вида параметров => деталь реализации, приватный метод.
  • Конструктор HuffmanTree модицифирует свой параметр. Это очень странно выглядит.
  • На этом фоне getTotalBitsCount() смотрится безумно странно. Это какая-то случайная статистика про дерево.
  • Неясен смысл обёртки ByteCode.
• *Archiver
  • Лишний ExtractedBytes и рядом.
  • DummyArchiver нигде не используется => неясно, зачем тут полиморфизм.
  • Неясно, зачем публичный setAdditionalDataSize().
  • Из-за прибития гвоздями массивов в качестве способа ввода-вывода вы не только потребляете линейное количество памяти, но и обязаны очень, очень аккуратно посчитать нужный размер массива. Выглядит жутко.

Стиль 4/8

• Не x.good(), а просто x
• Bytes
  • В одном месте параметр зовётся bits, в другом — count
  • inline для шаблонных функций сам поставится
  • Странно, что в BitsIstream потребовался и _bufferEnd, и _bitsLeft, и `_first
  • В read стоит поставить static_assert, что переданный тип — целочисленный и беззнаковый. Иначе будет переполнение знакового типа, что UB. Или неверное чтение вещественных типов.
  • Количество байт также стоит проверять на < 0.
  • static_cast<size_t>(count) - плохо, тут этот каст не по делу и заглушает предупреждение. Возможно, лучше принимать size_t count сразу.
  • Буфер лучше сбрасывать сразу после того, как дописали последний бит, а не перед следующей записью.
  • По vector<bool> тоже лучше итерироваться через for (const auto & : ...).
  • a << (b) — скобки не нужны (write<uint8_t>)
  • Много промежуточных переменных вроде cur/continuingBits/shift/cnt и скобок вокруг. Из-за этого сложно читать. Кажется, можно половину выкинуть и сократить код.
  • readBytes — лишнее условие в while. Почему-то eof() проверяется в двух местах. Должно хватать одного.
• CLI
  • Третий параметр у FlagHandler не читается. Лучше подсказку: , /*required=*/true.
  • DataFlagHandler --> FlagWithArgumentHandler?`, возможно..?
  • required() --> isRequired() для симметрии с hasFlag().
  • std::string _data = "" — лучше просто std::string _data;, конструктор по умолчанию.
• НЕ ИСПОЛЬЗУЙТЕ rand(), это не потокобезопасно как минимум. Используйте плюсовый рандом.
• HuffmanArchiver
  • Довольно длинные методы, сложно читать. Хотя бы добавьте пустые строчки между.
  • Зачем-то указываете шаблонный параметр при записи вектора булов. И ещё указываете его как const vector<bool>&, а не хотя бы vector<bool>.
• main
  • Очень много кода подряд и переменных. Хотя бы выделили функции чтения из файла (раз уж вы всё равно читаете файл целиком) или пустых строк добавили.