5
\$\begingroup\$

I have the following javascript code that creates a github issue viewer. Can anyone give me some advice on how to improve the code?

    ;(function (global, $) {
    var currentPage = 1,
    configSettings= {
        'base_url': 'https://api.github.com/repos/',
        'repo': 'npm',
        'repo_user': 'npm',
        'container': 'container'
    },
    fetchIssues = function fetchIssues (pageRequest) {
        var page = pageRequest || 1;
        $.ajax({
            url: configSettings.base_url + configSettings.repo + "/" + configSettings.repo_user + "/issues?page=" + page,
            success: function(result){
                var createItem = function(item) {
                  return "<li><h1>" + item.title + "</h1></li>";
                };
                var resultList = result.map(createItem);
                $('.' + configSettings.container).html("<ul>" + resultList + "</ul>");
            }
        }); 
    },
    next = function next(page) {
        currentPage += 1;
        fetchIssues(currentPage);
    },
    prev = function prev(page) {
        currentPage -= 1;
        fetchIssues(currentPage);
    },
    IssueViewer = {
        fetchIssues: fetchIssues,
        nextPage: next,
        prevPage: prev
    };

    global.IssueViewer = IssueViewer;
}(window, jQuery));

The html page:

<script type="text/javascript">
    $(document).ready(function() {
        IssueViewer.fetchIssues();


        $('.next').click(function () {
            IssueViewer.nextPage();
        });


        $('.prev').click(function () {
            IssueViewer.prevPage();
        });

    });
</script>

<a href = "#" class="next">Next</a>
<a href = "#" class="prev">Previous</a>
<div class="container">
</div>

Is this the right way to build dynamic content in javascript?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Just a few small suggestions, since I think this code is pretty clean and solves your simple problem well.

No need to declare your helper functions using var

Eg, replace:

fetchIssues = function fetchIssues (pageRequest) {

with:

function fetchIssues (pageRequest) {

Since the function is inside your top function, its scope will not leak out. This also gives you the freedom to order those inner helper functions in the source code however you like.

The list's <h1> are not headings for anything

Semantically, <h1> elements are supposed to be headers. Yet there is no content for which they are headers here -- they are simply titles. I would remove them and use CSS on the <li> elements to style them as you need.

Separate your data and presentation

This code is mixing your concerns:

var createItem = function(item) {
    return "<li><h1>" + item.title + "</h1></li>";
};
var resultList = result.map(createItem);
    $('.' + configSettings.container).html("<ul>" + resultList + "</ul>");
}

What you are currently calling IssueViewer should probably be renamed PagedIssues, and its only responsibility should be fetching of the data and paging. It should not depend on jquery. Instead, you can dependency inject the view code that creates the html, perhaps by adding a viewCallback property to your configSettings. Then the offending code above would be rewritten:

$.ajax({
    url: configSettings.base_url + configSettings.repo + "/" + configSettings.repo_user + "/issues?page=" + page,
    success: function(result){
        configSettings.viewCallback(result);
    }
}); 

And the viewCallback, which can depend on jquery, and which you'd pass in via configSettings (btw, you need to add that as a parameter to your main function, so this is possible), would look like this:

function viewCallback(result) {
    var createItem = function(item) {
      return "<li><h1>" + item.title + "</h1></li>";
    };
    var resultList = result.map(createItem);
    $('.' + configSettings.container).html("<ul>" + resultList + "</ul>");
}

This allows you to completely change your view, or create multiple views, without touching the code that handles issues fetching and paging.

\$\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.