H5P - SQL, Python, ....



I have created several SQL content types:

  * Based on the work of Degrange here https://h5p.org/node/1134219 I have developed a simple SQL IDE and a SQL question type. These are also based on Skulpt, also support Turtle graphics and p5.js.

   * An SQL question type compares SQL queries on a sqlite database against a result relation using sql.js.

  * Both content types are based on a codequestion content type, which should make it easier to query other languages with the same code base in the future.

Feedback would be greatly appreciated as this is the very first time I have developed H5P content types.

Demo: https://www.opencoding.de/course/view.php?id=2#section-2



otacke's picture


I'll gladly have a look when I find some time, but it was great if you checked for common issues such as violations of the H5P coding style guide that can easily be avoided.


otacke's picture


I thought: Why not. Please note that this is not a thorough code review (so there may be more that I didn't spot right away as I didn't try to understand what the code was doing) and I have not looked at the running instance, at the code only.

Starting with H5P.CodeQuestion seemed like a good starting point, although I later noticed that this was just a non-runnable library.

  • You might want to update the build chain slightly. I realize that it's from the h5p boilerplate repository (that was not updated a long time and) that has a couple of quirks.
  • Commonly, in `semantics.json` files the indentation is using 2 spaces, not 4.
  • You're releasing that as Joubel inside the LICENSE file? ;-)
  • What's the .h5p file? An accident from building libraries?
  • You should in fact check for coding violationse, e.g. mixing single quotes and double quotes, using `!=" instead of `!==`, missing semi-colons, etc. (or let eslint do so, cmp. a newer boilerplate for instance https://github.com/otacke/snordian-h5p-boilerplate)


  • H5P should be read only (line 5)


  • Strings are hardcoded and not translatable.
  • May be a matter of taste, but using member notation outside of the constructore `getDecode(code) { ... }` instead of `this.getDecodededCode = (code) => { ... };` inside the constructor feels nicer (e. g. line 20)
  • Try to make functions fail early (line 21) as you do in line 39, this often helps to understand what a function does way easier, e.g. here: ` if (!code) { return ''; } or simply sanitize `code` like code = (code typeof 'string') ? code : '';
  • Line 41 can be deleted if you change line 43 into `return this.runtime ?? this.runtime;`
  • Line 42 is code commented out.
  • Line 49: An empty function? How does calling createRuntime() work at all then? Is that defined again somewhere else?
  • Line 72 is not necessary if you define `content` on line 75 instead - and using `let` here is not required as in many other places where `const` should be used.
  • Line 76: Where does parent come from (guess I'd find out if looking at other code)? And is setting properties on it directly really a good idea? Either way, if UID is supposed to hint to a UUID, than that's good, because ids should be unique even if you have multiple instances.
  • Line 82: Why bother to store that value in a variable if you only use it in line 83? And I guess there's a reason for the hardcoded alt tag? Similar in other lines
  • Line 86: Don't be afraid of shorthands like `if (this.hasAssets)` in particular sincy you don't check for strict equality anyway
  • Line 92: I like Toto, but I could not guess what that variable is meant to be used for :-D
  • Function in Line 199 Blank lines for the win (and intelligibility).
  • Line 260: What's `var` doing in here if you're using ES6?
  • Line 311: If you want to return an object directly, you should wrap the return value in parentheses.
  • Line 329: So, here the constructor function actually starts (would be right at the top if member functions were written in class notation).
  • Line 347: Try to avoid using jQuery right away if possible - it will be removed from H5P at some point.
  • Line 369: If you need UUIDs, you can simply call H5P.createUUID()


  • Line 2: What's `width: 100%` necessary? Am only looking at the code, not a live instance, and `width: 100%` is often not necessary.
  • assigment [sic]

I then quickly glanced at some other repository and wondered: are those libraries copies of npm modules? If yes: Why bother to create H5P libraries out of them (that need their own maintenance, wrapping code, etc) if you could simply use them as dependencies?

Whoa! - Thanks!

I am very grateful for the feedback. As this is the first time I have programmed an H5P module (and to be honest, so much javascript), it helps me a lot.

As soon as I have some free time again, I will work through the feedback and update the modules.

Whoa! - Thanks!

I am very grateful for the feedback. As this is the first time I have programmed an H5P module (and to be honest, so much javascript), it helps me a lot.

As soon as I have some free time again, I will work through the feedback and update the modules.

otacke's picture

You're welcome! And don't feel intimidated. Every coder started small ...

I just with there was a way to review a whole repository like one could with pull requests. That would make things easier.

> Every coder started small ...

:-)  Glad to hear it :-D - Even though I've already done a lot of programming. But I've never been good friends with Javascript, because the approach to solve problems is often very different from what I'm used to in other languages.

For example, I wanted to declare the methods outside the constructor, but then ran into problems with `this` when I then passed these functions as callbacks in Promises, which I could only solve by using this strange way of declaring functions inside the constructor.

otacke's picture

Oh, one could notice that you've been coding before ...

Not sure what exactly your issue with `this` was, but I assume some of the usual confusion that arises if one uses `function () {}` declarations which in fact all come with their own local `this` reference. Have a look at https://github.com/otacke/snordian-h5p-boilerplate/blob/master/src/scripts/h5p-boilerplate-snordian.js for instance. Using class notation and `this` is what one expects it to be if one is used to Java or similar languages. Throw in using arrow functions only, and all is fine.

I wanted to say thank you again for your feedback. Due to illness and personal circumstances, it will take me a while to work through the feedback (it will probably be a few weeks before I find the time again) - but I will definitely use it!

otacke's picture

Don't mention it. Sometimes a review by someone else can feel weird, but if it's useful to you, I am glad.

I made a major rewrite of the plugins.I restructured some things (plugin dependencies and class structure), used the linter plugin to improve overall code quality.

The plugins can be found here https://codeberg.org/a_siebel/h5p-content-python-question and https://codeberg.org/a_siebel/h5p-content-sql-question . I produced a short introduction (language: german): https://youtu.be/Kb346xXr7f8

There is still work to do (especially translation which is still do be done) but I will now continue to test the plugin when creating Moodle courses.