3
\$\begingroup\$

I've recently just started with AngularJS and the best way for me to learn is to get some criticism to find out how I can do things better. I did a small project that calls a third party API and shows a bit of the return data.

HTML

  <body ng-controller="Album">
    <div class="container">
      <input id="artist" type="text" placeholder="Enter artist name" ng-model="artist">
      <button ng-click="getAlbums()">Get Albums</button><br/><br/>
      <div id="albums" ng-repeat="album in albums">
        <div ng-if="albums.length > 0">
        <h2>Album Info</h2>
            {{album.name}}<br/>
            <img src="{{album.image}}">
        </div>
      </div>
    </div>
  </body>

Controller

jamendoApp.controller('Album', function($scope, $http) {
  $scope.getAlbums = function() {
    $http.get("http://api.jamendo.com/v3.0/albums/?client_id=b6747d04&format=jsonpretty&artist_name=" + $scope.artist)
         .success(function(data) {
            if(data.headers.status == 'success') {
              $scope.albums = data.results;
            }
            else {
              alert("Error - " + data.headers.error_message);
            }
         })
         .error(function(x, status, error) {
           alert(error);
         });
     };
});

What I'm specifically interested in knowing is:

  • Am I writing the JavaScript for the controller correctly? It seems to work, but I can't help but feel like I'm not writing it the way actual JavaScript developers would.
  • Am I using the AngularJS directives correctly in the HTML? I'm especially concerned if I'm using the ng-if as it's intended.
  • Plus anything that may catch your eye that could be improved.

To view the whole application feel free to view it on GitHub.

\$\endgroup\$
2
\$\begingroup\$

Some pointers:

  • Move $http.get out of the controller to a service. Stuff that is not presentation logic should not be in your controller.

  • getAlbums should probably be a method of the controller and not the $scope , see this video on egghead.io on how to use controller as syntax.

  • I don't really see the point in your ng-if="albums.length > 0". Why is it there?
  • It's common practice to postfix your controllers - so AlbumController or AlbumCtrl are more common names. I usually try to stick to conventions like this to convey intent more clearly.
  • If you're considering minification - consider using the minification syntax - start by reading this.
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for tips, Ben! I should have mentioned that I was in the process of moving the $http.get to a service. Would you also use a factory for that? For the ng-if directive, I was just more or less playing with that, but I thought it would have been best to not have the "Album Info" display before hitting the button. \$\endgroup\$ – Jon Oct 14 '13 at 23:39
  • 1
    \$\begingroup\$ Your ng-repeat should handle that, before there are any albums you should only see a comment in your HTML where the repeated parent div would go. \$\endgroup\$ – Jason Goemaat Oct 28 '13 at 21: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.