Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Useless statement #116

Open
c-smile opened this issue Jun 29, 2019 · 8 comments
Open

Useless statement #116

c-smile opened this issue Jun 29, 2019 · 8 comments

Comments

@c-smile
Copy link

c-smile commented Jun 29, 2019

This statment:

nanomorph/index.js

Line 160 in 43d39d0

if (a.tagName !== b.tagName) return false

is useless as the function returns false by default.
@goto-bus-stop
Copy link
Member

I think if the condition on the next line is true, the function can still return true.

@tunnckoCore
Copy link

Ordering of the ifs are pretty important. That's why. So, no, it's not useless.

@c-smile
Copy link
Author

c-smile commented Jun 30, 2019 •
edited

Consider this:

function same(a,b) is called for a and b of Element type so effectively you get this:

function same (a, b) {
  ...
  if (a.tagName !== b.tagName) return false; // either false
  // if (a.type === TEXT_NODE) return a.nodeValue === b.nodeValue - dead statement if Elements
  return false; // or false again
}

If a and b are Node's then a.tagName is always undefined thus that part looks as this

function same (a, b) {
  ...
  // if (a.tagName !== b.tagName) return false; - dead statment if Nodes 
  if (a.type === TEXT_NODE) return a.nodeValue === b.nodeValue;
  return false;
}

@tunnckoCore
Copy link

Even if this makes sense, still. The thing is that we don't know what a and b are at the point where we call the same(). In one case it can be Element, in another, it can be Node. It's not completely useless or just sitting there, never used. I'm pretty sure it is covered when you run the coverage.

There are tons of tests. I also had reimplemented it (in demo-minmorph) a dozen of times just out of curiosity.

@c-smile
Copy link
Author

c-smile commented Jun 30, 2019

Try this:

<html>
    <head>
        <title>Test</title>
    </head>
    <body>
    <div>1</div>
    <div>2</div>
    </body>
<script type="text/javascript">

function same (a, b) {
  if (a.id) return a.id === b.id;
  if (a.isSameNode) return a.isSameNode(b);
  if (a.tagName !== b.tagName) return false;
  if (a.type === TEXT_NODE) return a.nodeValue === b.nodeValue;
  return false;
}

var divs = document.body.getElementsByTagName("div");
console.log( same(divs[0],divs[1]) );
</script>

</html>

You will see false in console. But I think true is expected there, right?

@tunnckoCore
Copy link

tunnckoCore commented Jun 30, 2019 •
edited

But I think true is expected there, right?

Mmm, nope. Why it would be? It goes to the a.nodeValue === b.nodeValue which should return false, because it's 1 === 2.

I'll say that again, same is called recursively anyway, so 1) we don't know what's what in a given point in time and 2) it makes sense to have exactly these checks.

@c-smile
Copy link
Author

c-smile commented Jun 30, 2019

Mmm, nope. Why it would be?

Then if (a.tagName !== b.tagName) return false; is useless.

QED.

@tunnckoCore
Copy link

Omg, in this exact case, yes of course. Remove it and run the tests, and you will see that we need it.

Even if we remove it or put it in separate function... We still need to have this check anyway. That's why it is there in the same.

I'm done here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@goto-bus-stop @tunnckoCore @c-smile