3
\$\begingroup\$

Just seeing if there is anyway to shorten these for statements?

...      
      fillMonthSelect: function() {
        var month = new Date().getMonth() + 1;

        for (var i = 1; i <= 12; i++) {
          $('.card-expiry-month').append($('<option value="'+i+'" '+(month === i ? "selected" : "")+'>'+i+'</option>'));
        }
      },

      fillYearSelect: function() {
        var year = new Date().getFullYear();

        for (var i = 0; i < 12; i++) {
          $('.card-expiry-year').append($("<option value='"+(i + year)+"' "+(i === 0 ? "selected" : "")+">"+(i + year)+"</option>"));
        }
      },
...
\$\endgroup\$
3
\$\begingroup\$

String building in code is typically messy, especially without a native String.Format() or sprintf() in Javascript. Building HTML is the worst offender, too.

Other than using a templating engine, like ICanHaz.js (which this use case alone doesn't really justify), you could also build a helper function for creating option tags.

function buildOption(value, isSelected) {
    return "<option value='"+value+"' "+(isSelected ? "selected" : "")+">"+value+"</option>";
}

If you're building a lot of these drop-down options, it might makes things more readable:

for (var i = 0; i < 12; i++) {
    var option = buildOption(i + year, i === 0);
    $('.card-expiry-year').append(option);
}

It's a good idea to avoid long lines/sections of code which have a simple purpose, but have to be read carefully to understand what they do (complicated regular expressions are a good example).

Extracting that functionality into a function avoids this, because the function signature is all someone would need to read to understand what it does.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

There are a couple of things I'd do to pretty up the code. First, you are looking up the same element with every loop iteration. I know it's common, but it is very inefficient and leads towards long lines.

I moved a couple of things into variables, with also split up those long lines and made the code far more readable, IMHO.

Try something like this instead:

  fillMonthSelect: function() {
    var month = new Date().getMonth() + 1;
    var el = $('.card-expiry-month');
    var selectedFlag;

    for (var i = 1; i <= 12; i++) {
      selectedFlag = (month === i ? "selected" : "");
      el.append($('<option value="'+i+'" '+ selectedFlag +'>'+i+'</option>'));
    }
  },

The next step would be to get a formatString function, so you could simply write something like:

  formatString("<option value='{0}' {1}>{0}</option>", i, selectedFlag);
| improve this answer | |
\$\endgroup\$

Your Answer

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

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