6
\$\begingroup\$

The goal here is to, add a credit card image, and remove all other credit card images, of a certain input field. I had a even larger code, with a lot of addClass() and removeClass()... then, I shrink it until I have this. Still, I feel it's still a huge block of code.

The point here is to short things up, but NOT until it's hard to understand what's going on.

if (event.isValid) {

    if (event.card.type == 'visa') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-visa'));

    } else if (event.card.type == 'master-card') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-master-card'));

    } else if (event.card.type == 'e-american-express') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-american-express'));

    } else if (event.card.type == 'diners-club') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-diners-club'));

    } else if (event.card.type == 'discover') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-discover'));

    } else if (event.card.type == 'jcb') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-jcb'));

    } else if (event.card.type == 'unionpay') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-unionpay'));

    } else if (event.card.type == 'maestro') {

        $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-maestro'));
    }

} else {

    $("#card-number").removeClass(CREDIT_CARD_LIST);
}
\$\endgroup\$
2
  • \$\begingroup\$ Is } else if (event.card.type == 'e-american-express') { a typo in that it should be == 'american-express'. \$\endgroup\$
    – Quill
    Commented Sep 3, 2015 at 9:44
  • \$\begingroup\$ I don't understand CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-visa') wouldn't it be the same as just adding 'e-visa' ? \$\endgroup\$
    – the_lotus
    Commented Sep 3, 2015 at 12:58

2 Answers 2

5
\$\begingroup\$

Create a map from card.type to what class it should be:

var cardTypeToClass = {  'visa': 'e-visa',
                         'master-card': 'e-master-card',
                         //...
                       };

This creates an associative array so if you give it a key value then you can get out the associated value.

in your case that is done like:

if (event.isValid) {
    $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, cardTypeToClass[event.card.type]));
} else {
    $("#card-number").removeClass(CREDIT_CARD_LIST);
}

This concentrates the mapping into a single section and separates out what the code actually does.

\$\endgroup\$
2
  • \$\begingroup\$ I don't understand how "cardTypeToClass[event.card.type]" will know what kind it is that we have to show. Care to explain a little further? \$\endgroup\$
    – user82648
    Commented Sep 3, 2015 at 8:34
  • \$\begingroup\$ I would extract a variable: var cardType = cardTypeToClass[event.card.type]; $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, cardType)); \$\endgroup\$
    – ANeves
    Commented Sep 3, 2015 at 9:40
4
\$\begingroup\$

It seems the value you're trying to replace for is really just the event.card.type prefixed by e-, so just string concatenate them together.

However, if some of the replace variables do not follow this pattern, it'd be best to use @ratchet freak's solution.

if (event.isValid) {
    $("#card-number").addClass(CREDIT_CARD_LIST.replace(CREDIT_CARD_LIST, 'e-' + event.card.type));
} else {
    $("#card-number").removeClass(CREDIT_CARD_LIST);
}
\$\endgroup\$
3
  • \$\begingroup\$ e-american-express doesn't follow the pattern \$\endgroup\$ Commented Sep 3, 2015 at 9:43
  • \$\begingroup\$ That looks like a typo though. \$\endgroup\$
    – Quill
    Commented Sep 3, 2015 at 9:44
  • 7
    \$\begingroup\$ If it doesn't follow the pattern, you should strongly consider making it follow the pattern rather than accommodating special cases. \$\endgroup\$ Commented Sep 3, 2015 at 10:29

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.