2
\$\begingroup\$

I am writing an "Enter key pressed" event-handler, so when user presses the "enter" key in the input of type text, this code is called:

    var email = "";
    var subscriberInput = $("input[name='subscriber_email']")
    // Subscribe RSS
    subscriberInput.keyup(function(e){
        if(e.which == 13){ // Enter key was pressed
            email = subscriberInput.val();
            $.post("/en/subscriber/add", { email: email }).done(function(data){
                 if(data == "success"){

                       subscribeBox.find('div.row').remove();

                       if(isEnglish){
                           subscribeBox.find('h5').text("You're subscribed!").addClass('green-text');
                       } else {
                           subscribeBox.find('h5').text("Sunteti abonat cu success").addClass('green-text');
                       }

                       setTimeout(function(){
                           subscribeBox.fadeOut('slow');
                       }, 5000)
                 } else {
                     alert(data);
                 }
            });
            return false;
        }
    });

How can I improve this code?

\$\endgroup\$
2
\$\begingroup\$

A few things to note before I explain improvements

  • Never forget var when declaring variables. I saw this in the code.

    email = subscriberInput.val();
    

    Now, best case scenario is that you declared email somewhere in the higher scopes. However, if it wasn't then JS will declare it as a global, which may have unforseen negative effects.

  • If you run a site that has translation capabilities, I suggest you don't hard-code messages. Use translation libraries instead. That way, you can globally change language with just a flick of a switch.

Then here's the improved code, given what you have:

subscriberInput.keyup(function (event) {

  // If you intent to prevent the default action of the enter, then I
  // suggest preventDefault() instead of return false
  event.preventDefault();

  // I recommend the "return if fail" pattern rather than "run when true"
  // because it removes unnecessary indentation. It's a case to case basis.
  if (event.which !== 13) return;

  $.post("/en/subscriber/add", {
    // The context of the function is the input box. We can simply use
    // the value property instead of calling out val()
    email: this.value
  }).done(function (data) {

    // Similar to above, use strict comparison as much as possible
    if (data === "success") {

      // Avoid repeating code by factoring out common code. In this case,
      // what set the code apart was the message.
      var message = isEnglish ? "You're subscribed!" : "Sunteti abonat cu success";

      subscribeBox.find('div.row')
                  .remove();

      // jQuery actually has a delay() function which can delay animations.
      // Better than using timers manually.
      subscribeBox.find('h5')
                  .addClass('green-text')
                  .text(message)
                  .delay(5000)
                  .fadeOut('slow');

    } else {

      // Now I don't know why you are doing the alert. If you are debugging,
      // I suggest you use console.log() or a custom logging library.
      alert(data);
    }
  });

});
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy