HW #3 (Huffman)

[tarasov.denis]

Домашечка

[Egor Suvorov]

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

Весьма приятно читать в среднем, спасибо.

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

• Не -e, а -u.
• UB: [в любых идентификаторах](​https://stackoverflow.com/a/228797/767632) запрещены двойные подчёркивания, включая дефайны и include guards.

Тесты 5/8: случаи хороши, но код надо сильно улучшить.

• Совершенно лишние try/catch. Если вылетит исключение, doctest его сам поймает и красиво упадёт, а чтобы ожидать исключение, есть специальные команды.
• Вы дублируете код, который записывает/читает FILE_SIGNATURE. Налажаете в одном месте — скопируете в тесты, не заметите подвох. В том же Compress empty лучше полностью сравнить всё содержимое stringstream с фиксированным массивом констант. Да, при изменении сигнатуры изменятся тесты, но ведь и формат тоже полностью поменяется. Что как раз повод взглянуть на тесты.
• Аналогично, лучше не читать из stringstream, а взять str и сравнить за одну операцию с известным массивом.
• Текст what() нужен для людей, а не для автопроверок. Автопроверки должны смотреть на коды ошибок или, что лучше, на конкретные типы исключений. В Extract invalid empty явно хочется проверить тип исключения.
• Вместо fail() лучше проверять у потока просто конвертацию к булу
• В тестах не хватает константности у всяких size
• Конструктор stringstream и так пустой.
• Не хватает тестов на разбор параметров.

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

• Исключения явно хочется разделить на несколько классов для автотестов.
• Очень нехорошо завязываться на size_t в формате. Равно как и на endianness.
• CLI незачем делать классом. Всё равно есть ровно один вариант использования: создать от argc, argv, вызвать run(). Это свободная функций. При этом всё ещё хочется протестировать разбор параметров.
• Плохо, что main всегда завершается успешно.
• buildCodes лучше возвращать map, иначе непонятна семантика, если в codes уже что-то есть.
• HuffmanTree::getChar() похоже на итератор или чтение из потока. Это точно не стоит делать через std::function<> — это лишние виртуальные вызовов и динамического выделения памяти (одно на вызов getChar). Хотя бы шаблонный функтор тут передавать.
• Кажется, что HuffmanNode::symbol_ имеет смысл только если !leftChild_ && !rightChild_. Чтобы это гарантировать, следует сделать это поле приватным и сделать геттер с ассёртом. И сделать проверку этого условия, оно много где возникает. Аналогично с count: оно не должно меняться.
• У вас unique_ptr только в вершинах, но по факту есть куча владеющих чистых указателей.
• При этом у recursiveWalk первый параметр должен быть чистым указателем: ему глубоко всё равно на способ владения вершиной.
• Не basic_istream<char>, а istream.
• unique_ptr<char[]> — vector<char>, пожалуйста. Заодно исчезнет стрёмный параметр uniqueElements по ссылке.
• stats_ — это не состояние HuffmanArchiver, а результат последней операции. Пусть его лучше операция и возвращает.
• В countFrequencies() и вообще лучше [мыть тарелки перед едой](​http://blog.algoprog.ru/init-not-clear/) (я про seekg)
• writeCompressedData переплетает битовый ввод-вывод с логикой Хаффмана. Это надо тестировать отдельно по-хорошему. Я бы сделал отдельную штуковину для битовых манипуляций (см. замечание выше про std::function).
• checkInFile()/checkOutFile() прекрасно заменяются на встроенные в iostream исключения.

Стиль 4/8

• Уберите C-style cast.
• Слишком много где пишете huffman::. Лучше просто на весь .cpp открыть namespace, всё равно вы реализуете функции из него. Или сделать using huffman::HuffmanException хотя бы.
• CLI::CLI: довольно недружелюбный к пользователю разбор параметров. Вы очень жёстко завязались на количество аргументов. Пользователь не хочет считать аргументы (и программист тоже), они хотят добавлять недостающие или убирать лишние. Число 5 встречается дважды и оба раза это магическая константа, которую код может сам прекрасно вывести. Лучше пройтись по всем аргументам.
• Непонятно, о чём говорит ошибка HuffmanException::CLI_MODE. Судя по коду — что режим не задан. В названии константы этого нет.
• NONEXISTENT_FILE — не совсем, на файл может не быть прав. Скорее "файл не удалось открыть".
• В CLI::run() не стоит assert, что mode_ корректен. И на прочие инварианты CLI.
• inf/of — несимметрично. Лучше input/output, не экономьте символы.
• exceptType — тип, с большой буквы, пожалуйста.
• HuffmanException::what() заглушает ошибки.
• Класс HuffmanTree, тесты HuffmanTree, а файл Huffman.h — несимметрично
• Конструктор HuffmanNode должен принимать unique_ptr — он же принимает владение детьми.
• Конструктор HuffmanException можно noexcept.
• leftChild_/rightChild_ можно просто left_/right_
• recursiveWalk — кажется, что оно не просто обходит, а с конкретной целью. Судя по параметру codes. И, наверное, currentCode можно не копировать каждый раз.
• vector<HuffmanNode*> nodes должен владеть своими нодами, судя по всему. unique_ptr, пожалуйста. Или по значению храните. В лямбду можно передавать unique_ptr по константной ссылке.
• По map table лучше итерироваться с const auto&, а не auto
• Huffman.cpp:32 — не хватает пустой строчки между соседними ифами.
• Huffman.cpp:52 — уберите запятые. Напишите нормальные фигурные скобки и ;
• Huffman.cpp:57 — очень странное место. Вы иногда можете создать вершину с одним ребёнком?
• a != nullptr ==> a
• Huffman.cpp:77 — это вроде только в одном случае бывает. Хочу assert.
• Huffman.cpp:111 — лучше перевернуть if. Сначала разобрать 0, потом 1.
• writeHeader/readHeader руками считают количество записанных/прочитанных байт. Пусть это происходит автоматически.

[Egor Suvorov]

P.S. А ещё напоминаю про final классы и noexcept методы в целом

[Egor Suvorov]

P.P.S. Код непортируемый: нужен бинарный режим файлов.

[tarasov.denis]

CLI по условию отдельный класс.

[Egor Suvorov]

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

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

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

Тесты 7/8

• Когда тесты только процедурные (т.е. вход генерируется автоматически), их сложно проверять. uint16 writing точно можно сделать не до A до Z, а руками сделать сколько-то вызовов и проверить результат. Будет проще и понятнее. Аналогично во всех тестах. Иначе приходиться понимать, а что же именно генерируют тесты.
• Например, я не могу быстро прочитать тест Bit writing manips tests и понять, что же такое делают BitWriter и BitReader и как вообще ими пользоваться.
• В Two characters лучше захаркодить, какой символ слева, а какой справа. А ещё лучше прямо битовые последовательности проверить — вы ими явно пользуетесь. В Three characters тоже. Да и в остальных небольших тестах (hello world на грани) — не предполагается, что поведение алгоритма меняется в процессе разработки, лучше его зафиксировать. Плюс вы сейчас никак не проверяете префиксность кодов, а так как раз проверите.

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

• const_cast яростно намекает, что вы делаете что-то не то. Вы вызываете не-legacy код из не-legacy кода, а const_cast всё равно нужен и не по делу.
• Приватные статические константы вроде MIN_ARGS_COUNT — детали реализации, должны жить в .cpp. Плюс они вообще не нужны: если аргументов мало, то лучше явно говорить "не хватает такого-то аргумента" (это у вас и так есть), чем "чот мало аргументов".
• Не используйте eof() для проверки конца файла. Есть куча способов получить ошибку чтения, вы разбираете только один случай.

Стиль 5.5/8 (в основном срезано из-за кривых отступов)

• Не используйте C-style cast (TestTree.cpp, например)
• buildCodes() — не заглушайте ошибки
• Не заглушайте ошибки в HuffmanArchiverException::what(): type_ всегда должен быть чему-то равен! assert
• Почти все сложные конструкторы вроде CLI можно сделать explicit (с появлением list-initialization это осмысленно даже для конструкторов от не одного параметра).
• У namespace huffman { на весь файл лучше отступ не делать, будет проще читать. Добавьте только } // namespace huffman на закрывающей скобке.
• vector<bool> лучше сравнивать не через .size() == 1 && [0] == 1, а через == vector<bool>{0, 1, 0, 1}, будет понятнее.
• Вместо << и >> для побайтового ввода-вывода следует использовать неформатированный ввод-вывод: get/put/read/write.
• Возможно, вместо char везде будет лучше сделать uint8_t, чтобы не надо было беспокоиться про потенциальную знаковость.
• TestTree.cpp:70: что за число 128? Лучше битовый литерал 0b1000'0000. Аналогично ниже. А ' можно разделять символы.
• char A = X; CHECK(A == Y) лучше записать как CHECK(X == Y);
• Вместо std::string() + static_cast<char> лучше записать '\0', а ещё лучше — массив из четырёх элементов и делать == с ним. Можно в крайнем случае 7сделать string(arr, arr + 4).
• Не используйте rand(), используйте <random> из C++.
• Лучше не is_open, а просто operator bool.
• В CLI.h поехали отступы.
• В заголовках левые инклуды (можете посмотреть на тулзу IWYU):
  • CLI.h не нужны включения ни HuffmanArchiver.h, ни fstream.
  • Инклуды должны быть целиком внутри гардов (привет от HuffmanExceptions.h)
  • Streammanips.h хватит iosfwd вместо iostream
  • HuffmanArchiver.h не хватает map
• HuffmanTree.cpp:
  • :39: const auto& дял оптимизации
  • Поехавшие отступы в строчках 56-57
  • Вы стараетесь выровнять аргументы строго правее имени метода, что плохо работает с длинными именами методов. Просто сделайте перенос строки сразу после (, отступ и спокойно пишите имена параметров.
• counter --> chars_read, плюс это цикл for.
• Странный формат: зачем сначала писать все символы, а потом частоты? Логичнее, мне кажется, если записывать сразу пары (символ, частота) друг за другом. Удобнее отлаживать бинарный формат.