hw_03

[Jura Khudyakov]

Напоминаю: переписка в Телеграме

[Egor Suvorov]

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

Третья попытка заблокирована из-за несоблюдения формата сдачи.

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

• Не соблюдено стандартное требование: hw_03 должно появиться в папке hw_03, а не bin.
• На пустом файле выдаётся размер сжатых данных (без учёта дополнительных) "1 байт". Алгоритм Хаффмана так работать не может. На рандомном файле тоже бывает: размер "сжатых данных" 10244 байта, а исходно было 10240 (то есть ошибка даже не в один символ!). Подозреваю, что это может быть из-за добавления фиктивных символов и кодов.
  • Вместо специального EOF char лучше просто длину кодируемых данных в начале записать. А то целый ценный символ тратите. Например, когда у вас есть два символа с частотой 10000, вы могли бы закодировать их в 20000 бит. А из-за лишнего символа получаем 30002 бита. Если я не ошибся.
• Есть UB: нельзя начинать глобальные имена (в том числе define) с нижнего подчёркивания: ​https://stackoverflow.com/a/228797/767632

Тесты 5/8

• CLI
  • Многовато случаев. Их не проверить глазами. Лучше разобрать 1-2 крайних случая для покрытия всех веток кода. Опционально можно разобрать 1-2 смешанных сверху, чтобы проверить комбинации. Это уже довольно хорошо. Если интересно, можно почитать теорию тут: ​https://en.wikipedia.org/wiki/All-pairs_testing
  • Тесты IncorrectArgv compress/IncorrectArgv extract вообще ничем не отличаются.
  • На будущее: вы написали "параметризованные тесты", просто их doctest не поддерживает в таком виде из коробки почему-то: ​https://github.com/onqtam/doctest/blob/master/doc/markdown/parameterized-tests.md
• Не тестируются отдельные куски HuffmanArchiver. Что логично: такой God Object протестировать невозможно.
  • Лучше входным файлам тоже добавить расширение, их будет удобнее искать. Например: .in, .out.
  • Тест full cycle вроде бесполезен? Если прошли предыдущие, то этот упасть не может..?
  • Тесты не самодостаточные: им нужны внешние ресурсы в виде файлов. Обычно это очень плохая идея. Например, я так не смог запустить ваши тесты из другой папки. Лучше сделать так, чтобы можно было в несколько строк написать тест "сожми и проверь" (stringstream), а дальше сделать кучу коротких тестов в коде (можно заодно будет добавлять комментарии, почему этот тест важен или в коде показывать, как именно получился такой ожидаемый вывод) плюс, возможно, сколько-то больших автосгенерированных. Сейчас у вас больше похоже на регрессионные тесты для фиксирования поведения - так тоже делают, но обычно с легаси. Такие тесты ловят баги, но их очень сложно читать/поддерживать/обновлять. Например, потому что неясно, что делать, если тест не прошёл - перегенерировать весь файл? Но ведь надо сравнивать побайтово?

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

• Пустые заголовки Test*.hpp не нужны.
• BYTE_SIZE лучше захардкодить в 8 и поставить static_assert(CHAR_BIT == 8) (есть такой макрос).
• Не используйте исключения для обработки ошибок программиста и проверки инвариантов. Это должны быть assert. Смотрю на HuffmanArchiver::pushToBuffer, HuffmanArchiver::getLastSizes.
• CLI - argc не нужен.
• HuffmanArchiver
  • Слишком много чего происходит. "God object". Какие-то подсчёт статистики с построением дерева, запись и чтение таблицы кодов, буферизация, ввод-вывод двухбайтовых чисел в правильном порядке байт, побитовый ввод-вывод - всё в одном классе. Явное дублирование кода (типа "читать до eof").
    • Это всё перпендикулярные вещи. Должны быть в разных классах, чтобы можно было их отдельно тестировать. Как проверить, что деление окей: можно описать всю логику класса одним предложением. Придётся переписать примерно полностью.
  • tuple<int, int, int> - очень плохая идея, лучше структура с именоваными полями. Можно даже её агрегатом оставить.
  • Кажется, это не класс, а пара независимых функций. Ради приличия стоит хотя бы вынести потоки в конструктор.
  • Кажется, что три поля lastOriginalSize, lastProcessedSize, lastHeaderSize на самом деле тесно связаны и инициализируются рядом => это не должно быть три независимых optional.
