HA#2 huffman

[Luchko Serega]

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

​http://en.cppreference.com/w/cpp/io/basic_istream/read
​http://en.cppreference.com/w/cpp/io/basic_ostream/write

​http://www.cplusplus.com/doc/tutorial/files/

[Vladimir Rutsky]

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

Это корректное использование reinterpret_cast:

uint32_t different_letters;
...
output_file.write(reinterpret_cast<char*>(&different_letters), sizeof(different_letters));

Здесь не подходит динамическое приведение типа (dynamic_cast) и не подразумевается статическое приведение типа (static_cast), здесь вы именно хотите посмотреть на область памяти, которая представлена указателем одного типа, как на массив байт --- reinterpret_cast предназначен именно для такой операции.

Замечания:

1. Папка с решением должна называться ha2, а у вас называется hw2.

2. В чем смысл объединения в класс функций, которые являются статическими методами?

struct Huffman {
    static bool encode(string ifile, string ofile);
    static bool decode(string ifile, string ofile);
};

Объект класса никак не используется и нет смысла в его инстанциации, вы можете вызывать методы как Huffman::encode(...).

3. Не используйте глобальные переменные:

uint8_t bit_buffer = 0;
uint8_t current_bit = 0;
uint32_t writtent_bytes = 0;

Да, вам нужно сохранять состояние между вызовами write_bit/get_bit.
Можно хранить его в статических переменных внутри write_bit/get_bit, но это не решает проблему с тем, что функции нужно вызывать в определённом порядке (нельзя одновременно декодировать в двух потоках, например).
Перенесите это состояние в члены класса Huffman (и сделайте методы endode/decode не статическими).

4. Передавайте объекты, которые не планируете модифицировать по константной ссылке:

bool Huffman::encode(string const & ifile, string const & ofile) {
...
Tree* huffman_algorithm(map<uint8_t, uint32_t> const & letters_frequency,
                        map<uint8_t, string>& codes_map) {
...
bool operator() (Tree const * a, Tree const * b)
...

5. Не используйте using namespace std; в заголовочном файле --- в противном случае вы "загрязняете" глобальную область видимости для всех пользователей вашего заголовочного файла. Используйте using namespace std; в *.cpp файлах (или в своей области видимости).

6. Используйте тип size_t для индексов и размеров:

for(int i=0; i < cur_code.length(); i++) {

7. huffman_algorithm не возвращает значения, если вызывается от пустого набора символов. На практике этого не проиходит, т.к. вы не вызываете этот метод на пустом наборе, но компилятор выдаёт предупреждение.

8. При кодировании/декодировании пустого файла вы выводите, что доп. данные занимают 8 байт, хотя вы их не записываете.

9. std::priority_queue не гарантирует, что при добавлении элементов с одним приоритетом они будут извлечены в каком-то определённом порядке, поэтому теоретически ваше решение может строить различные деревья при кодировании и декодировании, если у каких-то символов одинаковый частота встречаемости, при приведёт к ошибчному декодированию.

Сделайте сравнение в Compare стабильным.

10. Вместо -output должен поддерживаться флаг --output.

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

12. Ваше решение очень похоже на решение Александра Лучко.

Исправьте, пожалуйста, в течение 60 часов.

[Luchko Serega]

Здравствуйте.
1) Папка с решением должна называться ha2, а у вас называется hw2.
Решено
2,3) Не используйте глобальные переменные:
Перенесите это состояние в члены класса Huffman (и сделайте методы endode/decode не статическими).
Сделал
4) Передавайте объекты, которые не планируете модифицировать по константной ссылке:
Сделал
5) Не используйте using namespace std; в заголовочном файле --- в противном случае вы "загрязняете"
Сделал
6) Используйте тип size_t для индексов и размеров:
Поправил
7) huffman_algorithm не возвращает значения, если вызывается от пустого набора символов.
На практике этого не проиходит, т.к. вы не вызываете этот метод на пустом наборе, но компилятор выдаёт предупреждение.
Поправил
8) При кодировании/декодировании пустого файла вы выводите, что доп. данные занимают 8 байт, хотя вы их не записываете.
Сделал
9) std::priority_queue не гарантирует, что при добавлении элементов с одним приоритетом они будут извлечены
в каком-то определённом порядке, поэтому теоретически ваше решение может строить различные деревья
при кодировании и декодировании, если у каких-то символов одинаковый частота встречаемости, при приведёт
к ошибчному декодированию.
Добавил поля accumulator в класс Tree, теперь поддерево помнить все символы в себе, и два поддерева можно уникально
отличить используя это переменную, даже если частоты у них совпали.
10) Вместо -output должен поддерживаться флаг --output.
Сделал
11) Нет необходимости сохранять количество символов в исходном файле при сжатии
--- вы можете его вычислить просуммировав количества каждого встречающегося символа.
Сделал
12) Ваше решение очень похоже на решение Александра Лучко.
Да, мы с ним живём в одной комнате. И когда писали код, то объяснили друг другу что как можно решить.
Ссылки на ввод вывод, на написание бит в стаковерфлов юзали те же. Да и на вывод.
=======================
Попытался успеть в 60 часов.

[Vladimir Rutsky]

Замечания:

1. Заголовочный файл должен быть самодостаточным: в huffman.hpp вы используете std::ofstream, std::ifstream и другие типы. Плюс вы неявно предполагаете, что пользователь huffman.hpp делает using namespace std; --- вы используете std::uint8_t и аналогичные типы без указания std.

Исправьте, пожалуйста, в ближайшее время.

[Luchko Serega]

Я добавил std к типам. Но я не понял первого предложения.

Заголовочный файл должен быть самодостаточным: в huffman.hpp вы используете std::ofstream, std::ifstream и другие типы. (Надеюсь эти типы можно использовать, не понял как они на самодостаточность влияют.)

[Vladimir Rutsky]

Сергей, под самодостаточностью заголовочного понимается в частности то, что файл можно включить в "пустой" main.cpp, и он скомпилируется (т.е. не будет ошибок, вроде неизвестый класс std::ofstream, или неизвестный тип uint8_t).

[Luchko Serega]

Спасибо. Понял. Поправил.

[Vladimir Rutsky]

Решение зачтено.

[Vladimir Rutsky]

Milestone ha2-deadline deleted