3
\$\begingroup\$

I have a button dropdown that you can select different currencies from. When you select one, it replaces the button value with that currency type, and displays the currency symbol in a span in front of an input.

I have done some refactoring where I now have just one function getCurrency() that accepts parameters and inserts the proper values, but in here $(".dropdown-menu li").click( function(){...} I have 3 different if else statements. I know there has to be a way to do this by looping through my currencyData array and use this to target a specific element, but I cannot figure it out.

I want to make this more modular so it can scale up. If I had 30 different currencies, this would not work. I would have to change too many lines of code, where I should just be able to add another currency to the array.

It is live, you can view it here.

JS

var currencyData = [
  {
    "currency": "USD",
    "symbol" : "$"
  },
  {
    "currency": "EUR",
    "symbol" : "€"
  },
  {
    "currency": "JPY",
    "symbol" : "¥"
  }
];

/*
 * Inserts <li> list of values into the currency dropdown
*/
$.each( currencyData, function( key, val ) {

  var $li = $("<li name='currency'><a href='#'>" +
    val.currency + "</a></li>");

  $(".dropdown-menu").append( $li );
 });

/*
 * Generate currency symbol and type and swap them depending on which is clicked
*/
$(".dropdown-menu li").click( function() {

 if ( this.textContent === currencyData[0].currency ) {
    getCurrency(currencyData[0].currency, currencyData[0].symbol);

 } else if ( this.textContent === currencyData[1].currency ) {
    getCurrency(currencyData[1].currency, currencyData[1].symbol);

 } else if ( this.textContent === currencyData[2].currency ) {
    getCurrency(currencyData[2].currency, currencyData[2].symbol);
 };
});

/*
 * Swap currency type and symbol
*/
function getCurrency(currency, symbol) {
  // inserts visible currency symbol to form
  var $span = $("<span class='js-currency-symbol' > " + symbol + "</span>");

  // hidden input to save currency symbol
  var $input = $("<input type='text' class='js-hidden-currency-symbol' value=" +
   symbol + " style='display:none' name='currencySymbol'>");

  $('.js-currency-symbol').remove();
  $('.js-currency-btn').text( currency );
  $('.js-input-amount-prepend').append( $span );

  $('.js-hidden-currency-symbol').remove();
  $('.js-currency-btn-group').append( $input );
};

HTML

<div class="form-group"> <!-- Currency Input Section-->
    <div class="wrapper">
      <span class="js-input-amount-prepend input-amount-prepend">Amount:</span>
      <!-- INSERT  <span class="js-currency-symbol"> -->
      <input type="text" required class="js-payment-amount-input payment-amount-input" name="amount" placeholder="enter amount">
      <div class="btn-group js-currency-btn-group currency-btn-group">
        <button type="button" class="btn js-currency-btn currency-btn">Currency</button>
        <button type="button" class="btn dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
          <span class="caret"></span>
          <span class="sr-only">Toggle Dropdown</span>
        </button>
        <ul class="dropdown-menu">
          <!-- INSERT li Items -->
        </ul>
      </div>
      <!-- INSERT Hidden currency symbol input -->
    </div>
  </div> <!-- End Currency Input Section-->
\$\endgroup\$
1
\$\begingroup\$

Work with indices instead of elseif statements

You should write a function that identifies the index of this.textContent in the currencyData array. Why not try something like this:

function myIndexOf(o) {    
    for (var i = 0; i < currencyData.length; i++) {
        if (currencyData[i].currency == o) {
            return i;
        }
    }
    return -1;
}

Taken from this Stackoverflow answer.

Then you can run:

currentIndex = myIndexOf(this.textContent)
getCurrency(currencyData[currentIndex].currency, currencyData[currentIndex].symbol);

And it's entirely extensible.

Avoid unnecessary HTML updates

Seems like you can reduce $span to only update the innerHTML of a span element rather than reinitializing your span element each time you append it to $('.js-input-amount-prepend'). This would slightly shorten getCurrency

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. It took me a bit to digest what was happening but I got it and it works great. I understand what you're saying about the html. I will work on that now. \$\endgroup\$ – max Jul 16 '15 at 17:22

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.