3
\$\begingroup\$

I wrote this piece of code yesterday at work. This function will traverse a deep nested object to find and modify a value.

I know it's bad, so I am trying to refactor it. I have not found any significant way to improve it. This is kind of dealing with legacy code. Changing the object structure is off the book, I should only modify this function, because things are working I don't dare to touch other parts.

How would you refactor this?

Object structure:

{
    panel1Name: {
        screen: {
            screen1Name: {
                variable: {
                    var1: {
                        custom: [],
                    },
                    var2: {
                        custom: [],
                    },
                    var3: {
                        custom: [],
                    },
                    // many other var
                }
            },
            screen2Name: {
                // ...
            },
            // many other screens
        }
    },
    panel2Name: {
        // ...
    },
    // many other panels
}

// custom is the value I need to check and modify

Code

function removeImageFromSettings(imageName) {
  for (let panel in $scope.data) {
    if ($scope.data.hasOwnProperty(panel)) {
      for (let screen in $scope.data[panel].screen) {
        if ($scope.data[panel].screen.hasOwnProperty(screen)) {
          for (let variable in $scope.data[panel].screen[screen].variable) {
            if ($scope.data[panel].screen[screen].variable.hasOwnProperty(variable)) {
              const data = $scope.data[panel].screen[screen].variable[variable];

              if (data.custom && data.custom[0] === imageName) {
                data.custom[0] = '';
              }
            }
          }
        }
      }
    }
  }
}
\$\endgroup\$
5
  • \$\begingroup\$ No need for hasOwnProperty if you work with simple objects e.g. produced via JSON.parse or literals. \$\endgroup\$
    – woxxom
    Commented Nov 5, 2017 at 5:35
  • \$\begingroup\$ @wOxxOm Some third-party libraries might modify Object.prototype. \$\endgroup\$
    – ComFreek
    Commented Jan 3, 2018 at 20:23
  • \$\begingroup\$ @ComFreek the library can also modify Object.prototype.hasOwnProperty. \$\endgroup\$
    – woxxom
    Commented Jan 4, 2018 at 14:11
  • \$\begingroup\$ @wOxxOm and some libraries might do while(1){} ;) Modfying the prototype is done in the wild, e.g. in Chai (source). Whether this constitutes good coding style, is another story, but one should refrain from writing code, which randomly fails upon inclusion of other libraries. \$\endgroup\$
    – ComFreek
    Commented Jan 4, 2018 at 14:51
  • \$\begingroup\$ My point is that using hasOwnProperty won't help in case you're using such bad libraries. Unless you save the initial reference to hasOwnProperty or get it via iframe hack. \$\endgroup\$
    – woxxom
    Commented Jan 4, 2018 at 15:26

2 Answers 2

1
\$\begingroup\$

You could move the logic that gets each panel / screen etc into it's own function, and remove some of the indenting, like so:

function getChildren(obj) {
    // No need to check hasOwnProperty, and keys only returns ownProperties
    return obj ? Object.keys(obj).map(function(key) {return obj[key]}) : [];
}


function removeImageFromSettings(imageName) {
    let panels = getChildren($scope.data);

    let screens = panels.reduce(function(result, panel) {
        return result.concat(getChildren(panel.screen));
    }, []);

    let variables = screens.reduce(function(result, screen) {
        return result.concat(getChildren(screen.variable));
    }, []);

    variables.forEach(function(variable) {
        if (variable.custom && variable.custom[0] === imageName) {
            variable.custom[0] = "";
        }
    });
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ getChildren may check if Object.values is present and use it instead. \$\endgroup\$
    – woxxom
    Commented Nov 5, 2017 at 5:37
  • \$\begingroup\$ I would suggest const-declaring panels, screens and variables. In addition - this might just be my personal taste - I often prefer for .. of over .forEach because the latter resembles too much a functional style, which it isn't when you pass a callback which modifies the array. \$\endgroup\$
    – ComFreek
    Commented Jan 3, 2018 at 20:35
1
\$\begingroup\$

This looks like a very good fit for JSONPath (initial specification, list of libraries).

Of course, if you only have this single code part which has to do traversing/modifying in a JSONPath-like fashion, embedding an extra library is overkill. If you use a transpiler which supports ES2017, you can make use of the latest Object.values() method:

for (const panel of Object.values(data)) {
  for (const screen of Object.values(panel.screen)) {
    for (const variable of screen.variable) {
      if (variable.custom && variable.custom[0] === imageName) {
        variable.custom[0] = '';
      }
    }
  }
}
\$\endgroup\$

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.