HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Show number of pages next to Flickr threads

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
shownumberthreadsnextpagesflickr

Problem

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_numb

Solution

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:

  • You don't have any complicated workflow or expecting several answers, so I wouldn't use promises.



  • As I said before, I think your code is really hard to read for only being three functions



  • You could remove the HTML in line creating the elements by JavaScript, but I don't think is so much for calling it "evil"...



  • You could



  • 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(' ' + pages_number + '');
        }
    });
}

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();

Code Snippets

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
};
'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();

Context

StackExchange Code Review Q#93965, answer score: 2

Revisions (0)

No revisions yet.