HW_3 kirill.kondratyuk

[kondratyuk.kirill]

Здравствуйте

Я пока что не сделал исключения и тесты, но код работает и валгринд не особо ругается:)

С уважением, Кирилл

[Дмитрий Лапшин (lapshin)]

Пааапка и исполняемые фаааайлы

Корректность: я увидел необработанное исключение на каждом тесте. Вообще на каждом. Я даже руками запустил и тоже упало с

terminate called after throwing an instance of 'std::__ios_failure'
  what():  basic_ios::clear: iostream error

Собственно работать оно не может, потому что ты выставляешь флаг исключений на чтение а дальше читаешь циклом while. Цикл while не умеет ловить.

А в чём вообще план включать исключения и не ловить?

Если подоткнуть чтобы хоть как-то жило то: сообщает неправильную статистику (видимо на расжатии). Пустой файл крайне удивляет программу.

Тесты: нема.

Стиль:

1. Когда пишешь компаратор можно писать проще:

   if (l.a != r.a)
       return l.a < r.a;
   return l.r < r.b;
   

   А новые стандарты несут много счастья тут.
2. Магические числа сквозь код.
3. Ты число в строчку бит гоняешь? А стандартные функции чем не подошли (включая bitset)? std::string compressed_text; ты сжатый текст и в памяти держишь?! Не надо пжлст)
4. Сложить весь прочитанный текст в строчку, добавляя по символу, ради размера очень неэффективно. Рекомендую спросить у файла размер другим способом, а для кодирования просто откатить поток на начало. К предыдущему пункту: на вывод тоже не копить всё добро)
5. Зачем руками закрывать поток вывода когда его убьёт деструктор (и закроет?). Вернее причина вообще говоря есть, но вы её не знаете пока и лучше не знать для спокойного сна, и не решишь её так просто.
6. write_thing содержательные имена) Можно было бы сделать методами класса, чтобы не выставлять уж странный параметр про длину. Кстати нужен ли он?..
7. Я бы не пользовался текстовыми потоковыми методами (>> / <<).
8. u_int32_t это кто такой?) Кстати, число вхождений байт (256 чисел) можно было и массивом/вектором.
9. Умные указатели? Ну чтобы delete не писать потом и правило пяти не вспоминать?
10. Хм:

    // метод read_text_count_entries при чтении сразу читает текст, поэтому
    // чтобы не менять сигнатуру метода вынес его сюда
    

    Пахнет ж. А вообще зачем тебе два класса разных наследующих? Ты всё равно использовать их можешь только в контексте создать А или Б и тут же заиспользовать: ты не можешь использовать сжималку и расжималку взаимозаменяемо. Вообще публичный интерфейс мне не то чтобы сильно нравится: что за update_codes для пользователя?
11. strcmp: смотри как я умею

    std::string_view arg = argv[i];
    if (arg == "hi") {
    

4.

[kondratyuk.kirill]

Пока что на исключения меня не хватило
write_thing - читает что-то и прибавляет к счетчику память которую метод записал, поэтому счетчик я думаю что нужен
а вот что с текстовыми методами ввода вывода я не понял. нам же в задании написано мол сожмите текст.
про массив 256 знал, но если у нас будет только один символ, то мое решение будет лучше, а так как зачастую разлчиных символов в тексте примерно 50, то реально я разницы не вижу.
создал класс интерфейс просто чтобы избавиться от копипасты немножко, то есть там овер дофиг переменных у меня, для которых можно было бы не создавать отдельную структуру, а впихнуть все в этот класс и норм.
умные указатели еще не встроил, но скоро.
Ну и написал тестов еще

[kondratyuk.kirill]

Еще не понял про новые стандарты, то есть можно было бы написать лямбду - да, но честно говоря мне кажется что функтор здесь выглядит посолиднее.
про число->строка->бит сначала реальо показалось выгодным:)) но не сейчас:|
еще те методы которые read_text_count_entries разделил все-таки и вынес в unnamed namespace
для всех функций написал тесты, по-моему, кроме тех что непосредственно все собирают вместе
Когда напишу исключения помню что их можно тестить - затещу
Завтра скорее всего будет.

