#HW_03

[Egor Suvorov]

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

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

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

• Фатальное: есть UB: компаратор внутри HuffmanTree не является антирефлексивным.
• Не поддерживаются длинные версии флагов.
• При ошибке код возврата должен быть ненулевым.
• Не стоит завязываться на порядок байт внутри uint32_t, он разный на разных машинах.

Тесты 7/8

• Названия тестов Test1/Test2 ничего не говорят о том, чем эти тесты принципиально отличаются и что делать, если один из них упал.
• В тесте BitRead лучше не дублировать его код, а честно проверять, какие биты считались.
• А ещё, кажется, можно вынести из end-to-end тестов кучу общего кода.

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

• Не сошлись имена файлов в папках test, src, include — несимметрично.
• Не надо кодировать битовые последовательности строчками: это убивает проверку типов. И надо пихать assert'ы на символы. Лучше vector<bool> или что-нибудь похожее. Куда нельзя запихать ничего, кроме битовой последовательности.
• BitBuffer
  • Этим трём классам незачем быть полиморфными: вы всегда используете либо запись, либо чтение.
  • И непонятно, зачем выносить общие поля в общий класс.
  • Несимметрично, что читаем мы побитово, а пишем сразу пачками, причём фиксированными заранее. Лишняя связь между BitBufferWrite и Хаффманом. И тесты становятся сильно сложнее.
  • В деструкторе записывальщика хорошо бы проверить, что буфер пустой. Иначе явно ошибка программиста.
  • Раз уж есть namespace BitBuffer, то пусть классы будут Writer/Reader. Иначе дублирование получается.
• CLI
  • STREAMMODE — явно какая-то деталь реализации, зря торчит в CLI.h
  • Странно, что у CLI ровно один сценарий использования: сначала коснтруктор, потом ровно один раз run() (если больше раз, то неясно, что будет). Хорошо бы включить run() в конструктор. А потом, кажется, обнаружится, что Params и CLI можно не разделять. Ну или выкинуть класс CLI и сделать его одной функцией, оставить только CLI.
  • isFlag — тоже явная деталь реализации, незачем светиться в CLI.h
  • CLIException можно упростить при помощи using конструктора.
  • Не int32_t, а int всё-таки по умолчанию.
  • У вас CLI подогнан под main, а не под нормальный C++-интерфейс. Костыли в тестах на это намекают.
• Huffman
  • uint32_t для размера файлов и частот символов — очень мало.
  • Не должно быть чистых указателей в конструкторе, у которых вы берёте владение. Принимайте сразу unique_ptr, тогда семантика владения будет ясна.
  • HuffmanNode: странно, что один конструктор и для листьев, и для внутренних вершин. Они вообще по-разному себя ведут.
• HuffmanArchiver
  • Тип FrequencyTable и две связанных с ним функции намекают, что, возможно, это всё-таки отдельный класс.
  • Нехорошо, что метод readFrequencyTable несимметричен к writeFrequencyTable: ему требуется какая-то дополнительная информация. Пусть будут полностью взаимно обратны.
  • А ещё, кажется, getFrequencies тоже переедет в этот класс в каком-то виде.

Стиль 5/8

• Makefile: все-таки надо вынести компилятор и флаги компиляции/линковки в отдельные переменные
• namespace BitBuffer{ — пробел перед {
• BitBuffer
  • Очень зря принимаете коды по неконстантной ссылке. Можно случайно поменять. А раз уж потом всё равно копируете, то лучше принимать по значению и делать move. Тогда в случае, когда исходно было rvalue, не будет копий.
  • В тестах лучше не десятичные литералы, а двоичные: 0b1'000'00'00.
  • Лучше поставить static_assert(CHAR_BIT == 8), всё равно используете uint8_t везде.
  • Записывать uint8_t лучше не через write, а через put.
• CLI
  • Имена enum'ов — StreamMode, CamelCase.
  • createArgs можно заинлайнить в конструктор.
  • Скобки в return не нужны.
  • Лучше не цепочка if с continue, а цепочка else if и throw в конце в последнем else.
  • Незачем явно вызывать конструктор string{args[++i]}
  • Неясно, зачем Param делать полем, раз вы его всегда муваете в run(). Звучит как локальная переменная для run(). А тогда уж и getStreamParam может быть не методом, а просто храним ссылку/указатель на поле в мэпе.
• main
  • const_cast нужен для снятия констатности. Навешивать её лучше при помощи static_cast.
  • ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ТОЛЬКО ТАМ, ГДЕ НУЖНО (смотрю на sizes)
  • Все три обработки исключений у вас одинаковые, и все наследуются от std::exception. Можно сократить.
  • Не сбрасывайте буфер после каждой строчки (endl).
• Huffman
  • getLeave --> getLeaf, аналогично с isLeave. Это единственное число слова leaf.
  • getLeave лучше инвертировать if и сделать его симметричным: if (getBit() == 0) foo else bar. Биты-то симметричны.
  • Компаратор лучше сделать как сравнения пар/таплов, а не руками. Или лучше пишите в таком шаблоне: if (a.x != b.x) return a.x < b.x; if (a.y != b.y) return a.y < b.y; return false. Сейчас запутались, получили UB и минус кучу баллов.
  • unique_number не используется.
  • В root = std::unique_ptr явный вызов конструктора не нужен.
• HuffmanArchiver
  • statistics с большой буквы.
  • ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ТОЛЬКО ТАМ, ГДЕ НУЖНО (смотрю на symbol)
  • И снова не читайте при помощи reinterpret_cast один байт, лучше get.
  • Очень нетривиально, что getFrequencies читает из файла sizes.sizeOfExtraData. Я бы ожидал, что statistics — вообще пассивное поле, в которое только добавляют информацию.
  • Не amount, а number (первое для неисчисляемых, второе для исчисляемых).
  • compress/extract — очень длинный какой-то.
    • Лишний this->
    • Лишняя переменная codes
    • Плохо читается, потому что стена текста. Стоит добавить пустых строк между логическими блоками.
  • Тесты
    • Не Otherwise, а Non-empty table.
    • ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ТОЛЬКО ТАМ, ГДЕ НУЖНО (смотрю на CompressStats).

[samoylov.viktor]

Дедлайн третьей попытки 04.05 22:59(см. переписку в телеграмме)

[samoylov.viktor]

Вроде всё исправил, кроме static_assert(CHAR_BIT == 8)(не понял, где он должен быть) и явного вызова конструктора у unique_ptr(без него никак). Также unique_number всё-таки используется, чтобы у внутренних вершин не было одинаковых символов(см.переписку в телеграмме).