HW #3

[Дмитрий Свиридкин]

входные файлы для тестов стоит положить в отдельный каталог.

ERROR: test case THREW exception: ../test/sound.wav: Положите каталог на одном уровне с бинарным файлом.

target_compile_options(test_hw_03 PRIVATE -O2 -Werror -Wall -Wextra -Wshadow)

Не надо добавлять -O2. Уровень оптимизации выставляется автоматически, указанием cmake типа сборки.

cmake -DCMAKE_BUILD_TYPE=Release

add_compile_options(-std=c++17) -> set (CMAKE_CXX_STANDARD 17) Не используйте платформо-специфичные опции без необходимости.

void CLI::closeFiles() {
    is.close();
    os.close();
}

Зачем? Деструкторы сделают все за вас.

CLI::CLI(int argc, const std::vector<std::string_view>& argv)
Зачем argc, если у вас вектор?

Дизайн структуры CLI очень сомнительный. Она выполняет сразу два дейсвтия: парсит аргументы и открывает файлы. Хотя по смыслу должна только парсить аргументы.

У классы HuffmanTree? та же проблема: объект сначала аккумулирует статистику и не является деревом, а потом строит себя. Получается, сконструированный объект еще не готов к использованию. И класс выполняет две функции вместо одной.
Нужна отдельная сущность.

Код построения дерева делает сложные манипуляции с двумя очередями, возможно, корректные, но я не проверял. С priority_queue это делается намного короче, проще и точно корректно. Вы все равно испортили линейную асимптотику сортировкой.
Вынесите эти сложные манипуляции в собственный класс приоритетной очереди с простым интерфейсом и покройте тестами.

Для EndToEnd? тестов стоит сделать одну функцию, вместо копипасты ее в 4 файла с разными аргументами.
А еще лучше сделать простой bash-скрипт, вызывающий ваш архиватор и применяющий diff. Тогда это будет полный тест без привязки к вкомпилированным путям.

Магическая констатна 256 местами именованая, местами магическая.

8 + 6 + 1 + 7

[Бубнов Данил Константинович]

Написал скрипт, который не все файлы тестирует, а только один, который попросили, чтобы можно было с doctest интегрировать. И всё остальное вроде бы исправил, добавил ещё тесты для CLI.

[Бубнов Данил Константинович]

Добавил ещё один коммит уже после создания тикета: добавил const у методов

[Дмитрий Свиридкин]

Можно уменьшить вложенность кода в main.cpp с помощью ​https://en.cppreference.com/w/cpp/language/function-try-block

Вместо констант 0 или 1 для кода возврата лучше EXIT_SUCCESS/EXIT_FAILURE (cstdlib)

        std::ifstream is(cli.inputFileName);
        std::ofstream os(cli.outputFileName);

Под Windows будут проблемы.

    HuffmanNode* getMainNode() const;

Может указатель на константу?

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

---

8.5 + 8 + 4.5 + 7

[Бубнов Данил Константинович]

То есть иногда лучше потерять в скорости работы (всё равно же чуть больше времени займет ещё раз пройтись по файлу), но получить более читабельный код?

[Дмитрий Свиридкин]

Replying to Бубнов Данил Константинович:

То есть иногда лучше потерять в скорости работы (всё равно же чуть больше времени займет ещё раз пройтись по файлу), но получить более читабельный код?

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

У вас и так и так два прохода по файлу. Первый -- накопление статистики. Второй -- само кодирование.
Третьего прохода не нужно.

Наиболее оптимальным по производительности на маленьких файлах будет решение, закидывающее файл в память. На больших -- может и не поместиться:)

[Бубнов Данил Константинович]

Вроде бы всё исправил

[Дмитрий Свиридкин]

*

   if (is.fail() || os.fail()) {
        std::cout << "Error opening the file" << std::endl;
        return 1;
    }

А вот тут забыли константу. Вместо вывода и return можно было кинуть исключение.

• Можно из функций compress/extract вернуть структуру со статистикой сжатия, чтоб не хранить ее в полях HuffmanArchiver

•  const HuffmanNode* node = tree.getMainNode();
      for (size_t k = 0; k < numberOfWrittenBits; ++k) {
          bool t = manip.readBit();
          if (manip.ifstreamIsEnd()) {
              if (k + 1 != numberOfWrittenBits) {
                  throw std::ios_base::failure("Input file error: unexpected size");
              }
              break;
          }
          if (t) {
              node = node->leftChild.get();
          } else {
              node = node->rightChild.get();
          }
          if (node->leftChild == nullptr && node->rightChild == nullptr) {
              manip.writeByte(node->byte);
              ++newBytesCount;
              node = tree.getMainNode();
          }
      }
  

Вот эту логику стоит завернуть в какой-нибудь TreeWalker? с одним единственным методом
std::optional<byte> Consume(bit)

---

9 + 8 + 4.7 + 7.8