HW #3

[onofriychuk.ilya]

Забыл закоммитить два файла. Только что поправил

[Egor Suvorov]

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

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

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

• Сжатие файла из одного символа рапортует 10 байт для сжатых данных. Это явно что-то не то. Аналогично с тестом "все байты от 0 до 255": там размер "сжатых" данных получается 328 байт.

Тесты 5.5/8

• Не протестирован парсинг аргументов.
• Несимметрично, что тесты в двух разных файлах, а реализация - в одном. И так несколько раз.
• BitHandlers
  • Не хватает пустых строк между тестами и внутри тестов, идёт просто месиво строк.
  • Очень, очень длинные тесты. Смотри замечания по стилю.
  • Не хватает теста на запись обычного числа через writeInt32 - это гораздо важнее крайних случаев "0" и "UINT32_MAX".
• Очень часто внутри теста встречается последовательность: считать бит, проверить, что получилось, проверить бит. Это занимает три строчки, а должно не больше одной. Ну или хотя бы пустая строчка между соседними блоками должна быть, иначе вот такое невозможно парсить глазами:

           CHECK(!bit);
           bit = true;
           readerForCheck.readBit(bit);
           CHECK(bit);
  

• HuffmanTree
  • В Code building test не хватает проверки на то, какой именно код получился.
  • Во Frequencies decrease явно можно проверить, какие именно частоты хотим.
• HuffmanCompressor
  • В Test build frequency vector можно упростить: мы очень точно знаем, что за результат будет. Какие символы и в каком порядке. Лучше это и проверить.

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

• CLI
  • Лучше не отдельный метод parseExpression, а конструктор CLI. Иначе можно случайно вызвать isCompress() на пустом CLI.
• Обработка ошибок отсутствует, это не очень хорошо. Лучше врубить исключения у потоков и обрабатывать их централизованно в main(). Заодно станет проще сигнатура и тестирование у readBit() - теперь он просто возвращает значение, не нужны промежуточные переменные.
• BitHandlers
  • Константы INIT_BUFFER_SHIFT можно объявить сразу в .cpp в анонимном namespace. А ещё они явно не нужны - ну не имеет INIT_BUFFER_SHIFT никакого адекватного значения, кроме 7, если только код не переписать полностью.
  • Странно, что writeByte - метод, а writeInt32 - уже функция. Надо симметрично. Лучше всё функциями с ADL.
  • В деструкторе BitsWriter лучше проверять, что буфер сброшен. И сделать для этого отдельную функцию, которую пользователь должен явно вызвать.
  • BitsReader зря сбрасывает поток в начало. Это ему непринципиально. Пусть это делает вызывающий.
  • Не уверен, что возвращать количество байт - хорошая идея. Возникают вопросы: а как учитывается последний неоконченный байт?
• Huffman
  • Операторы </== у Symbol не соответствуют друг другу. Это плохо.
  • Структура Symbol отдельно от HuffmanNode не используется. Не плохо, но повод задуматься, нужна ли она.
  • Конструктор Symbol - explicit. Частота - явно не uint32_t, а что-то побольше, файлы бывают больше 4 ГБ.
  • HuffmanNode
    • Если убрать мутабельность (а это лучше сделать), то исчезнут сеттеры, геттеры станут только константными, плюс исчезнут инварианты. Вершина всегда будет создаваться сразу в корректном состоянии. Вроде это и так есть, осталось лишние методы стереть. restoreTheTree точно переписывается.
  • HuffmanTree
    • Плохо, что дерево сочетает в себе и дерево, и итератор по дереву. А ещё позволяет напрямую получить доступ к вершинам через getRoot(). Слишком много.
    • Как-то слишком высокоуровнево у вас дерево кодируется: в два вектора. Никогда эти два вектора напрямую анализировать не будем. Лучше уж сразу в поток байт/бит - смотря как лучше алгоритм показывает. Похоже, что кусок writeMetadata - это на самом деле про дерево, а не про сжатие или конкретный формат архива.
