3
\$\begingroup\$

I have this code that I've made to parse a string of HTML and tell me if it is all enclosed within a specified tag:

const textIsWrappedWithTag = (text, tag) => {
  const tempDocument = new DOMParser().parseFromString(text, "text/xml");
  if (tempDocument.getElementsByTagName("parsererror").length) {
    return false;
  }
  if (tempDocument.children.length === 1 && tempDocument.children[0].tagName.toLowerCase() === tag.toLowerCase()) {
    return true;
  }
  return false;
};

My test cases do what I want them to do:

const yes = '<p><div><span>one</span></div><div><p>two</p></div></p>';
const no = '<p>one</p><p>two</p><p>three</p>';
const no2 = '<div><p>one</p><p>two</p><p>three</p></div>';


textIsWrappedWithTag(yes, 'p');
> true
textIsWrappedWithTag(no, 'p');
> false
textIsWrappedWithTag(no2, 'p');
> false

I played around with this in the console to develop it, but I'm not super famliar with DOMParser and parsererror, so I want to be sure that I'm not overlooking something

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. textIsWrappedWithTag(yes, 'p'); actually returns false instead of true... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 11 at 22:35
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ ah sorry, I left out a .length check. Fixed that and now it returns true. \$\endgroup\$ – JacobIRR Jun 11 at 22:39
5
\$\begingroup\$

Your code looks good and concise.

Your test cases cover almost every branch of the code, which is also good.

You can improve the variable names:

  • The temp in tempDocument is not necessary. Therefore the variable name should better be document, or if that is too long, doc. Be careful though when abbreviating variable names, and don't make them ambiguous.

  • The no test case should rather be called parsererror. I had to run the code to see that it really tests this branch of the code. By improving the name of the variable, you make this more obvious.

There is a test case missing for a tag name other than p.

if (doc.children.length === 1 && doc.children[0].tagName.toLowerCase() === tag.toLowerCase()) {
  return true;
}
return false;

If you want, you can write this code a bit shorter:

return doc.children.length === 1
    && doc.children[0].tagName.toLowerCase() === tag.toLowerCase();

Your current code is a bit longer, but it allows you to set a breakpoint on the return true statement, which may be useful when debugging the code. But since you already have automated tests running, you will probably not need to set a breakpoint there. Your choice. Many programmers prefer the shorter form, but the longer form is not bad per se.

The expression doc.children.length === 1 is always true. This is because you are parsing the text using the text/xml type, and the XML standard requires exactly 1 root element. Everything else will end up in the parsererror branch. To verify this, you should add a test case for an empty string. And a test case for an XML comment, such as <!-- comment -->.

When I first read the function name textIsWrappedWithTag, I didn't know exactly what to expect from that function. My first thought was that textIsWrappedWithTag('<div><p>text</p></div>', 'p') would return true, since there is some text that is enclosed in a p tag. You could try to find a better name for that function, maybe topLevelElementIs.

By the way, the DOM API talks about elements, while the HTML standard calls them tags. If you want your function to feel like it belongs to the DOM API, you should call it textIsWrappedWithElement instead.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.