Add/remove class event handler

Question

I wrote this if statement event listener to add and remove a class to a couple links based on changed.owl.carousel. However, I know this is not the most DRY method in writing the code and often find myself writing more than is needed. Is there a cleaner way to write this code?

owl.on('changed.owl.carousel', function(event) {
  if ($('.owl-item.active>.item.slide3').length>0) {
    $('#link3').addClass('hover');
  } else if (jQuery('.owl-item.active>.item.slide4').length>0) {
    $('.link').removeClass('hover');
    $('#link4').addClass('hover');
  } else {
    $('.link').removeClass('hover');
  }
})

Welcome to Code Review, your first post looks good, except that the title should be descriptive of what the code does, not what you want to do be done to it in a review. Hope you get some good answers! — ferada
– ferada, Commented Nov 16, 2016 at 18:24
Check this code. Edit the question to include corresponding HTML, it may help to make the code better. — Tushar
– Tushar, Commented Nov 17, 2016 at 3:06
You really should add more context here, both javascript and HTML. I see red flags any time I see somename* (where * is a number) naming patterns show up in code. This code smell oftentimes indicates that you have places where you can improve your approach. It is hard to say here though without context. — Mike Brant
– Mike Brant, Commented Nov 18, 2016 at 20:25
You use link as both a class name and link# as an ID. IMO that makes it harder to read, and easier to make a mistake using one when you need the other. — Garry
– Garry, Commented Nov 21, 2016 at 22:09

Joseph · Accepted Answer · 2016-11-21 21:05:58Z

if ($('.owl-item.active>.item.slide3').length>0) {
  $('#link3').addClass('hover');
} else if (jQuery('.owl-item.active>.item.slide4').length>0) {
  $('.link').removeClass('hover');
  $('#link4').addClass('hover');
} else {
  $('.link').removeClass('hover');
}

I notice the use of both $ and jQuery. Suggesting you use one or the other, but not both. While both are perfectly valid, it's best to keep it consistent.

Also, put a space on both sides of the > for readability. It's not immediately clear that there's actually 2 classes in the selector.

Comparing against 0 is redundant as the number can already double as a boolean. length is a number that's either 0 or a positive integer. Any non-zero number is truthy while 0 is falsy.

I would suggest you avoid IDs. IDs should be unique but nothing prevents you or other code from reusing the same ID. This will lead to problems. For instance, $('#selector') will only return the first of many elements with the same ID.

Without the accompanying HTML, we cannot make assumptions as to how the compare logic works. This is as far as the review goes.

Stack Exchange Network

Add/remove class event handler

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Add/remove class event handler

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions