Problem
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
Solution
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
intempDocument
is not necessary. Therefore the variable name should better bedocument
, 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 calledparsererror
. 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.