Object-to-array flattening function

Question

I have written a function that is designed to convert an object into an array of key/pair values ("flattening it") like so:

Input

{
 "a": true,
 "b": {
 "c": 500
 }
}

Output

[
 ["a", true],
 ["b", [
 ["c", 500]
 ]
]

Here is the code I have written to do it:

function isCyclicObject(object, seenObjects) {
 seenObjects = (seenObjects instanceof Array) ? seenObjects : [];
 if (object instanceof Object) {
 if (seenObjects.indexOf(object) !== -1) {
 return true;
 }
 seenObjects.push(object);
 for (var key in object) {
 if (object.hasOwnProperty(key) && isCyclicObject(object[key], seenObjects)) {
 return true;
 }
 }
 return false;
 }
 return false;
}
function flattenObject(object) {
 if (!(object instanceof Object) || isCyclicObject(object)) {
 return null;
 }
 var flattened = [];
 for (var key in object) {
 var value = object[key];
 if (value instanceof Object) {
 value = flattenObject(value);
 if (value === null) {
 return null;
 }
 }
 flattened.push([key, value]);
 }
 return flattened;
}

I don't really have any major concerns over my code, however I would really appreciate having someone review it and point out any mistakes or flaws in my code that I haven't noticed. I'm also not all too familiar with how to lay out my JS code, so I believe that could possibly be improved too.

And here is the code I wrote to test its functionality:

function compareArrays(first, second) {
 if (!(first instanceof Array) || !(second instanceof Array)) {
 return false;
 }
 if (first.length != second.length) {
 return false;
 }
 for (var i = 0; i < first.length; i++) {
 var firstValue = first[i],
 secondValue = second[i];
 if (firstValue instanceof Array && secondValue instanceof Array) {
 if (!compareArrays(firstValue, secondValue)) {
 return false;
 }
 } else if (firstValue != secondValue) {
 return false;
 }
 }
 return true;
}
function testFlattenObject(object, expected) {
 var flattened = flattenObject(object);
 var objectString = JSON.stringify(object),
 expectedString = JSON.stringify(expected),
 flattenedString = JSON.stringify(flattened);
 var failed = false;
 if (object instanceof Object) {
 failed = !compareArrays(flattened, expected);
 } else {
 failed = flattened !== null;
 }
 if (failed) {
 console.error("Test failed for " + objectString + ": expected " + expectedString + " but got " + flattenedString);
 } else {
 console.log("Test passed");
 }
}
testFlattenObject({}, []);
testFlattenObject({
 a: true
}, [
 ["a", true]
]);
testFlattenObject({
 a: true,
 b: {
 c: 500
 }
}, [
 ["a", true],
 ["b", [
 ["c", 500]
 ]]
]);

• adrianton3: Your isCyclicObject function returns false positives for references to the same substructure. Take for examplevar z = {}; isCyclicObject({ a: z. b: z }). This happens because you're not popping anything off seenObjects.
• Jonah: The code is long.... Eg, you can shorten the flatten method into one line: jsfiddle.net/k4apowb7. Similar improvements can be made for the other function.

Answer 1

I guess correctness is the most important aspect - as I mentioned in a comment, isCyclicObject returns false positives for references to the same substructure.

I don't know if you intended to directly return null if a value is null (in the flattenObject function). Seems a little harsh to scrap the whole object if any one of its properties has a null value. I would have assumed the behavior should have been to skip pushing the property to the flattened array.

Performance-wise there is some room for improvement:

• the function isCyclicObject is called every time the flattenObject function is called. This is not necessary - you only need to perform the check for cycles once.

• an array is not the ideal data structure to use for seenObjects. I'd pick the native Set - don't worry, even IE (11) supports it. Read about it on MDN. Plus, to check if something is part of a set you get the neat set.has(something) method which is far better looking than array.indexOf(something) !== -1.

• for ... in is not very optimizable by JavaScript engines and what's more it will cause the whole function it's part of to not be optimized. You can read about this here. The other argument against for ... in is that it needs to be accompanied by hasOwnProperty. I see you're doing that for isCylicObject but not for flattenObject. Instead of for ... in you could use Object.keys(obj) which returns an array of the object's own properties directly.

Style-wise:

• don't expose seenObjects to the user. If the user accidentally types in a second parameter to seenObjects then strange things will happen and debugging it is going to be unpleasant. Instead you can initialise it in a closure like so:

  function isCyclicObject(object) {
   var seenObjects = []
   // let's shadow the original function
   function isCyclicObject(object) {
   // check every property or and call traverse recursively
   // your original logic, except for the `seenObjects` initialization
   }
   return isCyclicObject(object)
  }
  

This way you also eliminate that check which occurs every time, making the traversal faster.

• prefer to use const and let over var unless you're targeting old browsers

• prefer the more functional forEach/map/some in your case instead of for loops

EDIT:

Don't use ... instanceof Object to verify if something is an object. Objects with a null prototype will fail that test. Instead use `typeof ... === 'object' && ... !== null

• Downgoat: Just to note, even in the latest browsers. support for const and let is very iffy. I recommend using a transpiler when using very new features.