current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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.

share|improve this question

2 Answers 2

up vote 3 down vote accepted

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);
share|improve this answer
    
Thanks a lot for the pointers. –  Gordon Fontenot Mar 6 '12 at 19:11
    
Just realized, the dt var doesn't allow for a link. What's the best way to add an a tag into that dt? –  Gordon Fontenot Mar 8 '12 at 0:39
    
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? –  Gordon Fontenot Mar 8 '12 at 0:46
1  
Sounds good! You definitely need an <a>. –  Quentin Pradet Mar 8 '12 at 5:53
up vote 1 down vote

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)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.