0
\$\begingroup\$

I'm a .net programmer with littler experience in AngularJs and Javascript.

I have implemented an AlertService, AlertsController and Alerts view for alerts/notifications in my system. PaymentService is a service that uses the alert service to notify user of things.

Could you please give me any comments about my code?
Am I following best practises in AngularJs and Javascript? Should I handle errors in my services?

Here is my code:

alertService.js

'use strict';

app.service('AlertService', function () {

    var handlers = [];

    this.subscribe = function (handler) {
        handlers.push(handler);
    }

    this.success = function (message) {
        pushAlert({ type: 'success', message: message });
    };

    this.info = function (message) {
        pushAlert({ type: 'info', message: message });
    };

    this.warning = function (message) {
        pushAlert({ type: 'warning', message: message });
    };

    this.danger = function (message) {
        pushAlert({ type: 'danger', message: message });
    };

    function pushAlert(alert) {
        handlers.forEach(function (handler) {
            handler(alert);
        });
    }
});

alertsController.js

'use strict';

app.controller('AlertsController', ['$scope', 'AlertService', function ($scope, alertService) {

    $scope.alerts = [];

    alertService.subscribe(function (alert) {
        $scope.alerts.push(alert);
    });

    $scope.closeAlert = function(index) {
         $scope.alerts.splice(index, 1);
    };

}]);

alerts.html

<div ng-controller="AlertsController">
    <uib-alert ng-repeat="alert in alerts" type="{{alert.type}}" close="closeAlert($index)">{{alert.message}}</uib-alert>
</div>

paymentService.js

'use strict';

app.service('PaymentService', ['$http', 'AlertService',
    function ($http, alertService) {
        this.pay = function (id, paymentMethod, numberOfInstallments, creditCard, pin) {
            return $http.post('/api/payment',
                {
                    id: id,
                    paymentMethod: paymentMethod,
                    numberofInstallments: numberOfInstallments,
                    creditCard: creditCard,
                    pin: pin
                })
                .then(success, error);

            function success() {
                alertService.success('Payment confirmed.');
            }

            function error(response) {
                alertService.success(response.data);
            }
        };
    }
]);
\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Mar 18 '16 at 13:29
  • \$\begingroup\$ @Vogel612 thank you for your remark. I did not change my code to incorporate an answer. I added more code and an additional question before the question got answered and then I fixed one thing that the question was not about(alert.danger instead of alert.success). So I felt I was int he right... \$\endgroup\$ – Jakob Mar 21 '16 at 8:52
  • \$\begingroup\$ I did see the change you made as subject of Joseph's answer. Even minor changes are not permissible by site policy. In either case if you have good arguments for why the edit I rolled back is not invalidating, you're welcome to bring it up on Code Review Meta or maybe resolve this with Joseph :) \$\endgroup\$ – Vogel612 Mar 21 '16 at 11:38
1
\$\begingroup\$

So far, your code looks good. But here's some suggestions.

It's usually overkill to pull out success and error callbacks. You can simply inline them. Also, if you happen to have ES6 available via transpiler or if you just don't care about older browsers, by all means use it. It makes the code a lot shorter.

return $http.post('/api/payment', {
  id,
  paymentMethod,
  numberOfInstallments,
  creditCard,
  pin
}).then( _ => {
  alertService.success('Payment confirmed.');
}, _ => {
  alertService.success(response.data);
});

Angular also allows for implicit dependencies. Instead of an array of dependency names and a function at the end of the array, you can simply provide a function whose arguments are the name of the dependencies. Saves you keystrokes.

app.service('PaymentService', ['$http', 'AlertService', function ($http, alertService) {...}]);

app.service('PaymentService', function($http, AlertService){...});
\$\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.