1
\$\begingroup\$

I've decided to reproduce the JQuery selector - specifically for finding elements by class name, ID, and tag

So for example the tests could be written like

$('.box') === domSelector('.box') // Class
$('#box') === domSelector('#box') // ID
$('input') === domSelector('input') // Tag

Here is my approach to the problem. It works good, and I'd like to hear the ideas to simplify it or improve it in any way.

var domSelector = function(selectors) {    
  if (selectors.startsWith('.')) {
    return Array.from(document.getElementsByClassName(selectors.substring(1)));
  }
  else if (selectors.startsWith('#')) {
    return [document.getElementById(selectors.substring(1))];
  }
  else if (!/^[a-zA-Z0-9!@#\$%\^\&*\)\(+=._-]+$/g.test(selectors)) {
    throw Error('Invalid Selector');
  }
  else {
    return Array.from(document.getElementsByTagName(selectors));
  }
};
  1. 1st If - Check if passed value is class. (if it starts with a dot, it is a class)
  2. 2nd If - Check if a passed value is an ID (if it starts with #, it is an ID)
  3. 3rd If - Now since I already checked for both characters # and dot , I check if passed value starts with special character (Remember that it cannot start with dot or # as I already did that check). If it starts with a special character - I throw an exception that identifier is not in a valid format
  4. 4th If - The only left option is that element is tag. No further check needs to be done as if selector doesn't match any tags it will return an empty array.
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I added reinventing-the-wheel since you specifically mention JQuery's implementation of this. Feel free to remove it again if you disagree. \$\endgroup\$ – AlexV Jul 3 '19 at 13:37
  • \$\begingroup\$ Are you aware of querySelectorAll? \$\endgroup\$ – RoToRa Jul 3 '19 at 15:16
  • \$\begingroup\$ @RoToRa Yes I am, how ever it doesn't return what I need. It would just make me write more code. The best example is when using the querySelectorAll to find elements with a matching id. If there are two elements with the same id (which shouldn't happen, but can) - querySelectorAll will return both of them while jQuerys $('#byId') returns only one just like getElementById. \$\endgroup\$ – Dino Jul 4 '19 at 10:12
  • \$\begingroup\$ @Dino How is getting only one of two elements with the same ID a good thing? How do you know that you are getting the "right one"? \$\endgroup\$ – RoToRa Jul 4 '19 at 11:07
  • \$\begingroup\$ @RoToRa Element IDs should be unique within the entire document, it's not a valid HTML if you have same ID in multiple elements. That's why you expect to get only one element when getting it by ID. And if you use querySelectorAll you have to add an extra step to return only one element - because of course elements with the same ID's might occur by fault \$\endgroup\$ – Dino Jul 4 '19 at 13:31
1
\$\begingroup\$

Duplicated Ids!!! 😲

I notice in the comments there is mention of more than 1 element with the same Id and that getElementById will only return one element, which is correct for the majority of browsers depending on the version.

Even if getElementById acted the same across browsers As a developer intentionally using duplicated Id is VERY BAD! as it will force the browser into Quirks Mode effecting performance, layout and can effect how search engine bots crawl your site.

Though Google does say...

“Although we do recommend using valid HTML, it’s not likely to be a factor in how Google crawls and indexes your site.”

... from SEJ 6 Reasons Why Google Says Valid HTML Matters that links to Google Browser Compatibility support page

This is not a guarantee and not something you should trust your sites SEO on.

The simple and safe rule is. NEVER duplicate Id's!!!

Your code

Ignoring the above and looking at your code without regard to the DOM and that getElementById can return an array you can improve the code (readability and maintainability [1.]) as follows

function queryDOM(selector) {
    const VALIDATOR = /[a-z0-9!@#$%^&*)(+=._-]/i, NS = "http://www.w3.org/1999/xhtml";
    const type = selector[0], query = selector.slice(1);
    if (type === ".") { return [...document.getElementsByClassName(query)] }
    if (type === "#") { return [document.getElementById(query)] }
    if (! VALIDATOR.test(selector)) { throw new RangeError('Invalid Selector') }
    return [...document.getElementsByTagNameNS(NS, selector)];
}

or for the code newbies that need the spacing

function queryDOM(selector) {
    const VALIDATOR = /[a-z0-9!@#$%^&*)(+=._-]/i;
    const NS = "http://www.w3.org/1999/xhtml";
    const type = selector[0];
    const query = selector.slice(1);

    if (type === ".") { 
        return [...document.getElementsByClassName(query)];
    }

    if (type === "#") { 
        return [document.getElementById(query)]; 
    }

    if (! VALIDATOR.test(selector)) { 
        throw new RangeError("Invalid Selector"); 
    }

    return [...document.getElementsByTagNameNS(NS, selector)];
}

Reasoning

  • As a function declaration rather than a function expression so that the function is accessible at any time after parsing.
  • Extract the first character and remaining query in one spot to avoid logic/code duplication.
  • Remove the redundant else and supporting code to reduce noise.
  • Use ES6+ spread operator ... (AKA spread syntax) to create arrays from iteratable objects.
  • Use the simpler bracket access for single character in string rather than the more verbose String.startsWith which is meant for sub strings longer than one character.
  • Use String.slice rather than String.substring
  • Renamed function to hold to camel-case convention of upperCase acronyms and rearranging to stop clash with JS convention of PascalCase only functions used with the new token (Note your function will work as new DOMSelector())
  • Renamed argument to not imply its an array (plurals only for arrays or array like objects)
  • Throw a range error more suited to the error type (Really should not be throwing for this type of issue)
  • Remove the global flag from the RegExp
  • Removed redundant ^, $, + and back slashes inside the [] from RegExp
  • Added case insensitive flag to RegExp
  • Declared RegExp as a constant to take out of the statement to make it more readable.

  • As you include capitals in the validation of the selector I assume you are unaware that getElementsByTagName first converts the query string to lowercase. Thus I changed the function to use getElementsByTagNameNS

    The getElementsByTagNameNS which namespace (NS) to use is not known from your code and thus the example assumes "http://www.w3.org/1999/xhtml". Warning quirks mode will effect the namespace.

Additionally

The function has a failure if the selector argument is not a string. As the function does throw I assume you only use it inside a try catch block and you handle the error when it occurs.

Why the complications!

As your code relies on some assumptions that are flawed and do not compile with valid HTML I can not approve the above as worthy of anything but as an example of what not to do.

The code should use querySelectorAll and covert its iteratable return to an array. I would also drop the "DOM" in the name (what else could you be querying?) and include a node to focus the query.

Also removing the validation as query string can be used to search attributes that may conflict with the validation.

function query(str, node = document) { return [...node.querySelectorAll(str)] }

Or if you control code placement and use the "use strict" directive you can use an arrow function expression to assign a named constant to prevent accidental overwriting of the function.

"use strict"; // << Must be at top of code.
const query = (str, node = document) => [...node.querySelectorAll(str)];

This is not at all replacement of jQuery

BTW

A reminder. NEVER duplicate Id's!!!

See I added 3 exclamation points and made it bold to indicate how important this warning is.


Notes

[1.] Readability and maintainability are highly subjective quantities and as such reflex my view only. I have 40Years of experience (35 as a professional) if that counts for anything.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your descriptive post! I agree with some of the things, how ever with some I do not. If you take a look at the JQuery's #ID Selector API you will see it's using the getElementById, and states that it will return zero or one DOM element. And as far as I know querySelectorAll is static, which means if I dynamically add some elements after the page loads - it wouldn't return those newly added elements while on the other hand getElementsByTagName would. \$\endgroup\$ – Dino Jul 4 '19 at 17:20
  • \$\begingroup\$ @Dino By dynamically i presume you mean add HTML (markup) to the page (also very bad). If you add content via the DOM interface querySelectorAll will return a list of all matching elements including new one. Yes my bad I was thinking of direct referencing rather than getElementById (if referencing by id in JS browsers return an array in quirks mode) \$\endgroup\$ – Blindman67 Jul 4 '19 at 18:42

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.