HW #3

[Egor Suvorov]

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

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

• Проблемы с компиляцией

     osboxes@osboxes:~/cpp2019/cpp19/filippov.denis/hw_03$ make
  mkdir obj
  clang++-10 -Wall -Werror -std=c++17 -Iinclude -O2 -stdlib=libc++ -c -MMD -o obj/HuffmanArchiver.o src/HuffmanArchiver.cpp
  In file included from src/HuffmanArchiver.cpp:1:
  In file included from include/HuffmanArchiver.h:3:
  include/Huffman.h:56:50: error: implicit instantiation of undefined template 'std::__1::array<unsigned long, 256>'
    std::array<uint64_t, SIZE_OF_HUFFMAN_ALPHABET> _freqs;
                                                   ^
  /usr/lib/llvm-10/bin/../include/c++/v1/__tuple:219:64: note: template is declared here
  template <class _Tp, size_t _Size> struct _LIBCPP_TEMPLATE_VIS array;
                                                                 ^
  src/HuffmanArchiver.cpp:29:72: error: implicit instantiation of undefined template 'std::__1::array<std::__1::vector<bool, std::__1::allocator<bool> >, 256>'
    std::array<std::vector<bool>, HuffmanTree::SIZE_OF_HUFFMAN_ALPHABET> codes;
                                                                         ^
  /usr/lib/llvm-10/bin/../include/c++/v1/__tuple:219:64: note: template is declared here
  template <class _Tp, size_t _Size> struct _LIBCPP_TEMPLATE_VIS array;
                                                                 ^
  src/HuffmanArchiver.cpp:35:59: error: implicit instantiation of undefined template 'std::__1::array<unsigned long, 256>'
    _outputStream.write(reinterpret_cast<const char *>(freqs.data()),
                                                            ^
  /usr/lib/llvm-10/bin/../include/c++/v1/__tuple:219:64: note: template is declared here
  template <class _Tp, size_t _Size> struct _LIBCPP_TEMPLATE_VIS array;
                                                                 ^
  src/HuffmanArchiver.cpp:35:60: error: expected ')'
    _outputStream.write(reinterpret_cast<const char *>(freqs.data()),
                                                             ^
  src/HuffmanArchiver.cpp:35:53: note: to match this '('
    _outputStream.write(reinterpret_cast<const char *>(freqs.data()),
                                                      ^
  src/HuffmanArchiver.cpp:53:63: error: implicit instantiation of undefined template 'std::__1::array<unsigned long, 256>'
    std::array<uint64_t, HuffmanTree::SIZE_OF_HUFFMAN_ALPHABET> freqs;
                                                                ^
  /usr/lib/llvm-10/bin/../include/c++/v1/__tuple:219:64: note: template is declared here
  template <class _Tp, size_t _Size> struct _LIBCPP_TEMPLATE_VIS array;
                                                                 ^
  5 errors generated.
  Makefile:23: recipe for target 'obj/HuffmanArchiver.o' failed
  make: *** [obj/HuffmanArchiver.o] Error 1
  

• Падают ваши собственные тесты

  osboxes@osboxes:~/cpp2019/cpp-labs/hw_03/check$ ./run_tests.sh ../../../cpp19/filippov.denis/hw_03/hw_03 
  Running tests from ../../../cpp19/filippov.denis/hw_03/test_hw_03
  [doctest] doctest version is "2.3.7"
  [doctest] run with "--help" for options
  ===============================================================================
  test/TestHuffmanTree.cpp:6:
  TEST SUITE: Huffman tree works correct
  TEST CASE:  Check constructors of tree
  
  test/TestHuffmanTree.cpp:35: ERROR: CHECK( freqs == checkFreqsArray ) is NOT correct!
    values: CHECK( {?} == {?} )
  
  ===============================================================================
  [doctest] test cases:      3 |      2 passed |      1 failed |      0 skipped
  [doctest] assertions:      8 |      7 passed |      1 failed |
  [doctest] Status: FAILURE!
  Student's own tests fail
  

• Падает на пустом файле.
• С Valgrind падает на практически любых тестах.
• Test failed -- expected compressed data to be no longer than source data, got 257 > 256 (тут смотрим на сжатые данные без учёта дополнительных)
  • На этом же тесте: Test failed -- invalid compressed size reported: 2304 instead of 2305
  • Аналогичная картина есть на нескольких тестах

Тесты 1/8

• Не тестируется парсинг аргументов.
• Не тестируются битовые потоки.
• Не тестируется архиватор
• В тесте конструктора тестируется сразу куча всего для двух случаев сразу. Это должны быть два отдельных теста с подтестами как минимум.

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

• HuffmanCLI приниципиально не отличается от main. Например, вы не можете протестировать парсинг аргументов.
• HuffmanException
  • разделите хотя бы на ошибку парсинга аргументов и формат файла.
  • Унаследуйтесь от runtime_error/logic_error` и не надо перереализовывать руками.
• Потоки не обрабатывают возможные ошибки низлежащего потока.
• HuffmanNode приватен для HuffmanTree
• HuffmanTree
  • updateFreqs не имеет никакого отношения к дереву Хаффмана. Конструктор от строчки тоже странный (к тому же он должен быть от string_view).
  • Кажется, у дерева есть три стадии жизни, на которых можно делать разные операции. Сейчас вообще неочевидно, как его правильно использовать. Слишком много методов, полей, АААААА, я ничего не понял. Лучше так не делать. Тут склеились как минимум два класса: дерево Хаффмана и считалка частот.
  • buildCodes должен возвращать значение, иначе неясна семантика случая "передали непустой array".
• HuffmanArchiver не должен ничего выводить на экран никогда. Что делать с посчитанной статистикой — вопрос к main. Никогда не прибивайте потоки ввода-вывода гвоздями.

Стиль 6/8

• CLI
  • Нет нужных #include
  • Почему-то HuffmanException определён в CLI.cpp, но объявлен черт-те где.
  • Объявляйте переменные только там, где они реально нужны. cmd — это ужас.
  • Перемешан парсинг аргументов и открытие файлов.
• Stream
  • В деструкторе лучше не вызывать молча pushBitsByte(), а ставить assert, что пользователь это вызвал. Всё-таки это какое-то очень нетривиальное действие.
  • pushBitsByte — flush()
  • MAX_BIT_POS лесом, содержательнее будет поставить assert(CHAR_BIT == 8)
  • int32_t --> int
• HuffmanNode
  • Нехорошо, что можно сделать ноду с одним ребёнком. Лучше два конструктора.
  • getByte() noexcept
  • assert из isLeaf должен быть в конструкторе на самом деле. Заодно исчезнут аналогичные assert'ы из прочих мест.
• HuffmanTree
  • collapce --> collapse. Или вообще buildTree()? Я не понял разницы.
    • А ещё тут лучше две очереди сделать. Или rotate вызвать. Или emplace явный вместо emplace_back. Цикл for по индексам явно лишний.
  • getFreqsArray — хватит auto&, незачем ->
  • printFreqs() нужен для отладки? Тогда debugPrintFreqs()
• HuffmanArchiver
  • Не используйте eof(), проверяйте возвращаемое значение функции чтения. И [мойте тарелки перед едой](​http://blog.algoprog.ru/init-not-clear/).
  • ADDITIONAL_INFO_SIZE незачем делать константой. Точно не хватает assert'а на то, что она посчитана правильно.

[Egor Suvorov]

P.S. И ещё в паре мест не хватает final и noexcept.
P.P.S. Странно, что в isLeaf стоит assert, а в getByte() стоит исключение. Хотя на самом деле и там, и там у нас ошибка программиста и инвариантов, а не какая-то проблема, которую можно обработать. Это 5/8 по стилю.

[Egor Suvorov]

К сожалению, попытка сделана после дедлайна второй посылки. Первой попытки было недостаточно, чтобы разблокировать третью.

Так что это считается посылкой по акции и проверяется с весом 0.5. Будет выставлена наиболее удачная из двух.

[Egor Suvorov]

Напомнили, что была договорённость. Пишите в следующий раз прямо в тикете, пожалуйста.

[Egor Suvorov]

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

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

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

• Записывать через reinterpret_cast не стоит: это не будет работать на, скажем, древних маках и роутерах, где порядок байт у чисел другой.

Тесты 6.5/8

• В тестах Huffman archiver compress вы дублируете код, а надо получить тот же результат другим методом. Например, не надо писать через reinterpret_cast, надо честно собрать массив нулей нужной длины. Делается в пару строчек.
  • Там как-то очень, очень много разной настройки тестов происходит. Наверняка можно вынести в TEST_CASE и сделать SUB_CASE. Или какую-то вспомогательную функцию.
• Build from nontrivial example какой-то дико сложный. Я не понял, что там происходит. Лучше деревце поменьше. Или проверять в таком сложном не дерево, а сразу коды. Потому что рекурсивный dfs в тестах — это что-то явно лишнее.
• Чтобы было проще отлаживать, сделайте несколько простых CHECK подряд. Тогда doctest их распарсит. Не надо упихивать всё в одно условие.
• Никакого allOk, CHECK на каждое значение. Чем больше отладочной информации "что не так" — тем лучше.
• Многовато церемоний в тестах для того, чтобы что-то простое проверить. Кажется, что слишком сложные интерфейсы у классов, надо упростить, чтобы было проще запускать.

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

• Не хватает namespaces для порядка.
• HuffmanCLIException можно сделать проще и точнее: using std::logic_error::logic_error;. Аналогично с HuffmanException.
• HuffmanCLI
  • parseComLineAndRun даже не определён.
  • getMode() noexcept.
  • NUM_OF_ARGUMENTS — точно приватная деталь реализации в CLI.cpp
  • Не очень хорошо, что вы всё время храните аргументы. Лучше один раз распарсить и сохранить. Тогда сразу можно будет все ошибки выдать.
  • Странно, что argc у вас константный, а argv — нет.
  • Не влияет на баллы: обычно всё-таки целиком парсят вход, чтобы найти сразу все ошибки. А не пытаются распарсить "лениво". Например, сейчас у вас не будут найдены лишние аргументы, а это тоже популярная ошибка.
  • То, что вам приходится в тестах делать что-то странное с .data() — знак того, что интерфейс плохо спроектирован.
• Huffman
  • Очень странно, что есть отдельный HuffmanFreqsCounter (кстати, не должен иметь никакого отношения к Хаффману), но при этом вызываем tree.getFreqsArray(). А в extract() вообще массив создаём. Не надо ли "массив частот" всё-таки объектом сделать? А то много церемоний и в разных местах по-разному.
  • HuffmanTree
    • После вызова конструктора всегда надо вызывать buildForest() + buildTree(). Тогда это должен делать конструктор, чтобы нельзя было забыть. Заодно уйдут лишние флаги, лишние проверки, лишние поля (типа _nodes), лишние тесты с проверкой is_sorted.
• HuffmanFreqsCounter явно не в своём файле лежит, даже тесты намекают.
• HuffmanNode
  • Конструктор от двух вершин может сам посчитать себе байт и частоту.
  • Странно, что геттер для байта есть, а для частоты и _freq нет. Плюс странно, зачем HuffmanTree друг.

Стиль 5/8

• НЕ ИСПОЛЬЗУЙТЕ C-STYLE CAST (в тестах, например).
• ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ТОЛЬКО ТАМ, ГДЕ НУЖНО (в buildTree(), например, last не по делу).
• BitStream
  • CHAR_BIT, а не __CHAR_BIT__. И лучше static_assert, причём в какой-нибудь одной единице трансляции, а не во всех методах.
  • Не надо в ассёртах писать [], он и так говорит точную строчку файла. Или нет?
  • Не надо писать CHAR_BIT - 1, пишите 7.
  • Тесты
    • Не надо выводить ничего на экран в тестах.
    • Лучше не шестнадцатеричные значения, а двоичные: 0b1100'1010
    • Неконсистентное наличие/отсутствие пробелов между TEST_CASE и (
• HuffmanCLI
  • std::string result{} — тут {} лишние
  • Лишние конвертации в string_view, == и так работает.
• Huffman
  • readlData — опечатка.
  • Раз уж знаете про char_traits, то и int32_t — не то, что возвращает get().
• HuffmanNode
  • Код (!_left && _right) || (!_right && _left) не соотносится с both children have to be null or no null pointer
• HuffmanTree
  • Странное отсутствие пустой строки в .cpp между testGetForest() и debugPrintFreqs()
  • Лучше не > 1, а >= 2, чтобы было похоже на 2 из вложенного цикла.
  • По умолчанию цикл всё-таки по int, а не по size_t.
  • makeCodes зря принимает const unique_ptr<>&
• Вы пытаетесь упихнуть всё в 80 символов, забить. Хотя бы 120.

[Filippov Denis]

Договорились о дедлайне исправлений во вторник (5 мая) 22:59