• HuffmanArchiver
  • Это получилось две функции, которые вместо возврата результата сохраняют его в поля. Пусть лучше каждая функция возвращает структуру (иначе можно случайно забыть обнулить что-нибудь). А потоки можно в конструкторе привязать, если есть требование "должен быть класс HuffmanArchiver".
  • HuffmanCompressor/HuffmanExtractor - это просто свободные функции, а не классы.
  • Странно, что надо сортировать частоты символов перед тем, как скормить их в дерево. Это же алгоритму дерева важно, как отсортировать и каким образом. Пусть сам это гарантирует.

Стиль 4.5/8

• Оборачивайте локальные для TU константы/переменные в анонимный namespace. Вроде MESSEGE_ABOUT_FLAGS.
• НЕ ИСПОЛЬЗУЙТЕ C-STYLE CAST (Write MAX_UINT32).
• Лучше все поля либо делать с _ во всём проекте, либо нигде.
• Не хватает слов noexcept (конструктор Symbol).
• Методы в .cpp должны идти в том же порядке, что и в .h.
• Исключения лучше наследовать от logic_error/runtime_error и делать using` для конструкторов. Реализация будет в сумме в три строки.
• CLI
  • Многострочные константы лучше разносить на несколько строк: ​https://stackoverflow.com/a/1135862/767632 (я предпочитаю первый стиль из ответа с явными \n)
  • С optional<bool> лучше писать .emplace(false) вместо = false. Иначе путает if (_isCompress) перед этим.
• BitHandlers
  • countWritedBytes --> writtenBytes/bytesWritten.
  • getCountWritedBytes --> countWrittenBytes/countBytesWritten/getNumberOfWrittenBytes/getBytesWritten/getWrittenBytes.
  • writeBit(vector) --> writeBits. И тоже можно функцией с ADL, а не методом.
  • Не .good(), а просто static_cast<bool>(). А внутри ифа даже каст не нужен.
  • //TODO check success - исключения?
  • Вместо write/read для байт стоит использовать put/get
  • Скорее не resetBuffer, а flush(). А в чтении он, кажется, вообще не используется. Не надо ссылаться на внутренние детали реализации ("сбросить буфер"), лучше говорить в терминах абстракции ("добавить нулей до байта", "пропустить до целого байта").
  • Тесты
    • Лучше битовые литералы: 0b1000'0010
      • Можно завести один stringstream и сразу в него присвоить нужное содержимое. Незачем делить на ostream/istream.
      • Не переиспользуйте переменные (как в Read byte у byte есть два смысла), лучше добавляйте блоки и заводите переменные заново.
      • Есть shadowing в Reset after read 9 bit: две переменные i, это нехорошо.
• HuffmanTree
  • Замените итератор currentSym на range-based-for. И НЕ ОБЪЯВЛЯЙТЕ ПЕРЕМННЫЕ ВНЕ ЦИКЛА (тут бы помог цикл for).
  • auxiliaryVec - не лучше, чем tmp. Рассказывайте, что лежит в переменной, а не её тип.
  • direct --> direction. Лучше не сокращать. А то direct - это ещё и "прямой". represent --> representation/code.
  • step() заглушает ошибки. Нужен assert.

[onofriychuk.ilya]

Новый дедлайн 3 попытки -- 7 мая, 22:59 МСК

[onofriychuk.ilya]

Исправлены замечания, однако те, которые касаются тестов исправлены не все.

[Egor Suvorov]

Проверялась ревизия 4271. Поздравляю с итоговой десяткой!

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

Тесты 7/8

• Не хватает не пограничных тестов много где. Сплошные крайние случаи. Можно парочкой пожертвовать, зато добавить общий. В EndToEnd например, в построении массива частот.
• Во всех тестах на чтение/запись (включая HuffmanCompressor) ожидаемый ввод или вывод должны задаваться в виде массива байт, а не через хитрое переписывание кода. Например, в Write metadata стоит задать честный массив байт, а не через readUInt + циклы выражать. Будет проще читать + чётко проверяется формат.
• CLI
  • В тестах стоит использовать std::size вместо 6.
  • Безумно странно, что у вас все тесты размера 6. Например, Repeat flags -c может упасть не только от повторённого -c, но и от отсутствующего -o. Так что что на самом деле проверяют тесты — очень большой вопрос, потому что почти все тестовые данные некорректны сразу кучей способов.
  • Не проверяется разный порядок аргументов.
• BitsReader
  • 130 //10000010 --> 0b10000010
  • Вместо write(reinterpret_cast<char*>) можно же .put()? И заодно переменных лишних меньше.
  • Лучше отделять блоки внутри теста (и особенно тесты друг от друга) пустыми строчками. Например, на проверку одного бита уходит три строчки, это отдельный блок, чтобы читать проще.
  • Test readInt32 --> Test readUInt<uint32_t>(), чтобы имя соответствовало названию. Иначе создаётся впечатление, что есть какая-то функция readInt32, хотя это неправда.
• BitsWriter
  • CHECK(istr.str() == ""); — странный код. Вы только что один байт записали. Должна быть непустая строчка. А если вам важны нулевые символы, то нельзя сравнивать с сишными строками, надо честно собирать std::string от некоторого интервала значений.
  • Если надо записать блок в stringstream — то лучше завести массив чисел и создать stringstream от этого массива (или соответствующего string, который создан от begin/end массива).
• HuffmanTree
  • Плохо, что тест может пойти по разным веткам. Вероятно, это из-за неоднозначностей при сортировке? Тогда лучше какой-то порядок на вершинах зафорсировать.

HuffmanCompressor

• std::string str(100, 'a') для создания длинной строчки.
• Странно, что проверяется только частота символов.

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

• CLI
  • Не стоит запихивать справочное сообщение в исключение. Это лучше отдельно в main отдельным действием сделать. Например, если любая ошибка парсинга аргументов вылетела, то выдасть сообщение.
  • Не надо передавать отдельным параметром количество элементов в массиве - это не по-плюсовому.
  • Вместо const char ** лучше vector<string_view> или что-то такое плюсовое. Тогда минус strcmp, можно == использовать.
• BitHandlers
  • operator bool() должен быть explicit
  • Нехорошо, что читатель/писатель сами решают установить режим исключений на потоке. Лучше требовать это от пользователя. Иначе получается не "прозрачная обёртка", а что-то странное.
• Huffman
  • HuffmanQueue — точно деталь реализации из Huffman.cpp, в Huffman.h ей не место. А в ней точно popNode — деталь реализации и свободная функция, не метод.
  • Возможно, вместо vector<bool> для (де)сериализации дерева стоит использовать BitHandlers?. Название метода readByteFromBoolVector активно намекает.
  • 2 * n - 1 + 8 * n — какая-то странная формула, стоит её сделать хотя бы статической функцией дерева. Чтобы при изменении формата не надо было внезапно менять HuffmanArchiver.

Стиль 6.8/8

• Makefile
  • OBJECTS не используется. Можно при помощи filter, filter-out отделить там файлы Test* от остальных и убрать длинные строчки в целях EXE и EXE_TEST.
  • А ещё можно там же использовать $< (первая зависимость), $^ (все зависимости), $@ (имя цели)
• Неконсистентный стиль для приватных полей: _isCompress vs isCompress
• CLI
  • MESSAGE_ABOUT_FLAGS --> HELP_MESSAGE. А ещё его лучше как constexpr char[], чем std::string
• main
  • \"" + (cli.getInFileName()) + "\"." — скобочки не нужны.
• BitHandlers
  • Константы вроде INIT_BUFFER_SHIFT, мне кажется, бесполезны. Но вопрос вкуса.
  • Безумно странно, что ++writtenBytes и _ostr.put() находятся в разных ифах. Кажется, что writtenBytes означает не то, что написано в названии.
  • (uint8_t)(byte & ((uint8_t)1 << (shift - 1))) — лишние C-style касты, не нужен каст снаружи совсем (всё равно записываем в bool).
• Huffman
  • return !_currentNode->getLeft(); — стоит заодно поставить assert (можно в конструкторе), что оба ребёнка либо одновременно есть, либо нет.
• HuffmanArchiver
  • auto tree = readMetadata(input); — тут отлично сработает structured binding (который, впрочем, мы тогда ещё не прошли)
  • Очень странно, что extract требует отдельно разобрать случай "кодируем ровно один символ".
  • Имена istr/input путаются. Лучше inBytes/inBits или что-то такое.
  • countWritedBytes лучше перемеиновать в bytesWrittenBefore или что-то такое.
  • Временная переменная для representationTree не нужна.
  • Не надо дублировать 256 дважды: один раз сделали в объявлении массива, потом взяли .size().