5
\$\begingroup\$

This is a userscript that simply adds the number of pages that a thread has next to its title for Flickr interface. (FWIW it's not really done because the script is only loaded at page load, but not reloaded on subsequent AJAX calls that the user may make).

I'm far from being a seasoned JS developer, so I'm looking for guidelines to potentially bring the script to state-of-the-art JS standards.

Example of questions that I'm not sure about:

  • Should I use promises?
  • Is the workflow done well?
  • Is the inline CSS/HTML evil in this case?
  • Should I omit jQuery?
  • Am I making too many jQuery calls? (i.e. should I just make one query and find the next rows through siblings or something similar?)

Any pointers are appreciated! (this is also my first userscript, so feedback about the userscript itself is also appreciated)

// ==UserScript==
// @name        Flickr - Number of pages in a thread
// @namespace   flickr_
// @description This user script is simply displaying the number of pages in a thread on the main page of the group
// @include     http*://*flickr.com/groups/*
// @version     1
// @grant       none
// @run-at      document-start
// @require     http://ajax.googleapis.com/ajax/libs/jquery/1.2.6/jquery.js
// ==/UserScript==


var api_key = 'fe753c0dd6068fb6c74cb70458b1b9a0';
var POSTS_PER_PAGE = 100;


function call_flickr(method, params, callback) {
    'use strict';
    var url = "https://api.flickr.com/services/rest?";
    params.api_key = api_key;
    params.format = 'json';
    params.nojsoncallback = 1;
    params.method = method;
    $.get(url, params, function (data) {
        callback(JSON.parse(data));
    });
}


function number_of_pages(data) {
    'use strict';
    var url = '/groups/' + data.topics.path_alias + '/discuss/';
    data.topics.topic.forEach(function (topic) {
        var pages_number = Math.ceil(topic.count_replies / POSTS_PER_PAGE);
        var link = url + topic.id + '/';
        var last_link = link + 'page' + pages_number;
        var current_element = $('a[href=' + link + ']');
        if (pages_number > 1) {
            current_element.after(' <a style="font-size:12px; color:#aaa;" href="' +
                    last_link + '">' + pages_number + '</a>');
        }
    });
}


function threads_list(data) {
    'use strict';
    var group_id = data.group.id;
    var params = {};
    params.group_id = group_id;
    call_flickr('flickr.groups.discuss.topics.getList', params, number_of_pages);
}


function main() {
    'use strict';
    var params = {};
    params.url = document.URL;
    call_flickr('flickr.urls.lookupGroup', params, threads_list);
}


main();
\$\endgroup\$
2
\$\begingroup\$

First at all I not so used to usercript. But in general I think your solution is overengineered.

I can point:

  • Readability of code is not good.
  • The "standar" for JavaScript usually is camel case.

Regarding your questions:

  1. You don't have any complicated workflow or expecting several answers, so I wouldn't use promises.
  2. As I said before, I think your code is really hard to read for only being three functions
  3. You could remove the HTML in line creating the elements by JavaScript, but I don't think is so much for calling it "evil"...
  4. You could
  5. Don't think so

Few changes I would made:

Only declare one, 'use strict'

var url = "https://api.flickr.com/services/rest?";

var POSTS_PER_PAGE = 100;

var params = {
    url: document.URL,
    api_key: 'fe753c0dd6068fb6c74cb70458b1b9a0',
    method: 'flickr.urls.lookupGroup',
    format: 'json',
    nojsoncallback: 1
};

Whit this change I don't think that you are gaining so much whit your current function call_flickr.

So I would split it in two functions each one whit the call. Like this:

'use strict';

var url = "https://api.flickr.com/services/rest?";

var POSTS_PER_PAGE = 100;

var params = {
    url: document.URL,
    api_key: 'fe753c0dd6068fb6c74cb70458b1b9a0',
    format: 'json',
    nojsoncallback: 1
};


function numberOfPages(data) {
    var url = '/groups/' + data.topics.path_alias + '/discuss/';
    data.topics.topic.forEach(function (topic) {
        var pages_number = Math.ceil(topic.count_replies / POSTS_PER_PAGE);
        var link = url + topic.id + '/';
        var last_link = link + 'page' + pages_number;
        var current_element = $('a[href=' + link + ']');
        if (pages_number > 1) {
            current_element.after(' <a style="font-size:12px; color:#aaa;" href="' +
                    last_link + '">' + pages_number + '</a>');
        }
    });
}


function threadsList(data) {
    params.group_id = data.group.id;
    params.method = 'flickr.groups.discuss.topics.getList';

    $.get(url, params, function (data) {
        numberOfPages(JSON.parse(data));
    });
}


function main() {

    params.method = 'flickr.urls.lookupGroup';

    $.get(url, params, function (data) {
        threadsList(JSON.parse(data));
    });

}


main();
\$\endgroup\$

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.