current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

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');
  }
})
share|improve this question

bumped to the homepage by Community♦ 2 days ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.
    
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 Nov 16 '16 at 18:24
2  
Check this code. Edit the question to include corresponding HTML, it may help to make the code better. – Tushar Nov 17 '16 at 3:06
2  
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 Nov 18 '16 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 Nov 21 '16 at 22:09
up vote 0 down vote 
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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.