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 Sergey Sysoev

Добавил тесты, вынес некоторое в private

comment:2 Changed 3 years ago by Sergey Sysoev

Добавив 2 теста получил 100% coverage huffman'а, если бы это ещё говорило что-то про качество тестов...

comment:3 Changed 3 years ago by Дмитрий Лапшин (lapshin)

Owner: changed from Дмитрий Лапшин (lapshin) to Sergey Sysoev
Type: ожидается проверкаожидаются исправления

Корректность: давай разбирать аргументы) 14.

Тесты: чот кидают и падают. А, ну всё понятно, твои тесты завязаны на место откуда вызовут программу с тестами, что печально, потому что вообще говоря собирая CMake-ом ты не знаешь где окажется относительно исходников программа. Я пока зачту, но давай попробуем придумать аккуратное решение которое не боится всего этого. Coverage какая доля кода проходится тестами, 100% это сильно и хорошо (значит нет веток кода в которые тесты не заходят), но само по себе не такая идеальная метрика. 4.

Стиль:

  1. ../src: (target_)include_directores?
  2. Инклуд .cpp это мдаааа. Я понимаю что хочется потестить, но не, вот так делать не будем) Или выставляй честно для линковки, или не лезь.
  3. Анонимный неймспейс в заголовке это интересно. Короче очень странно код разделён.
  4. В .cpp так и хочется namespace жахнуть. Кстати, когда ты уже внутри элемента неймспейса не надо его явно писать. Это сделает код попроще...
  5. Кстати переносы на типе возврата бывают, но странные.
  6. Магические числа.
  7. Я чего-то не понял что за переабстракция в read_bytes с коллбеками. Наверняка можно более простой итнерфейс (и тем более без виртуального std::function).

5.

comment:4 Changed 3 years ago by Sergey Sysoev

Корректность: не понял что я делаю не так, на всякий случай поаккуратнее разобрал аргументы (теперь нельзя 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 .)

Стиль:

  1. target_include_directores.
  2. Сделал namespace публичным, назвал internal. Не лучшее решение, но по крайней мере понятно, что руками туда лезть не надо.
  3. Предыдущий пункт.
  4. Сделал, лучше действительно стало.
  5. Я много такого видел в std и недавно начитался сорцов ReactOS, захотелось попробовать. Звучит действительно удобно, когда строчка начинается с названия функции (самого полезного), а остальное можно подглядеть вокруг (сверху и справа) + объявление функции теперь легко отличить от какой-нибудь константы. Из минусов - очень уж многострочно.
  6. Сделал получше, правда в некоторых местах стало похуже.
  7. Переделал, но теперь у BinaryReader весь private наружу торчит и легко испортить ему состояние. Callback'и там появились, потому что read нельзя просто так прекратить посреди байта, а когда дочитал - оставшуюся часть байта нужно забыть и вернуть себя в нормальное состояние. Теперь всё это на совести пользователя.
Last edited 3 years ago by Sergey Sysoev (previous) (diff)

comment:5 Changed 3 years ago by Sergey Sysoev

Owner: changed from Sergey Sysoev to Дмитрий Лапшин (lapshin)
Type: ожидаются исправленияожидается проверка

Я забыл сделать reassign...

comment:6 Changed 3 years ago by Дмитрий Лапшин (lapshin)

Owner: changed from Дмитрий Лапшин (lapshin) to Sergey Sysoev
Type: ожидается проверкаожидаются исправления

Корректность: а давай аргументы разбирать в любом порядке. 14.

Тесты: 5.

Стиль:

  1. А стандартные? const int BITS_IN_BYTE = 8;.
  2. Сырые указатели, а при этом нет правила 5.
  3. А зачем виртуальный BitReader?
  4. Ох уж этот стиль с переносом после возвращаемого типа. Но люди используют местами.
  5. Местами кажется стандартные алгоритмы могут ещё помочь.

7.

comment:7 Changed 3 years ago by Sergey Sysoev

Owner: changed from Sergey Sysoev to Дмитрий Лапшин (lapshin)
Type: ожидаются исправленияожидается проверка
Version: 3.0

Корректность: Аргументы я вроде в любом порядке разбираю...

Стиль:

  1. BitReader хочется, чтобы можно было тестировать Trie без файлов. От reader'а Trie нужна только одна функция и я вынес её в virtual, чтобы в тестах унаследоваться и заимплементить без файлов.

Остальное поправил, но после дедлайна это вряд ли на что-то влияет.

comment:8 Changed 3 years ago by Дмитрий Лапшин (lapshin)

Resolution: задача сдана
Status: assignedclosed

15/5

Стиль:

  1. Сделал бы BitReader? на stringstream и вот тебе счастье. Держал бы в себе ссылку на iostream и радуйся жизни.

9.

Note: See TracTickets for help on using tickets.