1
\$\begingroup\$

I'm calling an API to request token, and making sure i'm not leaving any corners for errors and handle the result appropriately.

It's a web service POST call in general, The code gets credentials, formats them in the URL and parses the result.

The objective here is I'm looking if code overall is good when it comes to REST call, if any corner I'm missing for an error somewhere.

function Authentication() {
}

Authentication.prototype.requestToken = function(credentials) {
    var self = this;
    return new Promise(function(resolve, reject){
        https.request({
            method: 'POST',
            hostname: 'hostname.com',
            path: '/path-to-api' +
                '&client_id=' + credentials.clientId +
                '&client_secret=' + credentials.clientSecret +
                '&device_id=' + credentials.deviceId +
                '&device_token=' + credentials.deviceToken,
            headers: {
                'accept': 'application/json;charset=UTF-8',
                'content-type': 'application/x-www-form-urlencoded'
            }
        }, function(response) {
            var buffers = [];
            response.on('data', buffers.push.bind(buffers));
            response.on('end', function(){
                try {
                    self.token = JSON.parse(Buffer.concat(buffers).toString());
                    response.statusCode == 200 ? resolve(self.token) : reject(self.token);
                } catch (error) {
                    reject(error);
                }
            });
        }).on('error', reject).end();
    });
};

Authentication.prototype.token;
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please tell us more about what your code does and why you wrote it. Code lives in a context and a good review takes this context into account. \$\endgroup\$
    – Mast
    Feb 12 '19 at 20:54
  • \$\begingroup\$ @Mast I've added some details, hope it helps. The idea is to know if the http call is comprehensive covering all corners of errors and results. \$\endgroup\$
    – Developer
    Feb 12 '19 at 23:34
  • \$\begingroup\$ Much better already. \$\endgroup\$
    – Mast
    Feb 13 '19 at 14:44
1
\$\begingroup\$

A few minor quibbles:

  • You should always have a try-catch inside a new Promise, otherwise exceptions are swallowed.
  • Query parameters should be escaped
  • "new Promise" should be a rare sight. Never use it directly. Always wrap the thing you want to use with a promise interface (in this case, it's the https library)

And a major quibble:

  • This code is stuck in the past. You can simplify greatly if you embrace newer versions of javascript, and maybe add a couple of packages

For example:

const rp = require('request-promise')

class Authentication{
    constructor(){
        this.token = null
    }

    async requestToken(credentials){
        const token = await rp({
            method: 'POST',
            uri: 'https://hostname.com/path-to-api',
            json: true,
            qs: {
                client_id: credentials.clientId,
                client_secret: credentials.clientSecret,
                device_id: credentials.deviceId,
                device_token: credentials.deviceToken,
            },
            headers: {
                'accept': 'application/json;charset=UTF-8',
                'content-type': 'application/x-www-form-urlencoded'
            }
        })

        this.token = token
        return token
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ thanks but I don't see any error handlings, it's never a smooth ride, what if things goes wrong? my code was around all possibilites of errors, from json parsing, to http errors and returning it to the caller \$\endgroup\$
    – Developer
    Feb 16 '19 at 19:51
  • \$\begingroup\$ I didn't get you on You should always have a try-catch inside a new Promise, otherwise exceptions are swallowed. please explain more. I think my code try catch is inside. \$\endgroup\$
    – Developer
    Feb 16 '19 at 19:51
  • 1
    \$\begingroup\$ Your code didn't handle any errors, it just turned them into rejections. My version also does this. About try catch inside a new promise. Anything you don't reject yourself will be swallowed. For example, if credentials is null, you will get a swallowed exception. \$\endgroup\$ Feb 17 '19 at 6:28
  • 1
    \$\begingroup\$ in your version, I'll have a try catch outside or catch expression at the caller level of requestToken, appreciate if you can add that part just for a comprehensive deal. \$\endgroup\$
    – Developer
    Feb 17 '19 at 18:42

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.