HW #3

[Solovyev Gleb]

проедленный промежуточный дедлайн на один день до 22:59 пятницы

[Egor Suvorov]

Да, было такое. Это вторая попытка.

[Solovyev Gleb]

Replying to Egor Suvorov:

Да, было такое. Это вторая попытка.

Да, верно, просто не обратил внимания при создании тикета.

[Egor Suvorov]

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

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

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

• Не хватает открытия файлов в бинарном режиме: не будет работать под Windows.

Тесты 7.9/8

• В среднем красота! Разве что архитектура мешает сделать простые тесты: приходится очень много проверять.
• test HuffmanNode - это два разных тестах.

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

• Последовательности бит - точно не string. Скорее vector<bool>, чтобы пользоваться статической типизацией: есть значение - точно битовая строка.
• У объектов не должно быть "неинициализированного" состояния, используйте конструкторы. Смотрю на Symbol, CLI...
• Очень много геттеров, сеттеров, лишних методов, изменяемых структур. Невозможно отследить инварианты, читая только .h-файл. Пример: смотрим на тест test doNextDecryptDfsStep() & curNode checker, там жесть какая-то и проверяются три штуки одновременно.
  • Можно стереть/заинлайнить половину кода, станет проще и короче (например, getNodeCmp).
  • Лучше иммутабельные структуры: один раз вызвали нужный конструктор, потом только геттеры. Или агрегаты: все поля публичные, не надо извращаться с геттерами/сеттерами лишний раз.
• Много где используются "специальные значения" (типа нулей или пустых строк) вместо std::optional.
• Имхо, не стоит оборачивать исключения потоком в свои: это усложняет код и не упрощает обработку ошибок.
  • Аналогично, в HuffmanArchiver не надо ловить и перебрасывать исключения. Вы их не обрабатываете, не можете обработать, и не добавляете полезной информации. Пусть летит так. А ещё вы ловите вообще все std::iostream::failure и заявляете, что это всегда ошибка чтения - не верю. Поменяю пару строк где-то в недрах и этот инвариант нарушится.
• BitManips
  • Неожиданно, что нет способа записать один бит. Точнее, есть, но приватный. Это же самая базовая операция. А записать байт/строчку - это уже даже свободные функции могут быть плюс ADL.
  • В деструкторе writer стоит проверить, что записано целое число байт.
• huff_cli/huff_logic/huff_archiver --> huffman::cli...
• CLI
  • Лучше заменить parseArgs на CLI. Тогда у CLI станет очень простое иммутабельное состояние, не надо будет думать "а что если оно не проинициализировалось".
  • В C++ не принято отдельно передавать массив и его размер. И сишные строки тоже. vector/span/string_view... Заодно в тестах не надо будет создавать массив.
• HuffmanNodePtr объявляется очень много раз.
• Huffman
  • Несимметрично, что в этом файле столько всего. Разделение тестов на три файла намекает, что, возможно, этот файл тоже следовало распилить. Ну или тесты в один файл положить. Сейчас неясно, где искать тесты для классов/структур из этого файла.
  • Symbol
    • уберите геттеры/сеттеры, это просто структура-агрегат. Никаких дополнительных проверок нет. Но ещё точно стоит сделать explicit-конструктор какой-нибудь, чтобы не было неинициализированного состояния.
      • Довольно странно, что структура "Символ" не содержит, собственно, символ. Не используется ли она только как часть HuffmanNode? Если да - то их стоит объединить.
  • HuffmanNode
    • Конструктор от двух детей часто применяется с двумя пустыми детьми. С частотой и без символа. Это что-то очень непонятное. И как минимум стоит распилить этот конструктор на несколько частей: один для листов, один для внутренних верших, они разные. Причём второй может ещё и частоту посчитать.
      • Конструктор от Symbol на самом деле является конструктором от частоты. Незачем завязываться на Symbol.
      • Слишком, слишком много методов/геттеров/сеттеров. Наверняка можно выкинуть почти все и заменить на assert'ы.
      • Во имя иммутабельности от createLeftChild/createRightChild можно избавиться. При построении дерева можно сделать не цикл с указателем, а честную рекурсию.
  • SymbolDict
    • std::deque<HuffmanNodePtr> getSortedUniqueSymbolNodes() const; - это никакого отношения к таблице частот не имеет. Завязка на Хаффмана, причём ей вообще не надо быть методом.
      • getCount - скорее contains или count для симметрии с STL.
      • checkIfStringIsBitString - это точно не метод структуры, он поля не использует.
  • HuffmanCompressTree
    • После вызова конструктора всегда надо звать build(). Это плохо: снова сложный инвариант. Сделайте всё в конструкторе. Заодно исчезнут 2-3 поля и структура перестанет быть мутабельной.
  • HuffmanExtractTreeException - ровно один вид исключений, надо убрать обобщение.
    • DECRYPT_DFS_STEP_TO_NULL - это название про деталь реализации, а не "почему возникла ошибка". А возникла она потому что нам дали неверный код.
  • HuffmanExtractTree
    • doNextDecryptDfsStep - так себе интерфейс, снова добавляет мутабельность и сложный инвариант внутрь дерева (которое вообще не меняется).
