Chalarangelo/30-seconds-of-interviews

the answer is wrong for the question: "How do you compare two objects in JavaScript?"

chenjigeng opened this issue · 9 comments

The code can't work by calling

isDeepEqual(1, 2)

This is my answer:

function isDeepEqual(obj1, obj2, testPrototypes = false) {
    if (obj1 === obj2) {
        return true;
    }
    if (type(obj1) === 'Function' && type(obj2) === 'Function') {
        return obj.toString() === obj2.toString();
    }
    if (type(obj1) === 'Date' && type(obj2) === 'Date') {
        return obj1.getTime() === obj2.getTime();
    }
    
    if (type(obj1) !== type(obj2) || type(obj1) !== 'object') {
        return false;
    }

    const prototypesAreEqual = testPrototypes
        ? isDeepEqual(
            Object.getPrototypeOf(obj1),
            Object.getPrototypeOf(obj2),
            true
        )
        : true;

    const obj1Props = Object.getOwnPropertyNames(obj1);
    const obj2Props = Object.getOwnPropertyNames(obj2);

    return (
        obj1Props.length === obj2Props.length &&
        prototypesAreEqual &&
        obj1Props.every(prop => isDeepEqual(obj1[prop], obj2[prop]))
    )
}

function type(obj) {
    return Object.prototype.toString.call(obj);
}
flxwu commented

Please open a PR with the proposed change in the answer's code sample. Thanks! 😊

i'm going to reopen this to allow for easier discussion from #100

Are you saying this error or that it returns true with 1 and 2 used as arguments?

yes.and it will return true with { a:1 } amd { a: 2} used as arguments.The code can't work.

This is a confirmed problem. We might need to add better handling for this.

Or we could just use JSON.stringify(a) === JSON.stringify(b) as a short and sloppy...

But like I said it's sloppy and only really applies to POJO(Plain Old JavaScript Object)

We can solve it by adding a if statement.You can see my code above.

The one you've submitted in the PR seems to be working. I want to do some more in depth tests when I have some time or if you'd like to submit some tests for it in the PR as a comment on the thread I'd be more than happy to look it over. Either way, this is a helpful find!

@chenjigeng thanks for noticing this. The PR is merged. ❤️

right never reported test results did I... yeah it's working in all cases. Never found an edge case that could be considered a problem except for a SO if I went deep with auto generating object references