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
        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('h5').text("You're subscribed!").addClass('green-text');
                       } else {
                           subscribeBox.find('h5').text("Sunteti abonat cu success").addClass('green-text');

                       }, 5000)
                 } else {
            return false;

How can I improve this code?


1 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

  // 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";


      // jQuery actually has a delay() function which can delay animations.
      // Better than using timers manually.

    } 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.


Your Answer

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