patternjavascriptMinor

Github Issue Viewer

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

githubissueviewer

Problem

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 "" + item.title + "";
                };
                var resultList = result.map(createItem);
                $('.' + configSettings.container).html("" + resultList + "");
            }
        }); 
    },
    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:


    $(document).ready(function() {
        IssueViewer.fetchIssues();

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

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

    });

Next
Previous

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

Solution

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 `

 are not headings for anything

Semantically,

 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

elements to style them as you need.



Separate your data and presentation

This code is mixing your concerns:

var createItem = function(item) {
    return "" + item.title + "";
};
var resultList = result.map(createItem);
    $('.' + configSettings.container).html("" + resultList + "");
}


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 "" + item.title + "";
    };
    var resultList = result.map(createItem);
    $('.' + configSettings.container).html("" + resultList + "");
}

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

Code Snippets

fetchIssues = function fetchIssues (pageRequest) {

function fetchIssues (pageRequest) {

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

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

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>");
}

Context

StackExchange Code Review Q#107816, answer score: 2

Revisions (0)

No revisions yet.