JavaScript reducer that updates a deep value in a hash

Question

I've been roughing out a proof of concept enabling admin staff to flag submitted applications with issues as needed. We have many forms some of which are nested. So we end up with these big hashes to validate. Primarily we have issues with uploaded images (too blurry, too dark etc) but it could be any field.

So admin may see a field verified_documents -> drivers_licence -> front_image. If an issue is raised we end up with a hash to store.

{
    verified_documents: {
        drivers_licence: {
            front_image: ['Image is too blurry', 'Image is too dark']
        }
    }
}

The code works but is very smelly. The function in question is updateIssue. Which accepts a string representation of the path "verified_documents.drivers_licence.front_image" and an array of selected options from a dropdown.

The function's intentions:

1. Take a copy of current issues, an attempt to be pure and return a new object.
2. Split the string into an array.
3. Iterate over each value in the path array.
4. If path length is the last set its value to the array of issues.
5. If the value is an empty array set the value to undefined (only happens if an issue is removed).
6. If the key can't be found, create the next level as a child hash.
7. Returning the next level of nested object if no other condition is met.

I'm happy I was able to get it all working, but now it's not readable. Readability over performance is my objective, as this code is likely to change.

import { merge, isEmpty } from "lodash";

let Issues = undefined;

const getNestedValue = (nestedObj, pathArr) => {
  return pathArr.reduce(
    (obj, key) => (obj && obj[key] !== "undefined" ? obj[key] : undefined),
    nestedObj
  );
};

export class IssuesData {
  static updateIssue = (path, value) => {
    const valueObj = merge({}, Issues);
    const pathArr = path.split(".");
    pathArr.reduce((obj, key, i) => {
      if (pathArr.length - 1 === i) {
        if (isEmpty(value)) {
          return (obj[key] = undefined);
        }
        return (obj[key] = value);
      }

      if (obj[key] === undefined) {
        return (obj[key] = {});
      }
      return obj[key];
    }, valueObj);
    Issues = valueObj;
  };

  static getIssueByKey = path => {
    return getNestedValue(Issues, path.split("."));
  };

  static getIssues() {
    return Issues;
  }

  static setIssues(issues) {
    if (!Issues) {
      Issues = issues;
    }
  }
}

---

UPDATE:

After sleeping on it I managed to clean it up a little. This is more readable but there are still multiple return. If anyone has advice on cleaning it up further it'll be welcomed. I think I'll turn my attention to converting the static methods to instance methods now.

  static updateIssue = (path, value) => {
    const valueObj = merge({}, Issues);
    const pathArray = path.split(".");

    pathArray.reduce((obj, key, i) => {
      if (isLast(pathArray, i)) {
        if (isEmpty(value)) {
          return delete obj[key];
        }
        return (obj[key] = value);
      }
      return obj[key];
    }, valueObj);
    Issues = valueObj;
  };

Answer 1

Since you are already using lodash, don't reinvent the wheel! Nearly all of your code can be replaced by a single _.set call.

const Issues = {
  verified_documents: {
    drivers_licence: {
      front_image: ['Image is too blurry', 'Image is too dark']
    }
  }
}

console.log(Issues)
_.set(Issues, 'verified_documents.drivers_licence.front_image', [])
console.log(Issues)

.as-console-wrapper { max-height: 100% !important; }

<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.10/lodash.min.js"></script>

With that said, I believe there is merit to reviewing your approach.

1. Not mutating an object doesn't mean a function is pure. Sure, updateIssue doesn't mutate the Issues object, but it does overwrite it, making this function inherently impure.

2. Don't use merge to clone an object. Use clone or cloneDeep.

3. A class with nothing but static functions shouldn't be a class. Just use a plain object, or if you are using a proper module system, don't even bother wrapping functions in an object!

4. setIssues worries me. When I call a method called setSomething, I expect it to either set, or throw an error. Silently ignoring the call if Issues is already instantiated will probably lead to painful debugging later.

5. getNestedValue can be replaced with at.

6. Multiple returns is not necessarily a bad thing. The "single exit" rule is far more important when writing assembly than JavaScript. There's plenty of discussion on this. See this blog, another post, this software engineering question... plenty more to be found with a simple search.

7. getNestedValue looks like it has a bug, obj[key] !== "undefined" should probably have a typeof in front of it.

8. Your rewritten function, while cleaner, does not exactly replicate the behavior of your original implementation. If a key that does not exist in the object is specified, the original implementation will set it. The refactored function will throw.