HW #3

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

Для пустого файла не работает.

Internal error: cannot build HuffmanTree on zero symbols. Must be caused by invalid input.

На файле из одного символа 'x' тоже не работает

На файле из ровно одного типа символов тоже не работает (результат архивирования-разархивирования не совпадает с исходным файлом)

Статистика неправильная:
на вашем же Makefile:
hw_03 -c -f Makefile -o compressed

2999
2305
243

Но размер полученного файла: 2306 байт

Размер итогового файла должен быть равен сумме двух чисел из статистики (2305 + 243)

При разархивировании:

2306
2998
243

---

end-to-end тесты? Тесты CLI? тесты чтения/записи вспомогательных данных?
Тесты обратного преобразования: строка-кода в символ?

Валиадацией аргументов должен заниматься парсер.

            std::tuple sizes = archiver.compress();
            std::cout << std::get<0>(sizes) << '\n';
            std::cout << std::get<1>(sizes) << '\n';
            std::cout << std::get<2>(sizes) << '\n';

Если используете C++17, используйте до конца. structure binding.
std::tuple -- плохое решение для представления структур, чьи поля означают что-либо нетривиальное. Лучше стоя структура.

Если произошла ошибка, надо возвращать ненулевой код возврата.

А зачем вы выдумали ArgumentIterator?, который еще и не является полноценным c++ итератором? У вас и так argv указатель. А указатель вполне себе итератор. И зачем он нужен полем класса, если он одноразовый?

---

using HuffmanNodePtr = std::unique_ptr<HuffmanNode>;
Вот этот using у вас дважды причем в разных скоупах (в глобальном и в HuffmanNode?)

Скорее всего, должен быть один using в глобальном скоупе. Используйте forward declaration для HuffmanNode?

HuffmanArchiver? конструируется для двух имен файлов. И имеет два публичных метода. Вызвав один метод, больше нельзя никогда вызывать другой. Надо бы сделать так, чтоб вызов публичных методов не зависел от внутреннего состояния класса. В идеале, пользовательский должен иметь право вызывать любые публичные методы в произвольном порядке.

    root = std::move(std::make_unique<HuffmanNode>(std::move(nodes.back())));
}

#include <iostream>

HuffmanTree::HuffmanTree(const std::unordered_map<char, SymbolCode>& encodedSymbols)

Никогда не подключайте заголовки стандартной библиотеки посреди вашего кода. Это может привести к чему угодно. Будут в C++ модули, можно будет так делать.

---

Все файлы надо открывать в бинарном режиме

    std::unordered_map<char, SymbolCode> symbolCodes;
    SymbolCodesTable codesTable(symbolCodes);
    size_t tableSize = codesTable.readBinaryTable(reader);

А почему бы классу SymbolCodesTable? внутри себя не создать этот map? А дереву конструироваться не из map, а из SymbolCodesTable?. И readBinaryTable мог бы быть статическим методом.
Зачем придумывать странные зависимости между ссылками на стандартные контейнеры?

---

4 + 3.5 + 4.5 + 7

[Денис Лочмелис]

Теперь работает для пустого файла, одного символа, одного типа символа. Все отражено в тестах.

Статистика стала сильно лучше, но все еще местами отличается от правильной на +/-1.

Тестов стало раза в три больше.

Парсер переписан, убран бесполезный итератор.

Переработан интерфейс HuffmanArchiver?. Теперь его можно переиспользовать и тестировать.

Артефакты-инклуды убраны. Файлы теперь открываются правильно.

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

со статистикой что-то совсем плохо стало:
Файл из одного символа:

Test failed -- invalid compressed size reported: 22 instead of 11
Test failed -- expected compressed data to be no longer than source data, got 11 > 1

---

        if (!argParser.hasArgument("inputFile"))
            throw std::invalid_argument("no input file provided");
        std::string inputFile =
            argParser.getArgumentValue("inputFile");

        if (!argParser.hasArgument("outputFile"))
            throw std::invalid_argument("no output file provided");
        std::string outputFile =
            argParser.getArgumentValue("outputFile");

        bool compress = false;
        bool extract = false;
        if (argParser.hasArgument("compress"))
            compress = true;
        if (argParser.hasArgument("extract"))
            extract = true;

Мне кажется, это было и в прошлой версии. Код в main не должен этой ерундой заниматься.

Вложенность кода можно уменьшить с помощью function-try-block

*
два одинаковых using. Решается с помлощью forward declaratiron

struct HuffmanNode;

using HuffmanNodePtr = std::unique_ptr<HuffmanNode>;

struct HuffmanNode {
.....
};

• class HuffmanTreeIterator { // does not have much to do with iterators Наверное, тогда его надо назвать как-то по-другому.

• Объявление бинарных читалок стоило вынести в отдельный заголовок

• Это первое решение, которое свалилось по TL. Остальные проходят даже без оптимизаций.

  SymbolCode HuffmanTreeIterator::getCode(char symbol) {
      if (codes.find(symbol) == codes.end())
          throw HuffmanException("no such symbol in HuffmanTree");
      return codes[symbol];
  

1. Постоянное копирование векторов при возвращении значений.
2. Тут два прохода по map вместо одного.
3. Метод этот может быть константным.

void BinaryStreamWriter::write(const std::vector<bool>& data) {
    for (bool b : data)
        write(b);
}

void BinaryStreamWriter::write_uint32_t(uint32_t data) {
    for (unsigned int i = 0; i < 32; i++)
        write((data >> i) & 1);
}

void BinaryStreamWriter::write_char(char data) {
    for (unsigned int i = 0; i < CHAR_LENGTH; i++)
        write((data >> i) & 1);
}

Сведение задачи к уже решенной -- это замечательно, но тут дико неэффективно.

Исходный бинарный файл не надо читать по биту -- нажно читать только по байту. При использовании этого BinaryStreamReader? вы получаете замедление как минимум в 8 раз!

С записью при разархивировании то же самое

• Свою реализацию I/O тоже надо тестировать

---

6 + 7 + 4.5 + 7.5