HW#3

[Egor Suvorov]

Увы, третьей попытки на баллы нет. Захотите дорешать просто так — переоткрывайте тикет.

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

• Программа всегда выводит raw/compressed, а должна выводить разное в зависимости от ключей.
• Иногда выводится какая-то фигня вместо отчёта по байтам:

  Test failed -- invalid source size reported: 2 instead of 16
  Test failed -- invalid compressed size reported: 32 instead of 18
  Test failed -- expected compressed data to be no longer than source data, got 16 > 2
  

Тесты 2.5/8

• Названия файлов с тестами плохо соответствуют названиям заголовков — пахнет, что разделение по файлам не очень.
• TestBitWriter
  • Из-за обёртки testBitWriter возникает ощущение, что
• Много чего из Huffman.cpp не протестировано. Подозреваю, следствие архитектуры (смотри ниже).
  • Не протестировано создание кодов: какой длины они создаются, какого вида, оптимальность... Самое близкое — testPrefixFreenesOn, но вы просто переложили поиск багов с кода на тесты. Это нельзя проверить глазами. Должны быть простые тесты, которые можно проверить глазами, не запуская код.
  • Не протестирован здоровенный метод readMetadataAndBuildExtractor
  • Не протестирован здоровенный метод writeMetadata

Архитектура 1/5 — основная претензия к разделению на файлы и HuffmanArchiver.cpp

• CLI — используйте конструктор вместо parseArguments
• Слишком большой класс HuffmanNode. Да и HuffmanTree тоже.
  • Это сейчас не классы с ограниченной зоной ответственности, а свалка методов и полей, приправленная зачем-то геттерами и сеттерами.
  • Например, эти классы сейчас отвечают минимум за: построение дерева и корректную сортировку, подсчёт количества символов, ввод-вывод метаданных, спуск по дереву...
    • Этим всем должны заниматься отдельные функции/классы, которые отдельно тестируются.
    • Как проверить, что стало лучше: вы можете взять любой отдельный кусок алгоритма и написать на него юнит тест. И так получится, что это как раз юнит-тест целиком на класс/функцию. Например: "построить дерево/кода для таких-то частот".
    • Как минимум, название readMetadataAndBuildExtractor намекает, что у нас HuffmanTree может внутри себя строить некий Extractor, который что-то делает. И, видимо, его прям надо построить в каких-то случаях?
    • descend(HuffmanNode, bool) — похоже на метод Node.
  • Мутабельность HuffmanNode убрать. Это очень усложняет код. Это, на самом деле следствие предыдущего пункта: если куча ответственности, то у каждого объекта куча промежуточных состояний, методов и сложных инвариантов не по делу.
    • Кажется, что setData не нужно, setLeft/setRight/setCode тоже...
    • Особенно setCode выглядит странно в совокупности с computeCodes() — так вершина сама считает или ей кто-то код рассказывает?
    • После убирания мутабельности уйдут перегрузки для getLeft/getRight
    • incWeight() — на единицу? Для подсчёта статистики? Это единственный способ протестировать дерево? Брр.
• У вас три файла со всякими штуками ввода-вывода. Я не смог найти логику в разделении.
• IoBinManip.h/IoBinManip.hpp одновременно — жутковато.
• Очень странно, что вы много где проверяете флаги потоков, хотя в самом начале включили исключения для них.
• Не надо возвращать пары/кортежи там, где у вас на самом деле возвращается какая-то статистика. Лучше структуры с именованными полями.
• HuffmanArchiver зачем-то руками разбирает битовый поток. Хотя BitWriter у вас был.
• ceildiv — это очень тревожный симптом: вместо того, чтобы в одном классе сосредоточить ответственность записи/чтения и корректного подсчёта символов, вы считаете это по отдельности руками. Вот, пришлось даже специальную функцию завести. И следить, что она везде корректно используется.

Стиль 4/8

• Очень дикий плотный код с кучей состояний (но это больше про архитектуру).
• Не хватает noexcept у методов.
• Пробел между if и (
• Пробелы вокруг = в bool create = false.
• В чём смысл писать argc <= i + 1 вместо i + 1 >= argc? Обычно, имхо, размер буфера справа и конструкция >= n очёнь стандартная.
• CLIException — используйте using runtime_error::runtime_error
• std::string x = ""; — строчка и так всегда пустая по умолчанию.
• if (!inp.good()) — просто if (!inp)
  • (from.fail() && !from.eof()) || !from.bad() — тоже какая-то дико странная комбинация
• Странно, что ArchiveSizes — в Huffman.h, а не в HuffmanArchiver.h.
• Huffman.cpp:169: нет, raw pointer там вполне адекватен: мы хотим мутабельный невладеющий указатель на что-то. Ровно правильное место для чистого указателя.
• HuffmanArchiver.cpp:8: да, надо.
• HuffmanArchiver.cpp:54: нормально. Тут просто честный классический итеративный алгоритм. Хаффман принципиально по-другому не написать. Любая попытка завернуть его в подобие fold обречена на провал. Он придуман так, чтобы было удобно писать в итеративном стиле. В состояние дерева точно "текущую вершину" пихать не надо, дерево существует независимо от алгоритма (де)кодирования.
• testArchive: принимайте по const&

[Igor Engel]

Мда. С большинством согласен.

IoBinManip?.h/IoBinManip.hpp одновременно — жутковато.

Соколов предлагал так реализовывать заголовки с темплэйтами, чтобы было привычное разделение declaration/definition

В чём смысл писать argc <= i + 1 вместо i + 1 >= argc? Обычно, имхо, размер буфера справа и конструкция >= n очёнь стандартная.

Хм. Интересно. Чёт ни разу не видел >= n

Особенно setCode выглядит странно в совокупности с computeCodes() — так вершина сама считает или ей кто-то код рассказывает?

В зависимости от того, как строим дерево.
Возможно, стоило сделать что-то типа двух типов деревьев...

(from.fail() && !from.eof())

!from.bad() — тоже какая-то дико странная комбинация

Просто, если поток eof, то он fail... Поэтому "прочитать до eof'а" это какая-то нетривиальная операция, которую нельзя сделать если поставлен экспшен на fail... Так-что конкретно эта конструкция кажется единственный способ проверить, что мы адеватно дочитали до eof'а... А в остальных местах я уже запутался во флагах и исключениях из-за этой фигни с eof'ом... Мда.

[Egor Suvorov]

Про заголовки: окей, понял. Но я бы тогда убрал #pragma once из IoBinManip.hpp (его вообще должен включать только IoBinManip.h). И там тогда нельзя делать никаких typedef, потому что это уже не деталь реализации, а публичный интерфейс.

Если в зависимости от того, как строим дерево — это либо два вида деревьев, либо один общий (скорее всего, с setCode), и одна функция "задай коды на этом дереве так-то".

P.S. А ещё код непортируемый: нужно открывать файлы в бинарном режиме.