2
\$\begingroup\$

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');
  }
})
\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – ferada Nov 16 '16 at 18:24
  • 2
    \$\begingroup\$ Check this code. Edit the question to include corresponding HTML, it may help to make the code better. \$\endgroup\$ – Tushar Nov 17 '16 at 3:06
  • 3
    \$\begingroup\$ 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. \$\endgroup\$ – Mike Brant Nov 18 '16 at 20:25
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Garry Nov 21 '16 at 22:09
1
\$\begingroup\$
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.

\$\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.