Recent top tags in Tumblr blog

A friend of mine asked me to write a script for showing the most used tags in her Tumblr blog in the last days. I saw it as a great opportunity for learning JavaScript and accepted it, as it seemed simple enough while being useful.

I "chose" the requirements to be:

• Simple basic usage, not requiring JavaScript knowledge to modify the output (CSS only);

• Fetches the last N posts, or all posts up to N days ago, or both (whichever comes first);

• As a side use case, someone could be interested in "which tags did I use the most in a given period of time?", but in this case some Javascript could be needed.

I also tried to not use any existing library, however I had a problem with cross-domain requests, and found on the web that jQuery would help me with the JSONP protocol, which the Tumblr REST API does implement, and opted to use jQuery in the script. Aside from that, I've written the documentation as jsdoc comments. I also avoided the most recent ECMA6(?) implementations, as it seems most modern browsers do not support it.



`/**
* Display your recently most used tags in your Tumblr blog.
* Requires jQuery.
* @see getTags
* @see display
* @namespace
* @version 1.0.0
* @example
* RecentTags.getTags({
* blog: 'example.tumblr.com',
* maxTags: 5,
* maxPosts: 100
* }, function(data) {
* RecentTags.display(data, '#top-tag-div', 'top-tags', null);
* });
*/
var RecentTags = RecentTags || {

/**
* @typedef GetTagsInput
* @type {Object}
* @property {String} blog - Blog name
* @property {Number} maxTags - Amount of tags to collect
* @property {Number} [maxDays] - Look for posts up to maxDays ago. Either maxDays or maxPosts is required
* @property {Number} [maxPosts] - Look for this amount of posts. Either maxDays or maxPosts is required
* @property {Date} [date] - Date to consider "today". Useful for looking tag usage of a specific month
*/

/**
* Gets the recent tag usage as a pair

Let's bust some myths:

ES2015 is not supported in all major browsers

I will use ES2015 (commonly known as ES6) in my answer. Everything I tell you is supported by the latest stable version of Chrome and Firefox. Also, I'm fairly certain that most of the features I'll discuss have support in several versions back of Chrome and Firefox.

It's worth noting that most of the features are not supported in IE, so if you absolutely must have it, use a polyfill or older tech (i.e. XMLHttpRequest instead of fetch).

I need jQuery for JSONP requests

No, you do not. JSONP is a hack to get around the API author's decision to not supply CORS headers (For whatever reason). So JSONP wraps the response with a function call (of your choosing; example), so it becomes valid JavaScript code. Then all you do is append the code to the DOM with a ` element, and your function is called:

// Fetch API is relatively new. So not supported by IE.
// This can easily be a regular XMLHttpRequest as well.
fetch(`${apiUrlWithParameters}&callback=jsonpHandler`) // faulty, if no prior params
  .then(response => {
    let script = document.createElement('script');
    script.textContent = response;
    document.body.appendChild(script);
  }); // Also, handle errors with `.catch()`

function jsonpHandler(data) {
    // Handle the response here
}

See the fetch API for more details.

I need jQuery for collection iteration

No, you do not.

Mapping over arrays is easy. someArray.map(mappingFunction)

Mapping over objects is also quite easy. Object.keys(someObject).map(key => / key is the key. someObject[key] is the value. /)

I need jQuery for this other thing

No, you do not.

Unless you need older browser support (Older = IE8 and below), you almost certainly do not need jQuery as a hard dependency.

Great, now that we got that out of the way, in no particular order:



• Your fetch function has far too many arguments, it's hard to keep track. Consider passing an options object just like you've done with getTags.




-
It isn't clear what pair[0] and pair[1] are. Give more descriptive names:

let key = pair[0];
let value = pair[1];

-
Template strings are pretty cool and supported in Edge too!

-
Your display function can basically be shortened to return concatinator ? this.displayList(...) : this.displayTable(...);

-
What's the point of turning your nice key: value object into an array of arrays? It makes it less descriptive and less readable (also, less debuggable), while providing no real profit.

-
This

post.tags.forEach(function(tag) {
    data[tag] = (data[tag] || 0) + 1;
});

Smells like you wanted to use reduce().

-
It looks to me you only want to reveal the getTags and display functions from your module. Have a look at the Revealing Module Pattern to only reveal the methods you want exposed.

-
When providing with an options object argument, a good practice to follow is to have a defaults object defined in the function, and then have the passed options object assigned to it.

function(options /*, ... */) {
    let defaults = {
        maxTags: 10,
        maxPosts: 100,
    };
    Object.assign(defaults, options); // options will override defaults
    // snip

-
If required parameters of the options object are missing, you should throw an error (in your example, the blog field appears to be rather important.

-
typeof whatever === 'undefined' should only be used if you aren't sure anyone ever defined whatever, in the case of obj.whatever, if you know obj is defined (which it is, because it's this most of the times in your code), just use this.whatever === undefined.

-
typeof opt.maxDays === 'number' this check is redundant and misleading. opt.maxDays may be NaN and the check will pass. The > 0 check is sufficient.

-
Always use ===. ALWAYS.

-
Your fetch function mutates the data parameter passed to it. It means that the following is not true:

var myData = { ... }; // whatever
this.fetch(/* whatever */, myData);
// myData here is the same as before the fetch.

That's dangerous. Create new objects inside functions and return them. Mutating passed objects is mud you don't want to get into.

-
resp.response It's either that resp is the response, or response` is. In either case, one of them should be changed.

StackExchange Code Review Q#101068, answer score: 2