Traverse and modify a deep object

Question

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] = '';
 }
 }
 }
 }
 }
 }
 }
}

• woxxom: No need for hasOwnProperty if you work with simple objects e.g. produced via JSON.parse or literals.
• ComFreek: @wOxxOm Some third-party libraries might modify Object.prototype.
• woxxom: @ComFreek the library can also modify Object.prototype.hasOwnProperty.
• ComFreek: @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.
• woxxom: 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.

Answer 1

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] = "";
 }
 });
}

• woxxom: getChildren may check if Object.values is present and use it instead.
• ComFreek: 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.

Answer 2

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] = '';
 }
 }
 }
}