3
\$\begingroup\$

I'm willing to check an an object's values in typescript, based on another object's keys. The mental model is "the key exists, it's not an object, is the value in obj set, otherwise, it's an object, iterate again".

What would be your recommandations about this simple snippet :

/**
 * Returns an array. If the array is empty, the checked obj is fine.
 * Otherwise it's not.
 * 
 * @remarks
 * areValuesSet(obj, rules).length === 0 is the test to make.
 * 
 * @param obj {object} - object to verify
 * @param rules {object} - object containing the keys to check against
 * @returns {string[]} - array of incorrect keys
 */
const areValuesSet = (
  obj: { [propKey: string]: any },
  rules: { [propKey: string]: any }
): string[] =>
  Object.keys(rules).reduce((prev, key) => {
    if (typeof rules[key] === "object" && typeof obj[key] === "object") {
      return areValuesSet(obj[key], rules[key]);
    }

    return !obj ||
      !obj[key] ||
      (isEmpty(obj[key]) && typeof obj[key] !== 'boolean')
      ? [...prev, key]
      : [...prev];
  }, []);

I would enhance this with a "validator" function which would call areValuesSet like so :

/**
 * Call `rulesParser` and checks the returned value length equals 0
 *
 * @param obj {object} - object to verify
 * @param rulesObj {object} - object containing the keys to check against
 * @returns {boolean}
 */
export const validateRules = (
  obj: { [propKey: string]: any },
  rulesObj: { [propKey: string]: any }
): boolean => rulesParser(obj, rulesObj).length === 0;

Thank you

\$\endgroup\$
3
  • \$\begingroup\$ To validate, are you hoping that all primitive values in the input (possibly nested inside objects) are numbers? Is that the logic you wish to perform? \$\endgroup\$ Commented Oct 28, 2020 at 18:24
  • \$\begingroup\$ No I want to have primitives with no empty value. \$\endgroup\$
    – Neovea
    Commented Oct 29, 2020 at 8:33
  • \$\begingroup\$ I updated with lodash.isempty \$\endgroup\$
    – Neovea
    Commented Oct 29, 2020 at 10:49

1 Answer 1

3
\$\begingroup\$

Function and parameter names The function name areValuesSet could be more precise. It returns an array of incorrect property names, not a boolean of if values are set. Maybe call it getIncorrectProperties? Also, the argument named obj is uninformative. You want to indicate that it's the target value being checked. Maybe call it validateTarget or something similar.

Avoid any any is type-unsafe. Unless you have a really good reason for it, better to type something you don't know about as unknown instead. unknown is similar to any, but requires you to do type-narrowing first. In this case, using unknown will point out a bug in your current logic:

typeof null gives object You have:

if (typeof rules[key] === "object" && typeof obj[key] === "object") {
  return areValuesSet(obj[key], rules[key]);
}

This will result in the recursive call throwing if one of the values happens to be null, since null is an object. Add in and call:

const isObject = (arg: unknown) => typeof arg === 'object' && arg !== null;

!obj[key] excludes false Your current logic permits values of true but forbids values of false due to this check:

!obj[key] || (isEmpty(obj[key]) && typeof obj[key] !== 'boolean')

Is that deliberate? It's OK if it is, but it may well be a logic bug. If false should indeed be excluded, then typeof obj[key] !== 'boolean' simplifies to obj[key] !== true.

Nested property bugs

(1) The recursive call is only entered if both the rules and target are objects. If the rule is an object, and the target is a non-empty non-object, the target will pass, when it probably shouldn't. Eg:

// rules:
{
  prop: { val: 'val' }
}

// target:
{
  prop: 'foo'
}

should probably fail, but won't. If the rule is an object and the target value is not an object, you probably want to push the property to the array of errors.

(2) When a sub-object is found as a value, your reduce callback does:

return areValuesSet(obj[key], rules[key]);

Whatever may have been put into the accumulator array on prior iterations will be lost, since it's not being factored into the return value. For example, if you had a data structure for which the first 5 properties were wrong, but the 6th property is nested and correct, the function would return an empty array.

But reduce arguably isn't very appropriate in these sorts of situations anyway - see that link for a video by Chrome developers on the subject. It introduces an annoying amount of boilerplate code, especially in TS, and even well-formulated reduces like these often aren't entirely trivial to understand at a glance.

Consider creating an array outside the function and pushing to it instead - or use a recursive function which creates the array to return as a default argument when not passed. Including all the other suggestions as well:

type GenericObj = { [propKey: string]: unknown };
const isObject = (arg: unknown): arg is GenericObj => typeof arg === 'object' && arg !== null;

const getIncorrectProperties = (
  validateTarget: GenericObj,
  rules: GenericObj,
  incorrectProperties: string[] = [],
) => {
  for (const [key, ruleValue] of Object.entries(rules)) {
    const targetValue = validateTarget[key];
    if (isObject(ruleValue)) {
      if (!isObject(targetValue)) {
        incorrectProperties.push(key);
      } else {
        getIncorrectProperties(targetValue, ruleValue, incorrectProperties);
      }
    } else if (!targetValue || (isEmpty(targetValue) && targetValue !== true)) {
      incorrectProperties.push(key);
    }
  }
  return incorrectProperties;
};

Live snippet of compiled code including example validation, to show that it's working:

"use strict";
const isObject = (arg) => typeof arg === 'object' && arg !== null;
const getIncorrectProperties = (validateTarget, rules, incorrectProperties = []) => {
    for (const [key, ruleValue] of Object.entries(rules)) {
        const targetValue = validateTarget[key];
        if (isObject(ruleValue)) {
            if (!isObject(targetValue)) {
                incorrectProperties.push(key);
            }
            else {
                getIncorrectProperties(targetValue, ruleValue, incorrectProperties);
            }
        }
        else if (!targetValue || (_.isEmpty(targetValue) && targetValue !== true)) {
            incorrectProperties.push(key);
        }
    }
    return incorrectProperties;
};
console.log(getIncorrectProperties({
    prop2: {
        nested: 'val',
    }
}, {
    prop1: true,
    prop2: {
        nested: true,
        nested2: {
            deeplyNested: true
        }
    },
    prop3: true
}));
<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js"></script>

The only issue I have with the above code is the incorrectProperties argument. Although the approach works to create an array on initial call and return it at the end, having it as a parameter might be confusing, since the initial call should not take 3 arguments, only 2. If you think that's not acceptable, you could create an array in the function body on every call and return it at the end, and spread the result of the recursive call into that array.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, clear solution. I'm being more aware that I'm over using reduce method. Btw, I opt for a function scoped declaration of incorrectProperties . Thank you again \$\endgroup\$
    – Neovea
    Commented Oct 30, 2020 at 5:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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