HW #3

[Egor Suvorov]

Первая и последняя посылка — по акции, с весом 0.5. Проверялась ревизия 4118. После неё ещё были 4119 (23:00, после дедлайна) и 4177 (30 апреля, после дедлайна).

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

• Не компилируется под gcc: вы используете hash<basic_string<unsigned char>> — это что-то нестандартное.
• Не компилируется под clang: не хватает #include <cassert>, #include <climits> кое-где (в последних ревизиях почти всё поправлено)
• Из исправленного позднее:
  • Вы конвертируете строковой литерал в const char*, это запрещено.
  • Линкуете тесты вместе с hw_03.
• На ваших собственных тестах кучи обращений к неициализированным полям, ловится Valgrind'ом.
  • В новых версия появляются ещё и утечки. Ещё, например, иногда конструктор HuffmanTree вызывается от маленького вектора.
• Виснет (или очень медленно работает) на пятимегабайтном файле (в новых версиях получше)
• Статистика считается неверно: не сошлось с размером файла. На пустом файле получилось, что размер сжатых данных — 4-5 байт ( вз ависимости от версии), а не 0, как должно быть (это всё без учёта дополнительных данных).
• Дико неэффективное сжатие. Размер каждого кода округляется до байт.
• В новых версиях:
  • Неверно сжимается/разжимается файл размера 256, состоящий из всех байт по очереди. На нём же неверно показывается размер входного файла. Ещё на нём реальный сжатый размер получается ~5000, хотя должен быть не больше 256.
  • Также не получается сжать-разжать файл со строчкой Фибоначчи и ещё много каких. В старых версиях просто не протестировал.
• Не учтено, что порядок байт может быть разным на разных системах.

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

• Зачем-то для хранения битовых строк используются строчки с символами '0' и '1', это путает и убивает типобезопасность. А ещё зачем-то используются в этом же месте ustring, что совсем не по делу, хватило бы string.
• HuffmanNode
  • unique_ptr вместо чистых указателей решит сразу кучу проблем. С утечками и правилом пяти/нуля, скажем.
  • Вместо mergreWithOther лучше конструктор сделать. Или сделать это свободной функцией. А то несимметрично.
  • Странное разделение между публичными/приватными полями.
  • NodeCompare — точно какая-то деталь реализации, должна жить в .cpp
  • Непонятно, зачем в каждой вершине хранить множество всех символов, которые из неё достижимы.
• CLI
  • Все константы — детали реализации, должны жить либо как приватные статические члены, либо (что сильно лучше) внутри CLI.cpp в namespace {}
  • Аналогично с checkAndThrowWrongArgument — это функция в конкретном .cpp. Но лучше её выкинуть и честно рассказывать пользователю, что пошло не так.
  • Не надо использовать сишные строки, лучше string_view, span, vector...
• HuffmanException лучше наследовать от runtime_error/logic_error, заодно реализация станет сильно лучше.
• В Statistics = default() бесполезен: всё равно не будут проинициализированы поля.
• HuffmanArchiver
  • Кажется более разумным хотя бы вынести istream/ostream в параметры конструктора класса, иначе класс совсем бесполезный получается.
  • compressCode должна возвращать значение, а не изменять аргумент. Это точнее соответствует семантике и точно не замедлит, вы и так копируете.
  • Странно, что потребовались отдельные getFileSize, когда можно и так посчитать статистику, честным образом. Например, написав свою обёртку вокруг istream/ostream, которая ещё и статистику считает.

Тесты 0.5/8

• Не протестировано практически ничего. Три теста, запускающие конструкторы, без каких-либо проверок.

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

• НЕ ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ВНЕ ЦИКЛОВ, смотрю на unsigned char tmp.
• Huffman
  • Чистые указатели, чистый new — FATALITY.
  • Если уж принимате параметр по rvalue-ссылке, то делайте потом из него move.
  • Промежуточная переменная newNode не нужна
  • getCodesTable() - случай с пустым деревом лучше целиком разобрать отдельно. Иначе неясно, почему при пустом дереве мы всё равно запускаем traverseTree.
• HuffmanArchiver
  • Конструктор не нужен: static_assert можно писать просто вне функций.
  • extract/compress какие-то сложные, я не смог их быстро прочитать и понять логику.
• COMPILE_FLAGS --> CXXFLAGS
• Не .PHONY = all clean, а .PHONY: all clean
• CLI
  • bool modeSet_ --> optional<ArchiverMode> mode_
  • Если аргументов неверное количество, то всегда можно рассказать пользователю, что именно с ними не так: дубликат, чего-то не хватает... Лучше так, чем подгонять константу.
  • А цикл потом точно стоит делать до argc, чтобы обработать все аргументы.
  • После использования string_view сразу можно сделать ==. Кстати, константы тут явно мешаются, они ровно один раз встречаются.
• ustring character — очень противоречивое название. Вероятно, ustring code?

[Egor Suvorov]

P.S. Ещё неймспейсы, конечно, тоже нужны.

[Egor Suvorov]

P.P.S. Забыл в табличку поставить с весом 0.5, обновил.