3
\$\begingroup\$

This is my first angular application. I am sure there must be many mistakes like coding conventions, naming conventions or implementation mistakes like what goes where. Could you please review this application which converts currency from one to another.

Index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Index</title>
    <link rel="stylesheet" href="bootstrap.css">
    <link rel="stylesheet" href="CurrencyConverter.css">
</head>
<body ng-app='myApp'>

    <div ng-controller="CurrencyConverter" class="panel panel-default board">
        <h3>Currency converter</h3>
        <br>    
        <currencycontrol 
            currencyobject = from
            currencies = currencies
            direction = direction
            source = 1
        >
        </currencycontrol>

        <br>

        <currencycontrol
            currencyobject=to
            currencies = currencies
            direction = direction
            source = 0
        >
        </currencycontrol>

        <br>

        <div class="row description">
            <div class="col-xs-12 txt txt-large"><h5>1 {{from.selectedcurrency.description}} equals </h5></div>
            <div class="col-xs-12 txt txt-large"><h3>{{(convert(from.selectedcurrency.currency, to.selectedcurrency.currency, 1)|number:3)+" "+to.selectedcurrency.description}}</h3></div>
        </div>
    </div>
    <script src="angular.js" type="text/javascript"></script>
    <script src="Index.js" type="text/javascript"></script>
</body>
</html>

currencycontrol.html

<div class='row'>
    <div class='col-xs-8'>
        <input type='number' placeholder='{{currencyobject.selectedcurrency.currency}}' class='form-control' ng-model='currencyobject.amount' ng-focus='changeDirection(source)'>
    </div>
    <div class='col-xs-4'>
        <select name='currencySelect' id='currencySelect' ng-model='currencyobject.selectedcurrency'
            ng-options='c.currency for c in currencies' class='form-control' ng-focus='changeDirection(1)'>
        </select>
    </div>
</div>

Index.js

var app = angular.module("myApp", []);

function getCurrencies()
{
    var a = 
    [
        {
            currency: 'INR',
            value: 1,
            description: 'Indian Rupee'
        },
        {
            currency: 'USD',
            value: 0.015,
            description: 'US Dollar'
        },
        {
            currency: 'GBP',
            value: 0.010,
            description: 'British Pound'
        },
        {
            currency: 'AED',
            value: 0.054,
            description: 'United Arab Emirates Dirham'
        },
        {
            currency: 'AUD',
            value: 0.021,
            description: 'Australian Dollar'
        },
        {
            currency: 'EUR',
            value: 0.014,
            description: 'Euro'
        }
    ];
    return a;
}

app.controller('CurrencyConverter', function($scope, $filter)
{
    $scope.currencies = getCurrencies();
    $direction = 0;

    $scope.convert = function(from, to, amount)
    {
        var fromFound = $filter('filter')($scope.currencies, {currency: from}, true);
        var toFound = $filter('filter')($scope.currencies, {currency: to}, true);
        var IndianAmount = 0;
        if(fromFound.length)
            IndianAmount = amount/fromFound[0].value;
        var toAmount = toFound[0].value*IndianAmount
        return toAmount;
    }

    $scope.updateTarget = function()
    {
        $scope.to.amount = 
            $scope.convert(
                $scope.from.selectedcurrency.currency,
                $scope.to.selectedcurrency.currency,
                $scope.from.amount)
            if($scope.to.amount == 0)
                $scope.to.amount = '';
    }

    $scope.updateSource = function()
    {
        $scope.from.amount = 
            $scope.convert(
                $scope.to.selectedcurrency.currency,
                $scope.from.selectedcurrency.currency,
                $scope.to.amount)
            if($scope.from.amount == 0)
                $scope.from.amount = '';
    }

    $scope.from = 
    {
        selectedcurrency : $scope.currencies[0]
    };

    $scope.to = 
    {
        selectedcurrency : $scope.currencies[1]
    };

    $scope.$watch(
        'from.amount',
        function(newValue, oldValue)
        {
            if($scope.direction == 0)
                $scope.updateTarget();
        }
    );



    $scope.$watch(
        function()
        {
            return $scope.from.selectedcurrency.currency;
        },
        function(newValue, oldValue)
        {
            if($scope.direction == 0)
                $scope.updateTarget();
        }
    );

    $scope.$watch(
        function()
        {
            return $scope.to.selectedcurrency.currency;
        },
        function(newValue, oldValue)
        {
            if($scope.direction == 0)
                $scope.updateTarget();
        }
    );

    $scope.$watch(
        function()
        {
            return $scope.to.amount;
        },
        function(newValue, oldValue)
        {
            if($scope.direction == 1)
                $scope.updateSource();
        }
    );
});

app.directive('currencycontrol', function()
{
    return {

        scope:{
            currencyobject:'=',
            currencies:'=',
            direction:'=',
            source:'='
        },

        restrict: "E",

        templateUrl: "currencycontrol.html",

        controller: function($scope)
        {
            $scope.changeDirection = function(source)
            {
                if(source == 0)
                {
                    $scope.direction = 1;
                }
                else
                {
                    $scope.direction = 0;
                }
            }
        }
    };
});

CurrencyConverter.css

.board
{
    width: 700px;
    margin: 100px;
    box-shadow: 0 4px 8px 0 rgba(0, 0, 0, 0.2), 0 6px 20px 0 rgba(0, 0, 0, 0.19);
    background-color: transparent;
    padding-right:40px;
    padding-left:40px;
    padding-top:20px;
    padding-bottom:20px;
}

.description h3
{
    margin:0px;
}

angular.js is of version 1.5

\$\endgroup\$
1
  • \$\begingroup\$ jsfiddle output is jsfiddle.net/nagasandeepbondili/u8jope3u/4 \$\endgroup\$
    – Naga Sandeep
    Commented Feb 5, 2016 at 17:58

1 Answer 1

Reset to default
2
\$\begingroup\$

First, variables. fromFound and toFound doesn't seem clear to me. What are they? I don't remember finding anything in the app.

Over to your numbers, you clip your the numbers below the inputs to 3 decimal places. However, the inputs don't get the same treatment.

Next is your use of $watch. AngularJS already has an internal watch system (I believe they call it "digest"). It periodically checks for changes in the data and recompute anything that changes. Usage of $watch is like putting a "watch in a watch".

Your currency control appears to do too much. It should only know about the value and currency bindings. For switching source and destination, it should accept a function from the controller which it will call when it is focused. However, all the logic for switching who is source and destination is the responsibility of the controller.

Then I see this <br> under the <h3>. <h3> is a block-level element. This means whatever comes after it is forced under it. No need for the <br>. If the <br> is for spacing, use margin or padding instead. Also, <h*> tags are for headings. They're not for sizing. Use CSS to size the elements. Seeing that they're normal text, <span> should suffice.

\$\endgroup\$
0

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.