2
\$\begingroup\$

I need to compare 2 objects to determine if they have strictly the same properties.

This is what I came up with so far:

import isEmpty from 'lodash/isEmpty';

const shallowPropertiesMatch = (firstObject, secondObject) => {
  if (isEmpty(firstObject) || isEmpty(secondObject)) return false;

  const firstObjectKeys = Object.keys(firstObject);
  const secondObjectKeys = Object.keys(secondObject);

  if (firstObjectKeys.length !== secondObjectKeys.length) return false;
  if (!firstObjectKeys.every(value => secondObjectKeys.includes(value))) return false;

  return true;
};

I was wondering if there is a more efficient, elegant or simpler way of doing this?

\$\endgroup\$
1
\$\begingroup\$

How can you know if a function is efficient if you use 3rd party code. Even if you check the source, it is subject to change without notice so you can never know if your code is running the best it can. That is the price you pay for using 3rd party code.

However I don't see the need to use lodash/isEmpty as you determine that when you get the object keys. If there are no keys the object is empty.

Not delimiting a statement block eg if (isEmpty(firstObject) || isEmpty(secondObject)) return false; is a bad habit. Always delimit all blocks with {}.

Your naming is way too verbose. Use the functions context to imply meaning. The function name implies (well sort of) you are handling objects.

One solutions is as follows.

function compareObjKeys(A, B) {
    const keys = Object.keys(A);
    if (keys.length > 0) {
        const keysB = Object.keys(B);
        if (keysB.length === keys.length) {
            const keysA = new Set(keys);
            return keysB.every(k => keysA.has(k));
        }
    }
    return false;
}

But I would not go as far and favor a smaller source size. The performance savings of the above are minor and only when one of the objects is empty which I would imagine would be rare

function compareObjKeys(A, B) {
    const kA = new Set(Object.keys(A)), kB = Object.keys(B);
    return kB.length > 0 && kA.size === kB.length && kB.every(k => kA.has(k));
}
\$\endgroup\$
  • \$\begingroup\$ Please explain why being too verbose is a bad thing. shallowPropertiesMatch indicates the function returns a boolean, and ignores nested properties (it's located in /utils/object.js). With compareObjKeys I might want to double check what it does exactly before using it. \$\endgroup\$ – ThunderDev Mar 14 at 9:22
  • \$\begingroup\$ @ThunderDev Humans recognize words by shape first, sciencealert.com/… Camel case long words force extra cognitive effort when scanning code. This make it less readable and harder to spot potential bugs. \$\endgroup\$ – Blindman67 Mar 14 at 10:49
  • \$\begingroup\$ The article states there is evidence only to suggest we recognize words by shape. Furthermore, it's camel casing in general that hinders shape recognition, not particularly camel casing with long words. Also, I would have thought it required an extra cognitive effort for our brain to translate abbreviations. \$\endgroup\$ – ThunderDev Mar 14 at 13:57
  • \$\begingroup\$ @ThunderDev I assume you were able to read the linked image. If you find (!fristObjectKeys.every(value => secondObjectKeys.includes(value))) easier to read than (in context) kA.every(k => kB.includes(k)) you should keep with the style that best suits you. If you got to here without spotting the typo, maybe you should consider a shorter style? \$\endgroup\$ – Blindman67 Mar 14 at 14:33
  • \$\begingroup\$ Haha, clever, did not spot the typo ^^ to me abbreviations are still harder to interpret though. \$\endgroup\$ – ThunderDev Mar 14 at 14:54
1
\$\begingroup\$

By this condition, the function returns false when both objects are empty:

if (isEmpty(firstObject) || isEmpty(secondObject)) return false;

I would expect true in this case, and rewrite the condition as:

if (isEmpty(firstObject) != isEmpty(secondObject)) return false;

That is, return false if one of them is empty while the other is not.

In fact, this special treatment is not even necessary, because the rest of the function naturally handles the case of empty objects.


I was wondering if there is a more efficient, elegant or simpler way of doing this?

The current implementation is not efficient, because of this step:

if (!firstObjectKeys.every(value => secondObjectKeys.includes(value))) return false;

The problem is that secondObjectKeys is an array, and therefore .includes does a linear lookup. You can improve the linear-time lookup to constant-time lookup by converting secondObjectKeys to a set.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.