Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been learning the basics of node over the last couple of days and finally hooked up what I needed. I'm just after someone to comment on if I have gone the right way around this task.

It creates a server which users access to get updates to Orders through server side events. The events are published through redis which I subscribe to the queue.

var redis = require('redis'),
    http = require('http'),
    os = require("os"),
    PHPUnserialize = require('php-unserialize');

function sendServerSendEventForInternetOrders(req, res) {
    req.socket.setTimeout(0); // let request last as long as possible
    var hostname;
    if(os.hostname() == "kipos"){
        hostname = os.hostname()+".dev";
    }else{
        hostname = os.hostname();
    }
    res.writeHead(200, {
        'Content-Type': 'text/event-stream',
        'Cache-Control': 'no-cache',
        'Connection': 'keep-alive',
        'Access-Control-Allow-Origin': 'http://'+hostname
    });

    res.write('\n');

    var phpSession = req.url.match('\/internetOrders\/(.*)')[1];

    console.log(phpSession + " - created client");
    var redisClient = redis.createClient();

    // In case we encounter an error...print it out to the console
    redisClient.on("error", function (err) {
        console.log(phpSession + " - Redis Error: " + err);
    });

    var phpSession = req.url.match('\/internetOrders\/(.*)')[1];

    var taID;
    var takeaway;

    redisClient.get("PHPREDIS_SESSION:" + phpSession, function (err, reply) {
        if (reply != null || err != null) {
            taID = PHPUnserialize.unserializeSession(reply).LoggedIn.TaID;
            console.log(phpSession + " - " + taID);
            startSub();
        } else {
            res.end();
            console.log(phpSession + " - exit");
        }
    });
    function startSub() {
        redisClient.select(1, function () {
            redisClient.get(taID + "T", function (err, reply) {
                if (reply == null) {
                    res.end();
                    console.log(phpSession + " - exit");
                    return;
                }
                takeaway = JSON.parse(reply);
                console.log(phpSession + " - "+takeaway.TaWebName);
            });

            redisClient.subscribe(taID + "IOU");

            // When we receive a message from the redis connection
            redisClient.on("message", function (channel, message) {
                sendMessage(message);
            });

            req.on("close", function () {
                redisClient.unsubscribe();
                redisClient.quit();
                console.log(phpSession + " - exit");
            });
        });
    }
    function sendMessage(data){
        res.write("data: " + data + '\n\n'); // Note the extra newline
        console.log(phpSession + " - data: " + data);
    }
};

http.createServer(function (req, res) {
    console.log(req.url);
    if (req.headers.accept && req.headers.accept == 'text/event-stream') {
        if (req.url.match('\/internetOrders\/(.*)')) {
            sendServerSendEventForInternetOrders(req, res);
        }
        else if (req.url == '/windowsApp') {
            res.writeHead(404);
            res.end();
        }
        else {
            res.writeHead(404);
            res.end();
        }
    } else {
        res.writeHead(200, {'Content-Type': 'text/plain'});
        res.end("Hello Kipos!"); // Kept this in for checking node server is up!
    }
}).listen(8080);
share|improve this question

1 Answer 1

One thing that you could change is the way that this if else statement is written

var hostname;
if(os.hostname() == "kipos"){
    hostname = os.hostname()+".dev";
}else{
    hostname = os.hostname();
}

Here you are deciding what a variables value should be using an if/else statement, this is a good spot to use a JavaScript ternary operator.

Example Ternary Statement:

var variableName = red == blue ? trueValueAssignment : falseValueAssignment;

so your code would look like this

var hostname = os.hostname() == "kipos" ? os.hostname() + ".dev" : os.hostname();
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.