4
\$\begingroup\$

I'm fairly new to JQuery/JavaScript and I'm trying to refactor the following code:

$('#hour').click(function() {
  $('#clockHours').slideToggle();
  $('#clockMinutes').hide();
});

$('#minutes').click(function() {
  $('#clockMinutes').slideToggle();
  $('#clockHours').hide();
});

//print clicked onto views
$('.timeSelect').click(function() {
  var text = $( this ).text();
  $('#hourView').val( text );
});

$('.minuteSelect').click(function() {
  var text = $( this ).text();
  $('#minuteView').val( text );
});

I haven't had much luck figuring out how to improve the code. Here is a CodePen of the current working code and here's what I have tried.

\$\endgroup\$
5
  • \$\begingroup\$ What do you dislike about the current code? Because given that you are using jquery, and given the scope of this problem, your solution seems clear and readable. \$\endgroup\$
    – Jonah
    Commented Nov 11, 2015 at 22:30
  • 1
    \$\begingroup\$ There is no much code to refactor. I would suggest to avoid duplicated selectors storing them into variables. \$\endgroup\$
    – kosmos
    Commented Nov 12, 2015 at 6:45
  • \$\begingroup\$ My goal was to refactor the code in a way that would allow me to,for example, add an extra button with the same functions without having to write a new, separate function. I will continue to work on it and possibly bump the topic once I can start to make sense of my requirements and code. \$\endgroup\$
    – Lgalan90
    Commented Nov 12, 2015 at 20:03
  • \$\begingroup\$ The JS in the CodePen is more extensive than the code in the question. Do you have a reason for that? What exactly do you want us to review? \$\endgroup\$
    – Mast
    Commented Nov 18, 2015 at 12:41
  • \$\begingroup\$ The JS in the current working code CodePen includes extra functions for other parts of the clock. What I am trying to do is create a function that will allow the hour and minutes drop down to work (including any other buttons I may add in the future) through one function. \$\endgroup\$
    – Lgalan90
    Commented Nov 23, 2015 at 16:59

1 Answer 1

4
\$\begingroup\$

There a few simple things you could do to help clean up this code. The first thing I always recommend is to create a closure for your script using an IIFE. This allows your code to run in its own scope and keeps everything out of the global scope. You can also pass in jQuery to the function expression and safely refer to it as $.

(function($, undefined ) {
  // pass in undefined for very old (ES3) browsers
  // code goes here
})( jQuery );

The next thing you should consider is caching your selections. Since accessing the DOM is one of jQuerys slowest operations, you want to minimize this as much as possible. Anything you refer to more than once, you should cache.

var $min = $('#clockMinutes');
var $hr = $('#clockHours');

I prefix all of my jQuery selections with $ so I know they are jQuery objects.

You should also start using on for defining your events instead of the shorthand of click. Behind the scenes, click uses on so just skip the middle man.

$min.on('click', function() {
  //code here
});

You might also want to consider DRYing your code. Since the click functions on both the clockMinutes and clockHours are so similar, you can combine them and just pass in the "main" element you want to work on. For example:

function updateScreen( $el ) {
  $el.slideToggle();
  if ( $el === $hour ) { 
    $min.hide();
  } else {
    $hour.hide();}
  }
}

Here is an updated code sample based on the above. Note: I call an init function on document.ready to kick off the functionality.

(function( $, undefined ) {
  //place holder variables for our selections
  var $min, $hour, $minView, $hrView;

  function init() {
    $min = $('#clockMinutes');
    $hour = $('#clockHours');
    $minView = $('#clockMinutes');
    $hrView = $('#minuteView');
    $('#hour').on('click', function() { updateScreen($hour); });
    $(' #minutes').on('click', function() { updateScreen($min); });
    $('.timeSelect').on('click', function() { updateView($hrView); });
    $('.minuteSelect').on('click', function() {updateView($minView);});
  }

  function updateView( $view ) {
    $view.val( $(this).text() );
  }

  function updateScreen( $el ) {
    $el.slideToggle();
    if ( $el === $hour ) { 
      $min.hide();
    } else {
      $hour.hide();
    }
  }

  $(function() {
    init();
  });

})(  jQuery );

Hope you find this helpful. Let me know if you have any questions.

\$\endgroup\$
1
  • \$\begingroup\$ I've read the articles and found caching hint very helpful for me. Thanks for your informative answer Gary! I'll be working on making sense of the way your answer works and try to learn from it. \$\endgroup\$
    – Lgalan90
    Commented Nov 30, 2015 at 18:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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