Moodle
  1. Moodle
  2. MDL-17229

Shortanswer questiontype refactoring

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: None
    • Component/s: Questions
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      2236

      Description

      Tim, I just found that I was stupid, and my plugin would be much less in size (and easier to maintain) if I used inheritance from shortanswer instead of editing it's copy.

      Hovewer, shortanswer question isn't designed very well for inheritance, it requires many code duplication to simple tasks. There is no way question edit form can be useful when inherited.

      I would like to refactor both shortanswer classes (after we deal with MDL-17064) either by creating functions that will allow injections of new things in critical points, or by defining some options (such as additional fields in blanks and main options area) via functions/variables. It'll help me and any other maintainers of derivative question types, and It'll help Moodle too, since numerical and calculated questions are inherited from shortanswer, and there is much code duplication in the case.

      Are you interested in it?

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          This issue is just a question, you may answer "yes", or "no", or "please be more specific" or "please wait till ...." or something.

          To be more specific: I don't want to do anything drastic there, just some simple improvments. For example:
          1) replace 'question_shortanswer' with 'question_'.$this->name() in db calls (also in error notification) - it'll save a lot of trouble for the childs
          2) insert two new fill_options() calls in the save_question_options, in which all question-specific option handling can be placed
          if ($options = get_record("question_shortanswer", "question", $question->id)) {
          $options->answers = implode(",",$answers);
          $options->usecase = $question->usecase;
          if (!update_record("question_shortanswer", $options))

          { $result->error = "Could not update quiz shortanswer options! (id=$options->id)"; return $result; }
          } else {
          unset($options);
          $options->question = $question->id;
          $options->answers = implode(",",$answers);
          $options->usecase = $question->usecase;
          if (!insert_record("question_shortanswer", $options)) {
          $result->error = "Could not insert quiz shortanswer options!";
          so it will be like
          if ($options = get_record("question_shortanswer", "question", $question->id)) {
          $options->answers = implode(",",$answers);
          $this->fill_options($question, &$options);
          if (!update_record("question_shortanswer", $options)) { $result->error = "Could not update quiz shortanswer options! (id=$options->id)"; return $result; }

          } else {
          unset($options);
          $options->question = $question->id;
          $options->answers = implode(",",$answers);
          $this->fill_options($question, &$options);
          if (!insert_record("question_shortanswer", $options)) {
          $result->error = "Could not insert quiz shortanswer options!";

          so the childs can override it adding their options, instead of overriding entire save_question_options
          And similar changes in backup/restore functions too.
          (Oh, there seems to be some unaccomplished work in the base class for the question on more usable methods such as using extra_question_fields, but from TODO there it seems to be abandoned, and shortanswer overloads functions that used this framework entirely, without calling parent).

          3) One or two insert functions (as fill_options above) in the question editing form (in definition_inner(), because sequence of addElement calls is important there, for other functions you can just overload and call a parent), to allow childs to add their options, so the form becomes much more reusable in child question types.

          Is this OK? If you agree at least with some of the options above, let me know, and I'll create a subtasks for them. If not, you can just tell me that.

          Show
          Oleg Sychev added a comment - This issue is just a question, you may answer "yes", or "no", or "please be more specific" or "please wait till ...." or something. To be more specific: I don't want to do anything drastic there, just some simple improvments. For example: 1) replace 'question_shortanswer' with 'question_'.$this->name() in db calls (also in error notification) - it'll save a lot of trouble for the childs 2) insert two new fill_options() calls in the save_question_options, in which all question-specific option handling can be placed if ($options = get_record("question_shortanswer", "question", $question->id)) { $options->answers = implode(",",$answers); $options->usecase = $question->usecase; if (!update_record("question_shortanswer", $options)) { $result->error = "Could not update quiz shortanswer options! (id=$options->id)"; return $result; } } else { unset($options); $options->question = $question->id; $options->answers = implode(",",$answers); $options->usecase = $question->usecase; if (!insert_record("question_shortanswer", $options)) { $result->error = "Could not insert quiz shortanswer options!"; so it will be like if ($options = get_record("question_shortanswer", "question", $question->id)) { $options->answers = implode(",",$answers); $this->fill_options($question, &$options); if (!update_record("question_shortanswer", $options)) { $result->error = "Could not update quiz shortanswer options! (id=$options->id)"; return $result; } } else { unset($options); $options->question = $question->id; $options->answers = implode(",",$answers); $this->fill_options($question, &$options); if (!insert_record("question_shortanswer", $options)) { $result->error = "Could not insert quiz shortanswer options!"; so the childs can override it adding their options, instead of overriding entire save_question_options And similar changes in backup/restore functions too. (Oh, there seems to be some unaccomplished work in the base class for the question on more usable methods such as using extra_question_fields, but from TODO there it seems to be abandoned, and shortanswer overloads functions that used this framework entirely, without calling parent). 3) One or two insert functions (as fill_options above) in the question editing form (in definition_inner(), because sequence of addElement calls is important there, for other functions you can just overload and call a parent), to allow childs to add their options, so the form becomes much more reusable in child question types. Is this OK? If you agree at least with some of the options above, let me know, and I'll create a subtasks for them. If not, you can just tell me that.
          Hide
          Tim Hunt added a comment -

          Yes, you should be able to use extra_question_fields. The mechanism does work, I have just never had time to convert the core question types to use it. (I used in successfully in at least one contrib question type.)

          So, No to your suggestion, I am afraid.

          Show
          Tim Hunt added a comment - Yes, you should be able to use extra_question_fields. The mechanism does work, I have just never had time to convert the core question types to use it. (I used in successfully in at least one contrib question type.) So, No to your suggestion, I am afraid.
          Hide
          Oleg Sychev added a comment -

          Hello, Tim.
          "Yes, you should be able to use extra_question_fields. The mechanism does work, I have just never had time to convert the core question types to use it. (I used in successfully in at least one contrib question type.)"

          Oh, if all was that simple, I would really be glad and wouldn't bother you. I'm in no mind to improve everything there. But I'm afraid it can't. Sorry.

          I must inherit my question from shortanswer, to be able to stay in touch with it's improvments (preg uses it's interface as well, not just options handling), not from base class for all qtypes. So I must offer you my help converting shortanwer question to use extra_question_fields (or to include preg functionality in shortanswer, should you prefer it). This is not all straightforward thought: (the numbers below corresponds with numbers in my early comment):

          1) the change in table names handling should still be made in backup/restore functions - extra_question_fields mechanism doesn't work there, and it probably can't without some information about field types. Option table name can be accessed via extra_question_fields instead if applying $this->name() if you want.

          2) insert functions for options handling are still needed in backup/restore (see above).

          get_question_options and delete_question could be easily eliminated from shortanswer, using exta_question_fields instead

          but save_question_options can't, as base class lacks answers handling in this function (TODO isn't sufficient). There are several ways to deal with this:
          a) complete implementation of extra_answer_fields mechanism and eliminate save_question_options from shortanswer. I probably can't risk to offer such complex change in the core by myself, so it's up to you.
          b) add basic asnwers handling (without extra_answer_fields ) to base class function and eliminate save_question_options from shortanswer, this I can do
          c) make shortanswer call parent (to save options), and then handle answer's saving by itself, this I can do too.

          3) question editing form improvments are still needed, extra_question_fileds are no help at all there. I personally need only one insert function with empty body to add controls for question options, and some way to overload 'answerinstruct' text. But thinking globally one or two more insert function can be useful: to add things in repeated elements, and to add things after repeated elements.

          So what can you tell on this?

          Show
          Oleg Sychev added a comment - Hello, Tim. "Yes, you should be able to use extra_question_fields. The mechanism does work, I have just never had time to convert the core question types to use it. (I used in successfully in at least one contrib question type.)" Oh, if all was that simple, I would really be glad and wouldn't bother you. I'm in no mind to improve everything there. But I'm afraid it can't. Sorry. I must inherit my question from shortanswer, to be able to stay in touch with it's improvments (preg uses it's interface as well, not just options handling), not from base class for all qtypes. So I must offer you my help converting shortanwer question to use extra_question_fields (or to include preg functionality in shortanswer, should you prefer it). This is not all straightforward thought: (the numbers below corresponds with numbers in my early comment): 1) the change in table names handling should still be made in backup/restore functions - extra_question_fields mechanism doesn't work there, and it probably can't without some information about field types. Option table name can be accessed via extra_question_fields instead if applying $this->name() if you want. 2) insert functions for options handling are still needed in backup/restore (see above). get_question_options and delete_question could be easily eliminated from shortanswer, using exta_question_fields instead but save_question_options can't, as base class lacks answers handling in this function (TODO isn't sufficient). There are several ways to deal with this: a) complete implementation of extra_answer_fields mechanism and eliminate save_question_options from shortanswer. I probably can't risk to offer such complex change in the core by myself, so it's up to you. b) add basic asnwers handling (without extra_answer_fields ) to base class function and eliminate save_question_options from shortanswer, this I can do c) make shortanswer call parent (to save options), and then handle answer's saving by itself, this I can do too. 3) question editing form improvments are still needed, extra_question_fileds are no help at all there. I personally need only one insert function with empty body to add controls for question options, and some way to overload 'answerinstruct' text. But thinking globally one or two more insert function can be useful: to add things in repeated elements, and to add things after repeated elements. So what can you tell on this?
          Hide
          Tim Hunt added a comment - - edited

          Last subtask submitted for integration. Closing.

          Show
          Tim Hunt added a comment - - edited Last subtask submitted for integration. Closing.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: