"Enter key pressed" event-handler

Question

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?

Answer 1

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);
 }
 });
});