• HuffmanArchiver
  • Кажется, что поля и конструктор вообще не нужны. Это просто сборник из двух функций, даже статическими можно сделать. Я бы предложил привязываться к потокам ввода-вывода в конструкторе, а compress()/decompress() параметры не принимают и возвращают Statistics.

Стиль 5/8

• Всякие исключения лучше наследовать от logic_error/runtime_error и делать using конструктора.
• outs_.binary - это просто в точности константа std::ios::binary (потому что это член enum'а, а они видны как обычные члены. Так что эти assert'ы бесполезны.
• В реализациях оставляете пустые строки между методам (в .h лучше не оставлять, а группировать методы в группы). Например, в BitManips.cpp.
• НЕ ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ВНЕ ЦИКЛОВ (смотрю на void HuffmanCompressTree)
  • Аналогично не объявляйте переменные, пока они не потребутся. Например, CLI в main зачем-то определён в начале... И так в куче мест. Но это вы скорее заложник архитектуры, в которой всё мутирует, везде сложные инварианты.
• BitManips
  • constexpr size_t BITS_NUMBER = 8; - лишний, лучше захардкоидть.
  • buffIsClear - не стоит в интерфейсе ссылаться на внутренние детали реализации ("буфер" какой-то). Лучше сказать что-то вроде "записано целое число байт"/"записано нецелое число байт": hasIncompleteByte. Аналогично в читальщике.
  • getWrittenBytesNumber --> getBytesWritten. А сейчас оно ещё неконсистентно даже с именем поля.
  • bitToWriteIn_ --> nextBitPos_, writtenBytesCnt_ --> bytesWritten_
  • clearBuff --> скорее skipZerosUntilByteBorder. И может скорее возвращать "удалось или нет". А после этого checkIfUnreadBuffBitsAreZeros можно выпилить.
  • Обычно не buff, а buf.
  • Один байт следует записывать через put/get, а не reinterpret_cast.
  • Тесты
    • Вместо convertToByte используйте битовые литералы: 0b1110'0011.
      • Вместо записи в ans байт вперемешку с тестом, лучше сравнить ss.str() с готовым массивом/string. Будет проще код и понятнее логика, плюс чёткое отделение Arrange-Act-Assert.
• CLI
  • constexpr size_t ARGUMENTS_NUMBER = 6; - точно деталь реализации и должна жить в .cpp. А ещё эта проверка не нужна: если аргументов неверное количество, то всегда можно рассказать пользователю, что именно с ними не так: дубликат, чего-то не хватает... Лучше так, чем подгонять константу "6".
  • assert(0) --> assert(false). return после assert следует убрать.
  • Вместо == "" лучше .empty() (с != аналогично).
  • Тесты
    • Не надо делать argc руками, возьмите std::size()
• Huffman
  • MAX_DIFF_BYTES_COUNT - тоже явно деталь реализации из .cpp
  • Вместо isLabeled_ + label_ лучше std::optional<>.
  • getNodeCmp не нужен, просто передайте лямбду напрямую. А ещё это вообще свободная функция может быть в .cpp.
  • assert(minNodePtr->hasLeftChild() && minNodePtr->hasRightChild()); assert(!minNodePtr->isLabeled()); - наружу просится метод isLeaf()/isInner().
  • HuffmanCompressTree/HuffmanExtractTree - безумно странные названия. Назовите их по тому, что эти структуры делают, а не как используются. Типа: "бор" или "алгоритм Хаффмана" (но тогда не является ли это просто функцией, а не классом? Тоже надо подумать).
• HuffmanArchiver
  • } catch (const std::exception& ex) { throw; } - абсолютно бесполезная конструкция. Исключение и так вылетит.

[Solovyev Gleb]

Все исправил, кроме наследования исключений от std::runtime_error в местах, где были enum-ы - слишком неудобно и громоздко в данном случае.

Дедлайн третьей попытки был назначен на понедельник 11 мая 22:59.

[Solovyev Gleb]

Также вместо get и put пользуюсь read и write c reinterpret_cast, так как: работаю с unsigned char-ами, для этого нужен каст; при этом get и put от его использования не спасают (get точно, поэтому для консистенции write идет вместе с read).