5
\$\begingroup\$

I have used multiple map/flatmap combinations. Could anybody help me with the review?

var Rx = require('rx');
var RxNode = require('rx-node');
var request = require('request');
var _ = require('lodash');
var cheerio = require('cheerio');

exports.getResult = function(req, res) {
    var result = '';
    fetchContent({
            url: req.body.url
        }, req.body.season)
        .flatMap(getEpisodeRange)
        .map(formEpisodeUrl.bind(null, req.body.url, req.body.season))
        .flatMap(downloadHtml)
        .map(getGorillaUrl)
        .flatMap(postToGorilla)
        .map(getVideoUrl)
        .flatMap(prepareHtml)
        .subscribe(function(data) {
            result += data;
        }, function(error) {
            res.send(JSON.stringify(error))
        }, function() {
            res.send(result);
        });
}


function fetchContent(params) {
    console.log(params.url);
    var args = [].slice.call(arguments, 1)
    return Rx.Observable.create(function(observer) {
        request(params, function(error, response, body) {
            if (error) {
                observer.onError();
            } else {
                observer.onNext({
                    response: response,
                    body: body,
                    args: args
                });
            }
            observer.onCompleted();
        })
    });
};

function prepareHtml(url) {
    return `<a href=${url}></a>`;
}

function fileName(url, season, episode) {
    return Rx.Observable.just(url.split('/'))
        .last().map(function(name) {
            return name.replace('_', ' ').concat(` S${season}E${episode}`);
        });
}

function getVideoUrl(data) {
    var pattern = /"http(.*)(\.flv|\.mkv|\.mp4)"/;
    var matches = pattern.exec(data.body);
    if (matches && matches[0]) {
        return matches[0]
    } else {
        return null;
    }
}

function postToGorilla(url) {
    var params = {
        url: url,
        method: 'POST',
        headers: {
            'content-type': 'application/x-www-form-urlencoded',
        },
        form: {
            op: 'download1',
            method_free: 'Free+Download',
            id: _.last(url.split('/')),
        }
    }
    return fetchContent(params)
}

function getGorillaUrl(data) {
    try {
        var $ = cheerio.load(data.body);
        var url = $('a[title="gorillavid.in"]').first().attr('href').split('=')[1]
        return url = new Buffer(url, 'base64').toString('ascii');
    } catch (error) {
        return Rx.Observable.just(null);
    }
}

function downloadHtml(url) {
    return fetchContent({
        url
    });
}

function formEpisodeUrl(url, season, episode) {
    return url.replace('/serie/', '/episode/').concat(`_s${season}_e${episode}.html`);
}

function getEpisodeRange(data) {
    var $ = cheerio.load(data.body);
    return _.range(1, ($('span').filter(function() {
        return $(this).text() === `Season ${data.args[0]}`;
    }).parents('h2').siblings('ul').children('li').length) + 1 || 0, 1);
}

\$\endgroup\$
3
+100
\$\begingroup\$

Overall, I think this is very decent code, some observations:

JSHint

  • Use JsHint ;)
  • You have a number of missing semicolons, and an unnecessary semicolon
  • return url = new Buffer(url, 'base64').toString('ascii'); <- I dont think you wanted to assign to buffer?
  • You are not using the function fileName(url, season, episode){ anywhere

Odd stuff

  • You use the extremely cool fetchContent({ url }); but also the mundane

    {
      response: response,
      body: body,
      args: args
    }
    

    You should stick to 1 approach

  • Your regex /"http(.*)(\.flv|\.mkv|\.mp4)"/
    • Is not supporting avi, wmv, or mpg file extensions.
    • Is not supporting uppercase incoming file names extentions.
    • Keeps repeating the \. for each extension, so I would keep that out of the brackets.
    • If you plan for many entries, then I would keep the regex outside of the function, and generate that regex only 1 single time for performance
  • You have a stray console.log(params.url);, get rid of it
  • It seemed odd that the HTML you prepare has no text in the anchor:

    return `<a href=${url}></a>`;
    

    Also, do you want to consider at least surrounding the URL with double quotes?

Sugar

  • You use req.body 3 times in getResult, I would have had a var params = req.body; as a shortcut

  • This seems a good use case for a ternary:

    if (matches && matches[0]) {
        return 
    } else {
        return null;
    }
    

    could be

    return (matches && matches[0]) ? matches[0] : null;     
    

    I am still not sure why you return null actually. PrepareHtml will return <a href=null></a>, so perhaps consider

    return (matches && matches[0]) ? matches[0] : '';     
    

Anonymous functions kill kittens every day

  • The 3 functions within your subscribe call are anonymous. It is in general terrible to look at a stack trace filled with anonymous functions
  • You also have one in getEpisodeRange
  • I do like that you declare your other functions 'the right way'

Comments go a long way

  • getEpisodeRange needs comments, it is the most intense code
  • There are actually no comments at all in this code, and I can parse everything just fine except the above mentioned function

Style

  • Formatting looks good to me
  • Naming looks good to me
  • I did not find obvious repetitions from a DRY perspective
\$\endgroup\$
  • \$\begingroup\$ Thanks very much for the reply. It is really helpful.. But Can you please review this from the reactive perspective as well. Because this is my first reactive program. I am not sure if this is the right way. \$\endgroup\$ – Sreevisakh Jul 12 '16 at 7:46
  • \$\begingroup\$ I need a name for the a link as well. That is why I created the fileName function. I can only call the fileName function from my getEpisodeRange function but i couldn't find a way to pass that to the generateHtml function in the map chain. \$\endgroup\$ – Sreevisakh Jul 12 '16 at 7:58
  • \$\begingroup\$ @Sreevisakh I think your usage is awesome, almost tutorial worthy. I will check the fileName thing and see if I can find a work-around. \$\endgroup\$ – konijn Jul 12 '16 at 23:14

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.