[kondratyuk.kirill]

В общем и целом сделал:

1. Написал лямбду вместо функтора. Но остался вопрос, как это впихнуть в одну строчку, дабы не пложить новых переменных.

2. Магические числа вынесены в переменные внутри cpp файла. Кроме чисел 0 и 1 при работе с битами.

3. Больше не гоняю строку в бит и тп.

4. Теперь использую комбину seekg, tellg

5. Вот этого я на самом деле не понял. Но переделал так, что теперь эти потоки ввода и вывода в приватных полях класса

6. Переименовал _thing в _to_file. Там параметр не длины, а счетчик памяти, который, по моей задумке, нужно менять при записи. Можно было бы его менять вне функции, но я где-то мог его забыть, а так компилятор сам напишет мне мол забыл что-то передать в функцию.

7. Теперь пользуюсь только read и write. Поэтому теперь открываю все файлы как бинарные, так как все равно читаю либо по чару(биту), либо что-то специфичное, но зная что читаю

8. Сделал вместо мапы массив на 256. Тут заюзал accumulate с последней практики

9. Пока что умней

10. Тут я оч хотел избавиться от копипасты таким образом, поэтому так и сделал.

11. Учел и смог

Тесты сделал.

Программа не падает и работает как часы:)

[Дмитрий Лапшин (lapshin)]

Переводов строчки в конце многих выводов тебе яявно не хватает.

Корректность:

1. Cтатистику выводишь неверную. Что-то не так, может просто порядок чисел на разжатии. А может и не только, простым фиксом не поправилось. Пересмотри как в условии.
2. Иногда падает. Хз с чего, просто брык.
3. Иногда разжимает другой файл.

Ну хотя бы иногда работает, это да) 8.

Тесты:

1. Имя. Программы.
2. А компаратор векторов чем не подошёл?
3. Зачем в тестах выделяется дерево на куче?
4. Ну и недописанные тесты ж. Явно ещё крайних случаев не хватает.

3.

Стиль:

1. Входные байтики лучше считать unsigned, а то вдруг захочется обратиться как по индексу. Интересно к чему это я.
2. return in.eof(); Сколько раз говорить как проверять успешность ввода-вывода?
3. Да и вообще комбинация исключения + кода возврата для сообщения ошибок это странно. Тем более когда ты как бы размер уже знаешь.
4. std::string от чариков оооой. А если там ноль?)))))
5. Меня очень смущает класс node: сырые указатели (ты все спец операции сделал правильно? программа-то падает почему-то...), std::string для чего-то, всё очень публичное и в заголовке, куча конструкторов, а в cpp зачем-то дублируется оборачивающими функциями, короче не очень верю ;)
6. Кажется не всё из заголовка достойно там быть.
7. Я же правильно понимаю, что твой ожидает, что код в Хаффмане всегда до 8 бит?
8. Всякие addition_size и прочее просится в поля класса, а функции записи в методы, чтобы было удобно.
9. Порядок функций в файле очень странный)

Короче стало лучше, но ощущение непродуманности всё ещё гуляет, причём напрямую намекая на ошибки местами. 5.

[kondratyuk.kirill]

Стиль

1. Действительно интересно, потому что переменных кроме unsigned у меня просто нет.

2.3. Эти пункты в связке. Ну вообще я хотел хоть как-то отловить модификацию сжатого файла с информацией, мол если он передан не в том формате, то все очень плохо. Но пускай будет UB.
4.5. Изменил на аналог. Теперь это std::vector<uint8_t>. Написал свою функцию поиска без лямбд так как так короче.

6. Вот тут не понял совсем. В заголовке у меня только функции вида save, proceed и конструкторы, причем только их объявления. Да у меня есть шаблонные функции для считывания различного рода переменных, но если я вынесу их в cpp, то будет больше копипасты.
7. Нет, неправильно понимаете. Мой код каждому элементу сопоставляет какое-то установленное алгоритмом количество битиков. Далее все это конкатенирует и бьет на блоки по байту, то есть по 8 битиков, так как вставлять 1 бит мы не умеем вроде как.
8. Они и так поля класса, если я правильно понял о чем вы. Могу их только внутри еще в класс запихнуть. Я бы мог передавать метод который нужно дергать для увеличения счетчиков, но почему бы не сделать так? Так ведь будет намного меньше кода и как по мне так красивее, да я передаю управление над ней в какую-то неведомую функцию, но если я ее передаю, то я точно знаю в какой момент и куда я это делаю.
9. Порядок могу поменять на более подходящий согласно моему предпочтению. Где-то читал, что нужно соблюдать уровни абстракции.

