HW #3

[Egor Suvorov]

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

[Alexander Morozov]

Писал в телеграме, заслал код полностью до дедлайна и забыл про тикет

[Egor Suvorov]

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

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

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

• Не выводится статистика на экран — это минус третья попытка в сочетании с остальным.
• Тесты должны быть в файлах .cpp, они компилируются отдельно от TestMain.cpp и линкуются.
• HuffmanArchiver.cpp не включает <array> и у меня не компилируется (clang++-10 + libc++).
• Из-за using namespace Huffman; и одинакового имени класса и namespace получаем ошибку компиляции под тем же clang. Лучше было бы просто обернуть весь .cpp в namespace Huffman {}

     osboxes@osboxes:~/cpp2019/cpp19/morozov.aleksandr/hw_03$ make
  clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o obj/HuffmanArchiver.o src/HuffmanArchiver.cpp
  clang++-10 -O2 -Wall -Werror -std=c++17 -Iinclude -stdlib=libc++ -c -MMD -o obj/Huffman.o src/Huffman.cpp
  src/Huffman.cpp:40:9: error: reference to 'Huffman' is ambiguous
  CodeMap Huffman::Huffman::construct(std::map<char, freq_t> freqs) {
          ^
  include/Huffman.h:12:11: note: candidate found by name lookup is 'Huffman'
  namespace Huffman {
            ^
  include/Huffman.h:32:7: note: candidate found by name lookup is 'Huffman::Huffman'
  class Huffman {
        ^
  1 error generated.
  Makefile:26: recipe for target 'obj/Huffman.o' failed
  

Тесты 4/8

• Не протестирован битовый ввод-вывод.
• Не протестирован парсинг флагов.
• Не протестировано сохранение/чтение CodeMap а также всяких очень хитрых таблиц (смотрю на код extract и ужасаюсь).
• Файлы тестов очень плохо согласованы
• Классно используются SUBCASE. Спасибо, не знал, что так можно.

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

• Не хватает слов final и noexcept
• Не хватает namespaces. Особенно в Huffman.h, где объявляется, скажем, LetterCode — это точно хаффманоспецифично.
  • freq_t — зарезервированное POSIX имя. Как и любое имя, заканчивающееся на _t: ​https://stackoverflow.com/a/228797/767632
• bit_in_buf
  • operator bool тут должен быть explicit, чтобы нельзя было написать bit_in_buf(...) + 10 и получить 10 или 11.
• bit_out_buf
  • Странно, что всегда производится запись в поток вывода. Если хочется "сбросить буфер", то лучше это делать не в деструкторе, а сделать явный метод. А в деструкторе проверять, что поток сброшен. Так при использовании будет явнее видно, где именно появились "лишние" нулевые биты.
• Исключения лучше наследовать от logic_error/runtime_error и делать using конструкторов. Будет короче и проще.
• CLI
  • Класс бесполезен, хоть и требуется по заданию. Я бы скорее объединил его с Flags по такому поводу. И в конструкторе инициализировал поля input/output и остальные.
  • Заодно не потребовались бы optional — в любом корректном Flags они есть.
• Huffman
  • shared_ptr для вершин абсолютно не по делу. По факту вершиной единолично владеет родитель.
  • Конструктор внутренней вершины может сам посчитать частоту. Лучше так и сделать.
  • merge_nodes очень похож на конструктор внутренней вершины. Кажется, они друг без друга не могут, лучше склеить.
  • _digit хранить не надо, это выводится в процессе работы и код сложнее не становится. А так инвариант усложняется.
• HuffmanTree/Huffman/HuffmanArchiver имеют только статические функции. Это не класс, это namespace в лучшем случае. Лучше так и назвать.
• HuffmanArchiver
  • Кажется, что read_map/write_map — это методы объекта CodeMap всё-таки. Просто судя по параметрам.
• Есть ограничение на 32-битную длину файлов. Это мало.

Стиль 5/8

• НЕ ИСПОЛЬЗУЙТЕ C-STYLE CAST (смотрю на (char)rnd()).
• НЕ ИСПОЛЬЗУЙТЕ АЛЬТЕРНАТИВНЫЕ НАЗВАНИЯ ОПЕРАТОРОВ (смотрю на not)
• В Makefile в автогенерации зависимостей не хватает опции -MP, чтобы корректно обрабатывалось удаление .h-файлов
• За using std::shared_ptr в заголовках просят так не делать. Это действует на все файлы, которые включают этот заголовок.
• Неконсистентное именование файлов: то bin_manip, то HuffmanArchiver
• Побайтовый ввод-вывод гораздо лучше делать через неформатированный ввод (get/put), а не operator>>.
• bin_manip
  • friend operator>> лучше объявлять в секции public. Он не бывает непубличный.
  • Вместо member initializer list для _cnt и _val лучше использовать инициализацию поля по умолчанию.
  • Можно обойтись без флага _write_happened, если сделать другую инициализацию/инвариант.
• CLI
  • FlagParseException — в конструкторе лишние копирования.
  • Не нужен явный std::string при присваивании
  • optional<bool> и конструкция a = false путает. Я тупил над строчками 15-17 в CLI.cpp минуту. Лучше написать явно a.emplace(false).
• uint64_t наверняка надо брать из std:: и какого-то стандартного заголовка, Huffman.h этого не делает.
• Huffman
  • collect_codes принимает вершину по умному указателю почём зря. Хватит ссылки.
  • Вместо merge лучше создать один CodeMap и по ссылке в него везде добавлять. Всё равно скрывать в приватном интерфейсе.
  • В тестах переменная res не нужна. А s лучше назвать как-нибудь попонятнее.
• HuffmanArchiver
  • Кажется, в write_map можно заиспользовать побитовый поток.
  • const auto&
  • code_vec не нужен?
  • Разбросаны read_map/write_map по файлу. Должны идти рядом, как в .h. И снова можно побитовый поток..
  • Код идёт плотной стеной во всех функциях. Не видны логические куски, сложно читать, нельзя прочитать только кусочек функции.
  • Особенно этим грешит extract. Тут точно можно вынести отдельно чтение всяких хитрых таблиц и декодирование.