add if (!value.hasOwnProperty(vKey)) continue; that occurred bugs!

[XadillaX]

Before patching this patch, some functions attached to the object will be parsed and thrown errors!

For an example:

Array.prototype.foo = function() {};
var a = [];
for(var key in a) {
    console.log(key);
}

The result will be abc - this is not my expected result!

In your for ... in of util.js:

case Array:
  if (toString.call(value) !== '[object Array]') {
    err = 'Argument "' + key + '" is not a valid Array.';
  }
  if (!err && key === 'key') {
    for (var vKey in value) {
      ...

We should ignore abc this extended function.

[3rd-Eden]

Why remove this?

[XadillaX]

Sry, this is because of my JSHint.

#the never used variables#

[3rd-Eden]

sure, but re-defining a scoped undefined variable actually makes checking against undefined safe as you can override the global undefined variable with things.

[hybrist]

I think the redefining undefined part is obsolete in node and other modern runtimes:

> undefined = 'foo'
'foo'
> undefined
undefined

[3rd-Eden]

Some minor comments but LGTM.

[XadillaX]

@ronkorving @3rd-Eden So you both prefer to use Object.keys instead of for ... in?

[XadillaX]

I found another problem when I use Object.keys():

If the object is undefined, for(var i in undefined) will be ran as an empty object bug Object.keys(undefined) will throw an error!

This error makes mocha failed:

     TypeError: Cannot convert undefined or null to object
      at Function.keys (native)
      at Object.merge (/Users/XadillaX/code/node/node-memcached/lib/utils.js:113:10)
      at new Client (/Users/XadillaX/code/node/node-memcached/lib/memcached.js:58:9)
      at Context.<anonymous> (/Users/XadillaX/code/node/node-memcached/test/memcached-touch.test.js:21:21)
      at Test.Runnable.run (/usr/local/lib/node_modules/mocha/lib/runnable.js:217:15)
      at Runner.runTest (/usr/local/lib/node_modules/mocha/lib/runner.js:373:10)
      at /usr/local/lib/node_modules/mocha/lib/runner.js:451:12
      at next (/usr/local/lib/node_modules/mocha/lib/runner.js:298:14)
      at /usr/local/lib/node_modules/mocha/lib/runner.js:308:7
      at next (/usr/local/lib/node_modules/mocha/lib/runner.js:246:23)
      at Immediate._onImmediate (/usr/local/lib/node_modules/mocha/lib/runner.js:275:5)
      at processImmediate [as _immediateCallback] (timers.js:321:17)

So I think we still should use for ... in, or we should make sure object is not null at first: if(object !== undefined && object !== null).

@3rd-Eden @ronkorving

[ronkorving]

Correct, Object.keys breaks on non-objects. A simple truthy check would address that, right? That's my preferred style. But I suggest you go with whatever style @3rd-Eden prefers.

if (obj) {
  var keys = Object.keys(obj);
}

[3rd-Eden]

But doing a for in on an undefined is just as bad as doing an Object.keys on undefined. I would argue that a thrown error would be better as you're simply not receiving what you're expecting as the only proper use-case for a for in loop would be to iterate over an object.

If the code fails with Object.key there is a bigger problem with the code somewhere as it shouldn't even iterate on it.