Рефакторинг adaptive_hints
Issue #364
resolved
Изменение методов для рендеринга подсказок и их использования в correctwriting и preg
Comments (57)
-
reporter
-
repo owner
Там помимо упрощения перебора хинтов и для рендеринга кнопок хинтов возможно понадобятся, но кнопки это мелочи.
Причем нужно чтобы эти методы были доступны другим поведениям (по рисованию кнопок например), не являющихся потомками адаптивного. Но вы сначала таки расчертите и усвойте связи между классами поведения, вопроса и подсказок - вы же учили моделирование ПО? Потому что рендереры хинтов и рендереры вопросов как раз напрямую почти не связаны, ваш вопрос показывает что вы не разобрались во взаимодействии классов модулей. Да посмотрите классы question_attempt/question_attempt_step и разберитесь как в них хранятся переменные, которые принадлежат поведению - и какие методы есть для доступа к ним и поиска подходящих шагов выполнения попытки, вам это очень даже пригодится ибо общение поведения с рендерером вопроса происходит именно через эти переменные...
Вы способны как уже без года профессионал проанализировать код, разобраться в классах и предложить свое решение?
-
reporter
"не являсь потомками", значит что они будут их вызывать из какого-то класса?
На какие классы мне еще стоит обратить внимание?
Я очень на это надеюсь :)
-
repo owner
Значит что будут еще поведения которым нужно тоже делать кнопки, но классы которых не будут наследовать adaptive. В вашей же группе их и разрабатывают. Но вы с переменными и отображением хинтов разберитесь... Кнопки - это по мелочи на конец...
-
reporter
Итого у меня 2 задачи:
1 Вынести метод рендеринга кнопок из qbehaviour_adaptivehints_renderer (раскиданный по 2 методам) и предоставить использование этого метода другим классам (возможно не только этот метод)
Для решения этой проблемы можно создать класс - помощник со статическим методом
render_hint_button(question_attempt $qa, question_display_options $options)2 Избежать дублирования кода и сделать некоторые усовершенствования для question_attempt
Для решения этой задачи могу предложить добавить в класс-помощник функции, расширяющие возможности question_attempt. Главным образом это будет функция возвращающая подсказки с заданным префиксом.
Для сравнения сейчас используется код:
foreach ($hints as $hintkey) { if ($qa->get_last_step()->has_behaviour_var('_render_'.$hintkey)) { $hintobj = $question->hint_object($hintkey); ...После рефакторинга он сократится до:
foreach (prefix_helper::$get_hints_with_prefix('_render_', $qa) as $hintkey) { $hintobj = $question->hint_object($hintkey); ... -
repo owner
Я по второму пункту жду конкретных предложений по эффективному алгоритму подобной функции. Будет ли она обращаться к БД, к данным в памяти $qa, хранить свои...
Кроме того посмотрите какие задачи решает само поведение относительно шагов. Там тоже есть циклы перебора. Причем иногда ищется последний шаг с каким-либо хинтом или еще что-нибудь в этом роде...
-
reporter
Функция has_behaviour_var у question_attempt_step обращается к данным в памяти.
Конкретно has_behaviour_var ищет в массиве методом array_key_exists, которая возвращает ответ за O(1).
Можно проходить последовательно по массиву или вызывать has_behaviour_var, реальные изменения в скорости можно ожидать только при размере больше 10^6 (в пользу хэша, считая что нужных нам данных несравненно меньше чем размер массива) , но на сравнение строк или создание хеша в случае array_key_exists тратится несравнимо больше времени.
Итог: можно использовать has_behaviour_var.
get_last_step также быстро работает (возвращает последний элемент массива или специальный класс нулевого шага)
-
repo owner
Я слабо представляю как вы собираетесь реализовать get_hints_with_prefix('render', $qa) (кстати а почему hints, а не behaviour var?) с такими параметрами через has_behaviour_var. Вы же знаете только префикс, но не возможные его продолжения, а has_behaviour_var требует полных имен переменных.
Весь фокус бы в том, что нам надо уйти от получения всех вариантов hintkey и проверки нет ли их переменных случайно к тому, чтобы перебрать все переменные и проверить нет ли среди них с нужным префиксом. Если бы это была БД можно было бы использовать запрос с условием типа LIKE; но у нас уже массив...
-
reporter
Конкретно это будет скрыто внутри реализации.
Возможно, название get_behaviour_var_with_prefix будет правильней.
Но мы обязательно будем использовать все хинты, касающиеся рендера? Если нет, то это наоборот лишняя трата времени.
Если да, то несложно пройти по массиву и проверить каждый, но сильного выигрыша от этого не будет (на мой взгляд ни в плане понятности, ни в плане скорости)
-
repo owner
Генерация списка всех хинтов может быть затратным вариантом, включая трудоемкий подбор правильного ответа и вычисление ошибок - посмотрите как это делает CorrectWriting например. Поэтому желательно было бы обойтись без получения такого списка и перебирать все переменные вместо этого, ища нужный префикс - но это нужно реализовать максимально эффективно...
-
reporter
На данный момент я не знаю способа проверить быстрее, чем перебрать все элементы и вызвать у каждого strstr
Точнее знаю, но для одноразового использования этого будет избыточно. Плюс затраты на память и построение. Учитывая небольшой (я так предполагаю) размер массива
-
repo owner
Кешировать результат может быть полезно. Если вы разобрались, как работает бехайвор и рендерер вопроса, попробуйте оценить сколько раз за генерацию одной страницы может вызываться такой метод с разными префиксами. Там ведь бехайвор тоже часто перебирает переменные в функциях типа process_XXX - и там другие префиксы могут быть.
-
reporter
Кеширование или сохранение упорядоченного массива (можно будет делать поиск относительно быстро даже для префиксов) может ускорить дело. О каком размере массива мы говорим?
-
repo owner
Я уже сказал - максимальное количество хинтов дает CorrectWriting. Создайте вопросы, сделайте ответы с ошибками и поэкспериментируйте... Можете заодно и количество переборов отладкой подсчитать. Давайте работать, а не только на меня надеяться...
-
reporter
Хорошо =)
Про "максимальное количество хинтов дает CorrectWriting" не слышал.
Я сейчас попробую.
-
reporter
Создал несколько вопросов, хинты к ним. Дебаггером посчитал количество вызовов - получилось 20 - 100, в зависимости от количества подсказок. По-моему это достаточно малое количество, чтобы стоило оптимизировать.
-
repo owner
Надо смотреть на алгоритм, вычисляющий кешируемые данные. Если он линейный по сложности и не требует обращений к БД, тогда можно не кешировать. Если каждый раз идет обращение к БД или алгоритм имеет сложность как минимум квадратичную - при 20-100 вызовах для генерации одной страницы кешировать стоило бы...
-
repo owner
И я жду таки окончательных предложений по интерфейсу в виде заголовков методов классов с PHPDoc комментариями, объясняющими их работу (и указанием классов, куда их планируется разместить).
-
reporter
- Алгоритм имеет почти константную сложность (там поиск в хеш-массиве). Обращений к БД в нем не увидел.
- Предложения по поводу интерфейса:
class qbehaviour_render_helper { /* * Renders hint button. Could be used by behaviour or question renderer to avoid code duplication while rendering it. * @param hintobj object an object of a child of qtype_specific_hint class */ static public function render_hint_button(question_attempt $qa, question_display_options $options, $hintobj) { /* * Copy method qbehaviour_adaptivehints_renderer::render_hint_button */ } } class qtype_render_helper { /* * Search behaviour vars from last question step with given prefix * @return array of names behaviour vars without prefix */ static public function get_behaviour_vars_with_prefix(string $prefix, question_attempt $qa) { /* * Fill aray of vars if $qa->get_last_step()->has_behaviour_var($prefix.$hintkey) return true, * where $hintkey is element of hints array for this question attempt */ } -
repo owner
Метод get_behaviour_vars_with_prefix может использоваться не только из qtype - но и из класса поведения (не рендерера, а базового класса) - посмотрите особенно класс adaptivehints (но и interactivehints тоже). Если мне не изменяет память, там далеко не всегда речь идет о переменных именно последнего шага. И может даже встать задача найти последний шаг в котором была хоть одна переменная с заданным префиксом. (Также обратите внимание на использование префиксов для работы с multipe instance hints, особенно последовательными - но и параллельными тоже. Не всегда префикс будет одинаковый.) Проанализируйте внимательно ВСЕ циклы.
А render_hint_button - сюрприз - нужна не только поведению, там CorrectWriting их вполне из своего рендерера делает - посмотрите внимательно.
P.S. В Moodle не очень принято использование статических функций, они предпочитают создавать объекты и вызывать динамические, даже без состояния объекта - так лучше с наследованием бывает. В случае qtype_render_helper например объект может получать $qa в конструкторе, она одна на все будет...
-
reporter
По поводу стат. функций и различных вариантов префикса понял, по поводу последнего посмотрю.
По поводу размещения есть вопрос. Где обычно в moodle расположены такие вещи, как эти хелперы? То есть их же нельзя по логике сопоставить qtype или qbehaviour. Значит должно быть место, где лежат более общие вещи.
-
repo owner
Пока базовые классы хинтов лежат в poasquestion/classes - вы это могли видеть. Можно все базовое размещать там же... Только соблюдая правила Classes Autoloading...
-
reporter
Следующая итерация:
-
Если я правильно понимаю, надо брать все хинты "starting from the prefixes from save_hint_options" для параллельных multipe instance hints
-
Для последовательных меняется суффикс, про префикс ничего не нашел
-
В adaptive hints вроде используются только последний шаг, если что-то ищется с префиксом
class qtype_poasquestion_render_helper { public question_attempt $qa; public question_display_options $options /* * Renders hint button. Could be used by behaviour or question renderer to avoid code duplication while rendering it. * @param hintobj object an object of a child of qtype_specific_hint class */ public function render_hint_button($hintobj) { /* * Copy method qbehaviour_adaptivehints_renderer::render_hint_button */ } } class qtype_poasquestion_render_helper { public question_attempt $qa; /* * Search behaviour vars from last question step with given prefix * @return array of names behaviour vars without prefix */ public function get_behaviour_vars_from_last_step_with_prefix(string $prefix); /* * Search last entry of behaviour var from question with given prefix * @return name of behaviour var without prefix */ public function get_last_behaviour_var_with_prefix(string $prefix); /* * Search last question step from question where is behaviour var * with given prefix * @return question step */ public function get_last_question_step_with_prefixed_behaviour_var(string $prefix); } -
-
reporter
Прочитал старое сообщение:
"Вы способны как уже без года профессионал проанализировать код, разобраться в классах и предложить свое решение?"
Считаю, что профессионалом смогу называть себя лет через 5-6 активной работы на крупном предприятии, а пока что нет.
Да и вообще, в своей жизни видел очень немногих профессионалов, и то издалека.
-
repo owner
Да вы в бехейворе не рендерер смотрите, а сам основной класс бехейвора. А то у вас одни рендерер-хелперы.
Вот например не последний шаг с довольно утомительным перебором: https://bitbucket.org/oasychev/moodle-plugins/src/54f351ea235bee0dd6b876077873d853c21dd79f/question/behaviour/adaptivehints/behaviour.php?at=default&fileviewer=file-view-default#behaviour.php-71
Учтите что Лена например должна писать аналогичный класс для другого бехейвора. Так что устранять дублирование кода между ними надо по максимуму.
-
repo owner
И давайте без эмоций :) Вообще-то профессионал этот тот, кто зарабатывает себе на жизнь тем или иным занятием (сравните классификацию профессиональный/любительский у певцов, артистов и т.д. - там где любительство развито как таковое). Так что если вы уже устроились на работу, то вы вполне подходите под определение прямо сейчас...
-
reporter
Да, я видел этот пример. Собственно поэтому появилась
/* * Search last entry of behaviour var from question with given prefix * @return name of behaviour var without prefix */ public function get_last_behaviour_var_with_prefix(string $prefix);А с названием погорячился, да. Рендер там необязателен. Да и второй класс вообще не нужен. Их вполне можно объединить.
Еще можно подумать над функцией для суффиксов.
-
repo owner
Главный вопрос теперь - как методы qtype_poasquestion_render_helper будут реализовываться; через какие методы/свойства они получат доступ к нужным данным из question_attempt.
Кроме того, прочитайте в девелоперской документации Мудла данные на автозагрузку классов (сейчас классы хинтов в poasquestion уже через нее работают) и сделайте их частью соответствующего пространства имен.
Ну и конструкторы вы в классы как-от забыли дописать...
Еще есть вопрос про get_behaviour_vars_from_last_step_with_prefix - "Last" это точно именно характеристика метода? Может быть она из любого шага например будет получать информацию, а уж последний шаг ей передать не трудно. Это если имеется ввиду именно вообще последний; а не последний с какой-либо из указанных переменных...
-
reporter
Методы qtype_poasquestion_helper будут реализовываться через:
question_attempt::get_num_steps() question_attempt::get_step($i) question_attempt::get_last_step() question_attempt::get_last_step_with_behaviour_var($name) или перебор степов итератором question_attempt::get_reverse_step_iterator() question_attempt_step::has_behaviour_var($name)Для автозагрузки же нужен только префикс - qtype_poasquestion_<class name>
Конструктор добавлю.
По поводу last - можно сделать 2 метода, один для последнего шага, другой для шага с заданным номером.
Итого:
class qtype_poasquestion_helper { public question_attempt $qa; public question_display_options $options; public function __construct(question_attempt $qa, question_display_options $options); /* * Renders hint button. Could be used by behaviour or question renderer * to avoid code duplication while rendering it. * @param hintobj object an object of a child of qtype_specific_hint class */ public function render_hint_button($hintobj) { /* * Copy method qbehaviour_adaptivehints_renderer::render_hint_button */ } /* * Search behaviour vars from given question step with given prefix * @return array of names behaviour vars without prefix */ public function get_behaviour_vars_from_step_with_prefix(integer $step_number, string $prefix); /* * Search behaviour vars from last question step with given prefix * @return array of names behaviour vars without prefix */ public function get_behaviour_vars_from_last_step_with_prefix(string $prefix); /* * Search last entry of behaviour var from question with given prefix * @return name of behaviour var without prefix */ public function get_last_behaviour_var_with_prefix(string $prefix); /* * Search last question step from question where is behaviour var * with given prefix * @return question step */ public function get_last_question_step_with_prefixed_behaviour_var(string $prefix); } -
repo owner
Очевидно, что question_attempt_step::has_behaviour_var($name) и подобные функции, получающие конкретные имена переменных не позволят вам перебирать имена с произвольными префиксами не зная списка их продолжений.
-
reporter
Так мы же имеем список доступных продолжений?
Можно конечно и втупую перебирать...
-
repo owner
Желательный фокус был бы как раз обойтись без списка (которого я в параметрах ваших функций не наблюдаю). Но для этого надо получить доступ к оригинальному массиву данных или лезть в БД (если получение шага или запрос последнего шага с переменной все равно туда лезет).
Или как минимум перебирать оригинальные массивы со всеми переменными. Запросами, дающими переменную по имени вы тут не справитесь.
-
reporter
У этих классов есть методы загрузки из БД, потом они используют локальные данные (как я понял).
Префиксы брать можно из question_attempt.
Значит перебирать оригинальные массивы и проверять на префикс.
-
repo owner
Или в момент загрузки из БД дополнительно фильтровать нужное, если это можно сделать без увеличения количества запросов и только когда надо (при требуемом поведении). Оттуда очень красиво отфильтровывается через LIKE по префиксу...
-
reporter
Да, я попробую посмотреть. Это будет достаточно удобно.
Но как потом хранить всю эту информацию, там много всего. Ведь запросы приходят не в момент загрузки, и нельзя же хранить список для каждого префикса и шага.
В данном случае можно конечно обойтись парой заранее заданных префиксов, но это костыли.
-
repo owner
Сначала распишите процесс загрузки - что и в какой момент загружается, какие данные на этот момент (тип поведения? тип вопроса?) известны. Если он сложный - может быть нужна sequence диаграмма. Тогда можно будет подумать что и когда...
-
reporter
Сделал форк для correctwriting.
Поправил замечания с предыдущей пары.
Сделал тесты. Инструкция по тестированию состоит из запуска юнит-тестов.
Можно проверить.
https://bitbucket.org/ShadowGorn/moodle-plugins-preg-penskoy https://bitbucket.org/ShadowGorn/moodle-plugins-correctwriting-penskoy
Для тестирования рендеринга кнопок нужны тесты или behat. Я этим займусь.
-
repo owner
Вы похоже не поняли принципа разработки "каждый плагин в своем клоне". В correctwriting вы коммитите только изменения в CorrectWriting, там поведения и Preg не трогаем. В Preg - поведения и Preg. "Чужие" плагины находятся в клонах в стабильном состоянии последнего и меняться не должны, или будут проблемы при мержинге в репозиторий релизов (корневой). Рабочая версия собирается из нескольких репозиториев, напрямую они в Moodle не ставятся...
-
repo owner
По тестам - не годится. Ваши тесты проверяют новые функции, но не проверяют корректность апгрейда самих вопросов и поведений. Нужно либо писать walkthtought тесты для поведений, либо расписывать ручные инструкции для тестирования поведения.
-
reporter
Хм.. а почему это моя забота? Я не до конца знаю все аспекты работы поведений, и не могу написать тесты для этого. Это совсем не моя область. Я там подправил маленький кусочек и должен бы проверить себя этими тестами.
-
repo owner
"а почему это моя забота?" - Вы меняете код в поведениях и рендерерах вопросов - у вас должны быть либо ручные, либо автоматизированные тесты проверить - правильно ли вы его поменяли.Или вы думаете что нельзя внести ошибки по ходу рефакторинга? Вы хоть запускали CorrectWriting, Preg - проверяли, как подсказки в них выдаются, нормально или нет?
-
reporter
Вы похоже не поняли принципа разработки "каждый плагин в своем клоне".
В каждом клоне находятся изменения только в него.
В preg оставалась ни на что не указывающая ветка со старыми коммитами (битбакет не удалил). Сейчас ее нет.
-
reporter
да, запускал. Но все аспекты проверить никак. Могу только визуально увидеть, что кнопочка есть и все.
Мне именно это и показать в инструкции по тестированию?
-
repo owner
1) У вас в https://bitbucket.org/ShadowGorn/moodle-plugins-correctwriting-penskoy/commits/all есть два коммита в poasquestion, а для poasquestion и behaviours клон разработки - Preg. Так исторически сложилось.
2) Помимо того, что кнопочка есть вы должны еще убедится что после ее нажатия хинт показывается. И если нажато несколько кнопочек хинтов по очереди - они все показываются, пока не изменится ответ (нажата кнопка проверки). Чтобы увидеть кнопки хинтов у CorrectWriting надо пропустить, добавить лишнюю или переместить лексему.
-
reporter
Хм. По поводу расположения в poasquestion 2 выхода:
-
Не трогать correctwriting, так как это не было моей задачей
-
Найти новое местоположение hint_helper
Так как изначально вы предполагали, что находиться он будет в poasquestion, я его туда разместил. О тонкостях, принятых у вас я не знал. Был бы очень признателен, если бы вы сразу поясняли свои комментарии. Есть ли идеи, где может располагаться hint_helper в correctwriting? (До этого получается, что correctwriting не работал бы без adaptivehints?)
По поводу тестов.
Данных шагов будет достаточно для проверки? Мне в инструкции по тестированию это текстом указывать или картинками?
-
-
repo owner
Да все с кодом нормально располагается. Просто изменения в каталоге correctwriting должны комиттится в клон CorrectWriting. Изменения в poasquestion должны комитттится в клон Preg и только туда. Итоговая рабочая версия CorrectWriting например собирается из разных каталогов 3х клонов (CorrectWriting, Preg, FormalLangs). Так по группам разработчиков и срокам релизов сложилось исторически в проекте. На poasquestion который лежит в CorrectWriting вы вообще внимания не обращаете, это устаревшая версия которая нигде не используется и вызвана только клонированием от центрального репозитория при переходе на Битбакет.
Инструкция по тестированию пишется текстом, им вполне легко и коротко описывается что должен тестировщик увидеть после нажатия кнопки и т.д.
-
reporter
Понятно. Если сам correctwriting будет нерабочий, это нормально.
С тестами тоже понятно, спасибо.
Тогда надо доделать тесты и удалить 2 коммита из клона correctwriting.
-
repo owner
Да.
-
reporter
Еще вопрос. Инструкцию по тестированию прикрепить к этому issue или предоставить иным образом?
-
repo owner
В иссью написать текстом в комментарии; также она входит в пояснительную записку. Читайте уже на сайте материалы...
-
reporter
Для тестирования изменений, внесенных коммитом https://bitbucket.org/ShadowGorn/moodle-plugins-correctwriting-penskoy/commits/0f64b3209f365bdd085933c16c650f0fe0752aee необходимо:
-
создать тест c adaptive behaviour и внутри него вопрос correctwriting с несколькими подсказками,
-
начать тест и дойти до созданного вопроса,
-
увидеть кнопку с хинтом,
-
нажать на кнопку,
-
увидеть подсказку,
-
повторить пункты 4-5 еще раз
Для тестирования изменений, внесенных коммитом https://bitbucket.org/ShadowGorn/moodle-plugins-preg-penskoy/commits/9e648620d632727124e801d4a49764c546cbb58f необходимо:
-
создать тест c adaptive behaviour и внутри него вопрос preg с ответом из нескольких слов,
-
начать тест и дойти до созданного вопроса,
-
увидеть кнопки с хинтом следующего слова и следующего символа,
-
нажать на кнопку хинта следующего слова,
-
увидеть подсказку,
-
заполнить ответ подсказкой и добавить через пробел лишнюю букву,
-
нажать на кнопку хинта следующего символа,
-
повторить пунткы 4-7 еще раз.
Для тестирования изменений, внесенных коммитом https://bitbucket.org/ShadowGorn/moodle-plugins-preg-penskoy/commits/807b46ef25d6bee425cf88c7597ad00255bb64c0 необходимо:
- Запустить phpunit тесты из коммита https://bitbucket.org/ShadowGorn/moodle-plugins-preg-penskoy/commits/254d15f3880a5ad2b41f0aa272f62c5bd0700294.
-
-
repo owner
Завтра покажете в деле.
-
reporter
Олег Александрович, я нашел почему не показывалось название в how to fix
В методе qtype_correctwriting_response_mistake.token_descriptions было:
// TODO should we check "has_description" or just add quoted value instead? if (is_object($this->stringpair->correctstring()) && $this->stringpair->correctstring()->has_description($answerindex)) {Если (как указано в TODO) убрать проверку:
// TODO should we check "has_description" or just add quoted value instead? if (is_object($this->stringpair->correctstring())) {то название показывается.
Что мне с этим делать?
-
reporter
В adaptive mode рисунки с подсказками при "hint from teacher" не показываются (до рефакторинга тоже).
Если в процессе adaptivehints.process_hint добавить все подсказки в показываемые:
-
preg покажет все
-
correctwriting покажет все, если hint вызывается после хоть одного check (иначе падает, так как ошибка (которую он должен показать) не определена)
Предполагаю, что в
$hintkey = $this->adjust_hintkey($hintkey); if ($pendingstep->has_behaviour_var($hintkey.'btn')) { $result = $this->process_hint($pendingstep, $hintkey); }берутся только подсказки с кнопками, которыми генерируемые хинты (типа whatis_) не являются.
Как решить проблему - не знаю.
-
-
repo owner
Для начала в самом режиме interactive все работает нормально и показывается? Там используется класс https://bitbucket.org/oasychev/moodle-plugins-preg/src/6e7bcaf940cb02e291676f94f7e669374a753560/question/type/poasquestion/classes/moodle_hint_adapter.php?at=default&fileviewer=file-view-default Чтобы понять - посмотрите на содержимое $question->hints ($this->question есть например в любом классе хинта) - там по идее объекты этого класса должны быть.
Видимо его использование недоделали в классе https://bitbucket.org/oasychev/moodle-plugins-preg/src/6e7bcaf940cb02e291676f94f7e669374a753560/question/type/poasquestion/classes/hintmoodle.php?at=default&fileviewer=file-view-default где он только вызывает format_hint (который по сути format_text) для показа текста. А надо бы еще из адаптера взять hintkeys и наделать необходимых объектов...
-
reporter
Да, в interactive все работает.
Я понял, попробую завтра сделать по совету.
-
repo owner
- changed status to resolved
- Log in to comment
Я правильно понимаю, что существующие методы и их реализация вас устраивают?
Мне необходимо создать функции, которые будут упрощать перебор возможных хинтов, не занимая место в их рендере.
Какие методы qbehaviour_adaptivehints_renderer класса наверняка будут использоваться в подобных классах (я так понимаю, нужен будет класс-посредник между adaptive_hints от moodle и конкретными реализациями в данных плагинах)