Skip to content

Commit

Permalink
Add helper H5P.isEmpty
Browse files Browse the repository at this point in the history
  • Loading branch information
@Languafe
Languafe committed Sep 20, 2023
1 parent 51601da commit 6217d57
Showing 1 changed file with 29 additions and 0 deletions.
29 changes: 29 additions & 0 deletions js/h5p.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2123,6 +2123,35 @@ H5P.trim = function (value) {
// So should we make this function deprecated?
};

/**
* Recursive function that detects deep empty structures.
*
* @param {*} value
* @returns {bool}
*/
H5P.isEmpty = value => {

This comment has been minimized.

Sign in to view

Copy link
@otacke

otacke Sep 28, 2023

Contributor

@Languafe I checked that new function in H5P core. Just suggestions:

H5P.isEmpty = value => {
  if (!value && value !== 0 && value !== false) {
    return true; // undefined, null, NaN, and empty strings.
  }
  if (Array.isArray(value)) {
    return value.every(H5P.isEmpty); // Check if all elements in the array are empty.
  }
  if (typeof value === 'object') {
    return Object.values(value).every(H5P.isEmpty); // Check if all values in the object are empty.
  }
  return false; // All other cases
};

Using Array.prototype.every feels more concise, even though there's nothing wrong with the for-loop.

I also wonder about using bool as the type in the JSDoc comment. Why not boolean (which is what typeof true or typeof false would return (as the primitive data type) and what's commonly used throughout H5P code)?

Yes, I have too much time, obviously :-D

This comment has been minimized.

Sign in to view

Copy link
@icc

icc Sep 28, 2023

Member

What's interesting here is that the suggested improvement(using glorified loops) takes up around 40-50 % less space. However, the performance of the old code is about 40 % faster than the new suggestion and uses less memory. But this should only matter if you are processing larger amounts of data. In general, the new suggestion should be the better option for H5P, relying on native features, unless we are passing in large objects of course.

And yes, you are correct, it should be boolean and not bool :-)

This comment has been minimized.

Sign in to view

Copy link
@otacke

otacke Sep 28, 2023

Contributor

My suggestion only related to code clarity. I don't know the numbers, but if resources are an issue, then the good old for-loops often are a better choice, yes.

This comment has been minimized.

Sign in to view

Copy link
@boyum

boyum Sep 28, 2023

To gain clarity and keep performance, I'd suggest helper functions:)
if (!value && value !== 0 && value !== false) {
return true; // undefined, null, NaN and empty strings.
}
else if (Array.isArray(value)) {
for (let i = 0; i < value.length; i++) {
if (!H5P.isEmpty(value[i])) {
return false; // Array contains a non-empty value
}
}
return true; // Empty array
}
else if (typeof value === 'object') {
for (let prop in value) {
if (value.hasOwnProperty(prop) && !H5P.isEmpty(value[prop])) {
return false; // Object contains a non-empty value
}
}
return true; // Empty object
}
return false;
};

/**
* Check if JavaScript path/key is loaded.
*
Expand Down

0 comments on commit 6217d57

Please sign in to comment.