Opened 3 years ago
Closed 3 years ago
#691 closed ожидается проверка (задача сдана)
HW #2 (Huffman) sysoev.sergej
Reported by: | Sergey Sysoev | Owned by: | Дмитрий Лапшин (lapshin) |
---|---|---|---|
Component: | HW #3 (Huffman) | Version: | 3.0 |
Keywords: | Cc: |
Description
Доделал базовую функциональность.
Почему-то несколько функций отказались выноситься из .h файла (если выношу реализацию в .cpp CLion не ругается, но не компилируется потому что declaration without definition). Тесты пока не сделал, может быть ещё успею до проверки
Change History (8)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
Добавив 2 теста получил 100% coverage huffman'а, если бы это ещё говорило что-то про качество тестов...
comment:3 Changed 3 years ago by
Owner: | changed from Дмитрий Лапшин (lapshin) to Sergey Sysoev |
---|---|
Type: | ожидается проверка → ожидаются исправления |
Корректность: давай разбирать аргументы) 14.
Тесты: чот кидают и падают. А, ну всё понятно, твои тесты завязаны на место откуда вызовут программу с тестами, что печально, потому что вообще говоря собирая CMake-ом ты не знаешь где окажется относительно исходников программа. Я пока зачту, но давай попробуем придумать аккуратное решение которое не боится всего этого. Coverage какая доля кода проходится тестами, 100% это сильно и хорошо (значит нет веток кода в которые тесты не заходят), но само по себе не такая идеальная метрика. 4.
Стиль:
../src
:(target_)include_directores
?- Инклуд
.cpp
это мдаааа. Я понимаю что хочется потестить, но не, вот так делать не будем) Или выставляй честно для линковки, или не лезь. - Анонимный неймспейс в заголовке это интересно. Короче очень странно код разделён.
- В
.cpp
так и хочетсяnamespace
жахнуть. Кстати, когда ты уже внутри элемента неймспейса не надо его явно писать. Это сделает код попроще... - Кстати переносы на типе возврата бывают, но странные.
- Магические числа.
- Я чего-то не понял что за переабстракция в
read_bytes
с коллбеками. Наверняка можно более простой итнерфейс (и тем более без виртуальногоstd::function
).
5.
comment:4 Changed 3 years ago by
Корректность: не понял что я делаю не так, на всякий случай поаккуратнее разобрал аргументы (теперь нельзя 2 раза mode задать например) + сделал с string_view как вы другим советовали.
Тесты: странно, что не работало, я вроде постарался нормально сделать.
До этого я из source ресурсы для тестов копировал в папку с программой с помощью
add_custom_command(TARGET hw_02_test POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_directory "${PROJECT_SOURCE_DIR}/test/resources" $<TARGET_FILE_DIR:hw_02_test>)
Сделал немного по-другому, может так будет лучше:
file(COPY test/resources DESTINATION .)
Стиль:
- target_include_directores.
- Сделал namespace публичным, назвал internal. Не лучшее решение, но по крайней мере понятно, что руками туда лезть не надо.
- Предыдущий пункт.
- Сделал, лучше действительно стало.
- Я много такого видел в std и недавно начитался сорцов ReactOS, захотелось попробовать. Звучит действительно удобно, когда строчка начинается с названия функции (самого полезного), а остальное можно подглядеть вокруг (сверху и справа) + объявление функции теперь легко отличить от какой-нибудь константы. Из минусов - очень уж многострочно.
- Сделал получше, правда в некоторых местах стало похуже.
- Переделал, но теперь у
BinaryReader
весь private наружу торчит и легко испортить ему состояние. Callback'и там появились, потому что read нельзя просто так прекратить посреди байта, а когда дочитал - оставшуюся часть байта нужно забыть и вернуть себя в нормальное состояние. Теперь всё это на совести пользователя.
comment:5 Changed 3 years ago by
Owner: | changed from Sergey Sysoev to Дмитрий Лапшин (lapshin) |
---|---|
Type: | ожидаются исправления → ожидается проверка |
Я забыл сделать reassign...
comment:6 Changed 3 years ago by
Owner: | changed from Дмитрий Лапшин (lapshin) to Sergey Sysoev |
---|---|
Type: | ожидается проверка → ожидаются исправления |
Корректность: а давай аргументы разбирать в любом порядке. 14.
Тесты: 5.
Стиль:
- А стандартные?
const int BITS_IN_BYTE = 8;
. - Сырые указатели, а при этом нет правила 5.
- А зачем виртуальный
BitReader
? - Ох уж этот стиль с переносом после возвращаемого типа. Но люди используют местами.
- Местами кажется стандартные алгоритмы могут ещё помочь.
7.
comment:7 Changed 3 years ago by
Owner: | changed from Sergey Sysoev to Дмитрий Лапшин (lapshin) |
---|---|
Type: | ожидаются исправления → ожидается проверка |
Version: | → 3.0 |
Корректность: Аргументы я вроде в любом порядке разбираю...
Стиль:
BitReader
хочется, чтобы можно было тестировать Trie без файлов. От reader'а Trie нужна только одна функция и я вынес её в virtual, чтобы в тестах унаследоваться и заимплементить без файлов.
Остальное поправил, но после дедлайна это вряд ли на что-то влияет.
comment:8 Changed 3 years ago by
Resolution: | → задача сдана |
---|---|
Status: | assigned → closed |
15/5
Стиль:
- Сделал бы BitReader? на stringstream и вот тебе счастье. Держал бы в себе ссылку на iostream и радуйся жизни.
9.
Добавил тесты, вынес некоторое в private