Search a search button on page and toggle open class and focus on the input

Question

I have a jQuery solution as below. I am confident the solution can be made better in terms of OO way using constructor pattern

 var $search = $('.search');
 if ($search.length > 0) {
 $search.removeClass('dropdown radix-dropdown-processed');
 $('.search__button').on('click', function (e) {
 e.preventDefault();
 $search.toggleClass('open');
 $('#site_search_query').focus();
 });
 }

Can someone help in making the code better as in reviewing the code so that it is better in terms of performance

• konijn: This is a snippet really. The snippet is fine and does not require OO in my opinion.

Answer 1

I am confident the solution can be made better in terms of OO way using constructor pattern

Here's the first problem: You assume writing (classical) OO makes everything better. Personally, it's more cumbersome and confusing. You worry what this is, there is state literally everywhere, methods are very side-effect-y, all that unnecessary boilerplate to make a simple operation look fancy but more complicated.

Keep it simple. I'd prefer simple functions over classical OOP.

On the other hand, you are already doing OO. It's just that it's not classical OO, but you're still operating on objects.

Here's a few corrections to the snippet

const $search = $('.search');

A $() function returns an instance of jQuery. It's similar to doing jQuery search = new jQuery('.search') if you're into classical OOP notation.

$search.removeClass('dropdown radix-dropdown-processed'); 

jQuery objects are array-like objects, containing the matches of your selector. Unless explicitly stated, most methods loop through that array and apply to each item. No items, no loop, therefore method won't run. This makes the length check redundant.

$('.search').on('click', '.search__button', function (e) {
 e.preventDefault();
 e.delegateTarget.toggleClass('open');
});

Now assuming .search__button resides inside .search, you can use delegation. This is where jQuery attaches the handler on an ancestor element to listen for descendant events. The advantage is that it does not rely on the descendant to be there on bind. Also good if you have a lot of descendants to listen. You attach the handler once on the ancestor and not per descendant.

Additionally, you don't need to keep reference of the ancestor you attached the handler to. The event object has a delegateTarget which references the ancestor you attached the event to, in this case .search.

$('#site_search_query').focus();

IDs are brittle. The can only appear once in the page but nothing stops you from putting more than one. The problem appears when you try to grab their reference. document.getElementById as well as jQuery only return the very first one it finds. If your element's id collides with another's and that other comes first, you get the idea.