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

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