unclechu / node-deep-extend

Recursive extend module
MIT License
202 stars 53 forks source link

Does the initial check in the `key in obj` loop actual do anything? #12

Closed watson closed 9 years ago

watson commented 9 years ago

Inside the main loop there is a simple if statement:

for (key in obj) {
  if ( ! (key in obj)) continue;
  ...
}

This if statement was introduced as a response to issue #5: https://github.com/unclechu/node-deep-extend/commit/3f794748f95f86928dfb7bba40beee8a9ec1349f#diff-168726dbe96b3ce427e7fedce31bb0bcR53

Previously the code looked like this:

for (key in obj) {
  if (obj[key] !== void 0) {
    ...
  }
}

Ok, so previously undefined (here written as void 0) was ignored, where as the new code treats it just as any other value. But I cannot understand what the new if-statement actually does? What edge case will trigger it?

moll commented 9 years ago

That if check makes no sense. If something is enumerable, there's no way it isn't there.

unclechu commented 9 years ago

@watson @moll This checks that key exists in obj.

deepExtend({ a: 1, g: 123 }, { g:  (void 0) }); // returns { a: 1, g: undefined }

But with !== void 0 condition:

deepExtend({ a: 1, g: 123 }, { g:  (void 0) }); // returns { a: 1 }
moll commented 9 years ago

Unless I'm reading the code wrong, the obj whose key is checked is the same one iterated over one line above. There it makes no sense. I could understand if it were some other object.

watson commented 9 years ago

@unclechu I get why you wanted to get rid of the obj[key] !== void 0 guard as that would leave out undefined values. But the thing I don't understand is the new !(key in obj) statement. Is there ever a situation where that would evaluate to true?

unclechu commented 9 years ago

@watson @moll (key in obj) check that obj has key even if obj[key] is undefined, but exists it returns true. obj[key] can be undefined and exists in the same time. See this issue: #5, this solve it.

moll commented 9 years ago

No, that test is useless in the code @watson quoted. There is no way it can ever be false. Just delete it and convince yourself by seeing all tests still pass.

watson commented 9 years ago

@unclechu Doesn't the outer for (key in obj) loop guarantee that the key always exist in the obj??

watson commented 9 years ago

@unclechu what you are writing is not wrong and I totally agree that obj[key] will evaluate falsy which is why I agree that it should be removed as you correctly did - but at the same time you added the if ( ! (key in obj)) continue; part - it's that part that I think should be removed - that doesn't have anything to do with checking if it's undefined or not.

unclechu commented 9 years ago

@watson Sorry, u was right, it's just useless condition. Removed.

unclechu commented 9 years ago

Removed in v0.3.3.