HW #3

[Egor Suvorov]

Пока в очереди.

[Egor Suvorov]

Проверялась ревизия 4014 или раньше.

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

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

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

Тесты 5/8

• Никогда не используйте файлы для тестов. Используйте stringstream.
• В тестах дерева Хаффмана проверяйте и коды символов тоже, а не только длину кодов.
• Не используйте rand(), используйте <random>
• Тесты сложноваты. Например, Binary encoding должен быть не сложнее, чем: создали два массива (вход и выход), запустили дерево, проверили результат. Но вам тут сильно мешает, что у вас всё через файлы.
• Что за catch(...) O_O Не используйте CHECK(everything_is_ok), ставьте CHECK() на каждое условие.

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

• BitReader/BitWriter
  • Плохо, что он не оборачивает поток (например, приняв его по ссылке), а пытается эмулировать все конструкторы. Из-за этого раздувается интерфейс и ответственность.
  • Обычно всё-таки перемещение не запрещают.
  • Тут намного лучше лучше правило нуля. Копировать ifstream нельзя => BitReader тоже нельзя. и деструктор у ifstream адекватный.
  • Странный метод readNextByte(), когда уже есть operator>>(). И он же только в этом операторе и вызывается.
• HuffmanNode
  • Конструктор от чистых указателей — неясна семантика владения. Надо уточнить. Например, при помощи unique_ptr
  • Лучше не возвращать чистые указатели, а ссылки.
  • Можно переименовать в Node, оно и так внутри дерева лежит.
• HuffmanTree
  • Странный метод buildCodes: надо вызывать всегда, причём всегда с определёнными параметрами. Как минимум он приватный. А вообще похож на часть конструктора.
  • getTreeSize() — зачем?
  • explicit HuffmanTree(const std::vector<std::pair<std::size_t, unsigned char>> &frequencies); — очень нехорошо, что здесь предполагается отсортированность массива частот. Это нужно в первую очередь дереву, вот пусть и заботится об этом.
• HuffmanException/ParserException
  • Можно упростить при помощи using конструктора
• HuffmanArchiver
  • Многовато полей, многовато ответственности. И файлы открывает, и статистику хранит, и количество символов в файле считает... Статистику точно можно вынести отдельно. И снова возникает проблема с тем, что файлы пронизывают всю архитектуру из-за BitReader.
  • Многовато лишних методов. Которые ещё и данные передают через поля. Например, countFrequencies вполне мог бы быть отдельной функцией, которая принимает файл и возвращает массив. И можно было бы заодно протестировать.
• CLI
  • ArchiverMode — enum class, пожалуйста.
  • Геттеры не noexcept, потому что копируют строчку.
  • Наличие const_cast в тестах и количество необходимых для тестирования церемоний намекает, что у CLI ни разу не плюсовый интерфейс. Сделайте что-нибудь более приличное для C++, а не копируйте тяжкое наследие C.

Стиль 2.5/8

• Include guards в .cpp не нужны O_O
• Не нужен отступ внутри namespace на весь файл.

*

        std::vector<std::pair<std::size_t, unsigned char>> frequencies = {{1, 'e'},
                                                                          {1, 's'},
                                                                          {2, 't'}};
// Лучше писать как
std::vector<std::pair<std::size_t, unsigned char>> frequencies = {
    {1, 'e'}, {1, 's'}, {2, 't'}
};

• new и make_unique НЕ ВОЗВРАЩАЮТ nullptr, это не malloc. Они сразу кидают bad_alloc:​https://isocpp.org/wiki/faq/freestore-mgmt#new-never-returns-null
• НЕ ИСПОЛЬЗУЙТЕ new, используйте unique_ptr.
• ОБЪЯВЛЯЙТЕ ПЕРЕМЕННЫЕ ТОЛЬКО ТАМ, ГДЕ ОНИ НУЖНЫ (смотрю на symbol в HuffmanArchiver)
• В include guards лучше разделять слова _
• BitStream
  • static_cast<bool > — пробел лишний. Да и вообще много где. Пройдитесь автоформаттером.
  • Лучше битовые литералы даже вместо 128.
  • В Test bit write совершенно не по делу заводятся переменные.
• HuffmanNode
  • Используйте member initialization list.
  • make_unique не может вернуть nullptr. Не нужен этот if. И не кидайте bad_alloc самостоятельно.
  • Не нужны скобки в return
• HuffmanTree
  • nextNode — какой-то дикий метод. Это точно можно сделать проще. И там скорее не исключение в конце, а assert, потому что ошибка программиста. Подсказка: обычно алгоритм объясняется не в терминах очереди и указателя на элемент массива, а сразу в терминах очереди вершин. Тогда куча случаев уедет.
  • В HuffmanTree() тоже дичь какая-то. bad_alloc не нужен, чистые указатели запрещены, tmp не нужно.
  • Неясно, зачем codes_ размера 256 + 1.
• HuffmanArchiver
  • Снова не нужна переменная tmp
  • Вместо throw error; просто throw;
  • В extract происходит что-то непонятное. Почему output_size в битах? Почему цикл идёт до результата какого-то целочисленного деления?
• CLI
  • Используйте цепочку else if вместо if + continue. Как и выше.
• main
  • Не сбрасывайте буфер после каждой строчки (endl).

[Ruslan Salkaev]

Что успел, то исправил