Тесты.

1. Имя. Программы. Я не понимаю, что вы имеете в виду. Объединил все в один блок.
2. Компаратор векторов я не нашел, поэтому написал свой. Теперь узнал, что есть стандартный.
3. В тестах выделяется дерево на куче для того чтобы эмулировать работу реального алгоритма, поэтому эти тесты наиболее приближены к реальности.
4. Не смог понять какие еще тесты нужны, я нашел что при каких-то рофельных символах в тексте у меня он откидывается видимо. Если нет, то я не знаю что не так. Поэтому протещу это еще.

Корректность.

1. Не понимаю что за неверная стата. Помогите, дайте тест какой-нибудь этакий.
2. У меня вот не падает ни на чем кроме картинки. Теперь не падает вроде.
3. Что это значит.

Не закрывайте, пожалуйста, сразу этот тикет. Я хочу понять как должно быть.

[Дмитрий Лапшин (lapshin)]

Корректность: со статистикой как раз трудно подсказать, у меня любой пример отичается. Но теперь оно не ломается и поэтому ок, 14.

В тестах выделяется дерево на куче для того чтобы эмулировать работу реального алгоритма, поэтому эти тесты наиболее приближены к реальности.

Если хочется интеграционных тестов (хорошее желание!) это делается не так и, честно, мало относится к приближению к реальности.

Например, стоило бы потестировать, что происходит если некоторый набор байт сжать и разжать: если на выходе другой ответ, это явно намекает, что при использовании кода в реальности он не сработал. А если сработал это хороший знак.

По сути ты тестируешь лишь построение дерева. Это хорошо, конечно, но можно больше. 4.

Стиль:

Про чтения: мне казалось у тебя где-то было чтение в char, а он пёс знает какой знаковости. Кажется даже стреляло именно на тестах с бинарными/юникодными файлами, где есть символ с старшим битом, читается как отрицательный, обращаешься в массив, ОЙ, дальше UB. На деле разжимаешь не то или падаешь. Может не явно, а как раз из std::string или чего-то такого. Сейчас не виду char и не вижу проблем (ну кроме статистики, не знаю что с ней не так).

Дальше про стиль: в заголовке выставлен read_from_file, который там не сильно нужен (этот шаблон нужен лишь в реализации), и более того: вместо того чтобы в него передавать счётчик, можно было бы:

1. Сделать это шаблонным методом класса-состояния (и не передавать этот счётчик и файл постоянно).
2. Можно было бы просто выделить класс для бинарного ввода-вывода, если не хочется замешивать всё это в класс про само сжатие. (тем более для битового у тебя уже есть!)
3. А ещё можно было бы просто делать tell/seek)

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

Сюда же идёт struct node:

1. А зачем она в заголовке?
2. А зачем если она в заголовке у неё всё публичное, хотя внутри явно два владеющих сырых указателя?
3. Если нет умных указателей, где правило пяти?

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

В твоём стиле, например, заголовок мог бы не содержать определение node (для взятия указателя оно не нужно), или класс был бы приватный в worker, или имел бы полные спецметоды.

Вообще понятно, что в рамках реализации нормально сделать какую-то внутреннюю структуру публичной по полям, раз её снаружи всё равно не достать.

Ещё есть константы которые не очень связаны между собой в коде, когда связаны по смыслу и выражаются друг через друга (256 и 8). Есть деструктор который делает close на файл (хотя он и сам закроется) и удаляет дерево (хотя мог бы держать корень дерева явно/через unique_ptr).

Эти пункты сильно портят впечатление от кода: много оставлено мест, через которые могут вещи ломаться, хотя прикрыть их несложно. 7.

Тикет не закрываю, если ещё что-то есть обсудить.