5
\$\begingroup\$

I am fairly new to AngularJS and I am trying to improve my knowledge of this JavaScript Framework by building small real-world web-apps.

In this case, I have built a currency converter app with harnesses an exchange rate API built by me which runs here.

I want to know whether there exists a scope of improvement and optimization in my code.

app.js

var app = angular.module('QConvert', ['qconvertctrlmodule']);

qconvertctrl.js

var app = angular.module('qconvertctrlmodule', [])
    .controller('QConvertController', function($scope, $http, $log, $interval) {

        $scope.currencyObject = {
                from : "",
                to  : "",
                amount : "",
                amount_converted : "",
                exchangeRate : ""
        };

        $scope.currencyCodes = [{value : 'INR', display : 'Indian Rupee'}, {value : 'USD', display : 'US Dollar'}, {value : 'GBP', display : 'British Pound'}];

        $scope.getexchangerate = function() {

            $log.info("FROM : " + $scope.currencyObject.from);

            $log.info("TO : " + $scope.currencyObject.to);

            $http.get("http://api.decoded.cf/currency.php", {params : {from : $scope.currencyObject.from,
                to : $scope.currencyObject.to, amount : 1}})
                .then(function(response) {

                    $scope.currencyObject.exchangeRate = response.data.amount_converted;

                    $log.info(response.data.amount_converted);


            });

        };


        $interval(function() {
            $scope.getexchangerate();
            console.log('Exchange rates refreshed!');
        }, 5000);

    });

app.filter('toDecimal', function() {
    return function(input, precision) {
        return input.toFixed(precision);
    };
});

index.html

<div class="container">
      <div class="page-header">
        <h1>QConvert</h1>
      </div>
      <p class="lead">A Seamless Currency Converter</p>
      <div class="row" ng-controller="QConvertController">
      <div class="col-md-8 col-md-offset-2">
        <div class="form-group">
          <label for="amount">Amount</label>
          <input type="number" step="any" class="form-control" id="amount" ng-model="currencyObject.amount">
        </div>
      </div>
        <div class="col-md-8 col-md-offset-2">
            <div class="form-group">
             <label for="from">From</label>
              <select class="form-control" id="from" ng-model="currencyObject.from" ng-change="getexchangerate()">
                <option ng-repeat="currencyCode in currencyCodes" value="{{currencyCode.value}}">{{currencyCode.display}}</option>
              </select>
            </div>
        </div>

        <div class="col-md-8 col-md-offset-2">
            <div class="form-group">
             <label for="to">To</label>
              <select class="form-control" id="to" ng-model="currencyObject.to" ng-change="getexchangerate()">
                <option ng-repeat="currencyCode in currencyCodes" value="{{currencyCode.value}}">{{currencyCode.display}}</option>
              </select>
            </div>
        </div>

        <br>
        <br>
        <br>
        <div class="col-md-8 col-md-offset-2" ng-show="currencyObject.amount">
            <h3 class="display-4" ng-If="currencyObject.from != currencyObject.to">{{currencyObject.amount}} {{currencyObject.from}} <span ng-show="currencyObject.to">=</span> <span ng-show="(currencyObject.amount * currencyObject.exchangeRate)">{{(currencyObject.amount * currencyObject.exchangeRate)  | toDecimal:3}}</span> {{currencyObject.to}}</h3>
            <h3 class="display-4" ng-If="(currencyObject.from == currencyObject.to && currencyObject.from !== undefined && currencyObject.to !== undefined)">{{currencyObject.amount}} {{currencyObject.from}} = {{currencyObject.amount}} {{currencyObject.from}}</h3>
        </div>

      </div>
    </div>

The app is running live here.

\$\endgroup\$
  • \$\begingroup\$ Consider using the currency filter to display the numbers with currency symbols as prefixes. \$\endgroup\$ – Dan Pantry Aug 11 '16 at 13:53
  • \$\begingroup\$ the only input I really have is you could use the angular $number filter as opposed to this custom one you've built. \$\endgroup\$ – aaronmallen Aug 18 '16 at 1:18
1
\$\begingroup\$

Feedback #1:

Angular can actually build the select for you without the repeat via the ng-options attribute directive:

<select class="form-control" 
        id="to" 
        ng-model="currencyObject.to" 
        ng-change="getexchangerate()" 
        ng-options="currencyCode.value as currencyCode.display for currencyCode in currencyCodes"></select>

Feedback #2:

Angular provides a filter for formatting numbers, $number, so no need to write your own.

Feedback #3:

Use proper camelCase for function names (i.e. getExchangeRate instead of getexchangerate). As well as don't mix your snake_case and camelCase. Use snake_case for CONSTANTS (of which you have none) (i.e. $scope.currencyObject.amountConverted instead of $scope.currencyObject.amount_converted).

Feedback #4:

Remove unused $scope properties. You never set nor call $scope.currencyObject.amount_converted, so get rid of it.

Feedback #5:

You're making a request without having all of your params. If I change the value of the first select, but haven't changed the value of the second you are firing the promise each time. This can easily be fixed with a simple if statement:

// Set initial values null, so they return false.
$scope.currencyObject = {
  from : null,
  to  : null,
  amount : 0,
  exchangeRate : null
}

$scope.getExchangeRate = function() {
  $log.info("FROM : " + $scope.currencyObject.from);
  $log.info("TO : " + $scope.currencyObject.to);
  var options = {
    url: 'http://api.decoded.cf/currency.php',
    method: 'GET',
    params: {
      from : $scope.currencyObject.from,
      to : $scope.currencyObject.to, 
      amount : $scope.currencyObject.amount
    }
  };
  if ($scope.currencyObject.from && $scope.currencyObject.to) {
    $http(options).then(function(response) {
      $scope.currencyObject.exchangeRate = response.data.amount_converted;
      $log.info(response.data.amount_converted);
    });
  }    
};

Feedback #6:

You have no error handling around your $http request, but on an app this simplistic that may be by design.

\$\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.