• Huffman
  • Делать hash<bitset<514>> глобально для всех пользователей Huffman.hpp - плохая идея. Лучше отдельный тип.
  • HuffmanNode
    • HuffmanNode(size_t, size_t, size_t); - что тут что? Добавьте имена параметров.
    • Писать на индексах - ну такое. Например, здесь сразу получается, что нельзя, чтобы внутренняя вершина сама посчитала свой размер от детей.
  • HuffmanTree - снова огромный God Object с как минимум двумя режимами использования. Причём никак не проверяется, что используется правильный режим: могу сначала вызвать buildTreeBackward, а потом buildTreeForward. Тут ещё есть подсчёт частот символов

Стиль 5.5/8

• Все локальные для TU константы (вроде allArgvCompress) следует объявлять в анонимном namespace.
• Не хватает слов final, noexcept, explicit (вроде конструктора у CLI)
• Makefile
  • Лучше не целями переменные модифицировать, а переменными среды. Иначе, имхо, какое-то очень сложное состояние получается. Но тут я не уверен, просто никогда такого не видел. Обычно разбивают на стадию configure/make.
  • Не хватает флага -MP на случай удаления какого-нибудь заголовочного файла.
• CLI
  • using what не нужен, методы и так автоматически будут видны. Это с конструкторами специфика (потому что они всегда новые должны быть).
  • Не надо создавать промежуточную переменную для исключения на все случаи жизни. Пишите в каждом случае throw CLIException("Wrong arguments") и расскажите, почему именно аргументы неверные. Это в любом случае сообщение для человека, а не машиночитаемое.
  • Не надо проверять количество аргументов в начале конструктора. Если их не то количество, в любом случае это ошибка по конкретной причине (не хватает такого-то аргумента/дубликат). Это понятнее пользователю.
  • Вместо цепочки if-continue лучше if-else-if-else-throw
  • Не обрабатывается ошибка "дублирующаяся опция", а просто молча заглушается. Аналогично заглушаются прочие ошибки. Хотя бы assert, заглушать нельзя.
  • Вместо массива INPUT_FILE_OPTIONS лучше сделать set. Будет единообразнее и не надо писать цикл/эмулировать питоновский for-else.
  • Я сначала не заметил enum class Mode, потому что он спрятался в методах. Лучше типы в отдельном блоке указывать.
• Shit --> Stuff. В контексте лучше RandomShit1 --> UniformRandomData (заодно указать, что именно за рандом).
• main
  • Не надо явно закрывать файлы, используйте RAII
  • Вместо цикла for лучше конструктор vector(first, last)
  • Слишком плотный кусок кода в main, надо добавить пустых строк между логическими блоками.
  • Structured binding на три инта - это жесть. Невозможно проверить, что он правильно написан. Смотри замечание по архитектуре.
• К сожалению, детально прочитать HuffmanArchiver и Huffman я не смог: они слишком много чего делают, я не могу удержать это в голове. А по общему стилю (не по деталям реализации) там претензий почти нет.
• HuffmanArchiver
  • inttypes.h --> cinttypes+std::. И точно нужен он, а не cstddef?
• Huffman
  • // in fact, 2*n-1, but bitset know no difference - что?
  • Типы вроде bitStorage/codeType следут начинать с большой буквы, иначе путается с именами переменных.
  • Названия forward/backward непонятные. "Из частот символов" - более понятное.