H5P - SQL, Python, ....

Hi!

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.

Cheers,
Oliver

Hey!

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)

entries/h5p-code-question.js

• H5P should be read only (line 5)

scripts/h5p-code-question.js

• 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()

styles/h5p-code-question.css

• 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?