2
\$\begingroup\$

Super-duper javascript noob here. I'm trying to fetch public repos from GitHub, and add them to a dl. I know this won't work on every website due to cross-site scripting, but GitHub offers a JSON-P API. I'm fairly certain that I'm doing this mostly the right way (it works), but the append function looks ugly, and I'm not sure if the way I'm triggering the script is considered best practice.

gh-project.js:

function get_github_public_repos(username) {  
    var the_url = "https://api.github.com/users/" + username + "/repos"

    $.ajax({
      url: the_url,
      dataType: 'jsonp',
      type: 'get',
      success: function(data) {
        for(i = 0; i < data.data.length; i++){
          var repo = data.data[i];
          if (!repo.fork) {
            $('#gh-projects').append("<dt><a href='" + repo.url +"' title='" + repo.name + "'>" + repo.name + "</a>" + "</dt>" + 
            "<dd class='project-description'>" + repo.description + "</dd>" );
          }
        }
      }
    });
}

projects.html:

    <!-- snip -->
    <dl class="project-list" id="gh-projects">
        <!-- To be filled in by Javascript. Hooray! -->
    </dl>
</section>
<script src="/js/gh-projects.js"></script>
<script>get_github_public_repos('gfontenot')</script>
<!-- end of file -->

Thanks for the pointers.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Your code looks good, and I'm not going to suggest comestic changes. However:

  1. the_url is not informative enough, try to find a variable that conveys that this url will let you fetch the repositories of an user.
  2. Did you consider that repo.name could contain a ' which would cause you to output wrong html? Fortunately, GitHub prevents such things to happen.
  3. Declare var i; at the top of your success function. Here's why.
  4. The way you're building your html string is dangerous. It's also hard to read and easy to get wrong.

    As Philip suggested, using some sort of template would be a good idea, but there's no way to do this in standard jQuery (jQuery templates have been deprecated, and the way forward seems to be JsRender which is not yet Beta). Fortunately, your code is not big enough for you to need that. Instead, use attr(), text() and addClass() wisely:

    var dt = $('<dt>').attr('href', repo.url).attr('title', repo.name).text(repo.name);
    var dd = $('<dd>').addClass('project-description').text(repo.description);
    $('#gh-projects').append(dt).append(dd);
    
\$\endgroup\$
3
  • \$\begingroup\$ Just realized, the dt var doesn't allow for a link. What's the best way to add an a tag into that dt? \$\endgroup\$ Mar 8, 2012 at 0:39
  • \$\begingroup\$ Would var a = $('<a>').attr('href', repo.url).attr('title', repo.name).text(repo.name); and then var dt = $('<dt>').append(a); be the right way to handle this? \$\endgroup\$ Mar 8, 2012 at 0:46
  • 1
    \$\begingroup\$ Sounds good! You definitely need an <a>. \$\endgroup\$ Mar 8, 2012 at 5:53
1
\$\begingroup\$

Looks fine dude,

Peronsal pref, although like you I am NO javascript guru, however...

(function($){

    var GIt = {
        init: function(){
            this.get_repo_list();
        },
        get_repo_list : function(){

               var uri = "https://api.github.com/users/", 
                   loadRepo = function(u){
                        var u= (u) ? u: 'default';
                        $.ajax({
                            url: uri + u + '/repos',
                            dataType : 'jsonp',
                            type : 'GET',
                            success : function(callback){
                               var dl  = $("#gh-projects"),
                                   elem = '...';

                             //Use Jquery tmpl maybe?
                             ...dl.append(elem);
                            }
                        });
                   }
                   loadRepo('username');
        }
    }

    $(function(){
       GIT.init();
    })
})(jQuery)
\$\endgroup\$

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.