How to optimize a Node.js script

Question

Hello, I would like the help of users to optimize a script I made in Node.js. I do not understand much of the language and I made a very amateur system that is a little slow.

var express = require('express')();
var http = require('http').Server(express);
var request = require('request');

var io = require('socket.io')(http);
var fs = require('fs');
var xml = require('xml2js');

var parser = new xml.Parser();

express.get('/', function(req, res){
    res.send('Dedicated server of Turn Network running!');
});

var tMusic;
var aTurn = {
    url: {
        history: '../../../Playlist/pgm/Montagem/MusicHistory.xml',
        current: '../../../Playlist/pgm/current.xml'
    },
    startServer: function(){
        aTurn.readData();

        setTimeout(function(){
            aTurn.updateData();
        }, 3000);
    },
    readData: function(){
        fs.readFile(aTurn.url.history, function(err, data){
            parser.parseString(data, function(err, result){
                tMusic = result['MusicHistory']['Item0'][0].$.Music;
            });
        });
        setTimeout(function(){
            aTurn.readData();
        }, 1000);
    },
    updateData: function(){
        fs.readFile(aTurn.url.history, function(err, data){
            parser.parseString(data, function(err, result){
                var current = result['MusicHistory']['Item0'][0].$.Music;
                var split = current.split(' - ');

                if(tMusic === current){
                }
                else if(tMusic == undefined || tMusic.length <= 0){
                    aTurn.emitData();
                }
                else{
                    aTurn.emitData();
                }
            });
        });
        setTimeout(function(){
            aTurn.updateData();
        }, 1000);
    },
    emitData: function(){
        function album(music){
            return new Promise(function(resolve, reject){
                request('http://source.localhost/provider/saveArt/'+music+'', function (error, response, result){
                    data = JSON.parse(result);
                    resolve(data['returnAlbum']);
                });
            });
        }
        fs.readFile(aTurn.url.current, function(err, data){
            parser.parseString(data, function(err, result){
                var music = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Artist'] + ' - ' + result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Title'];
                var music = music.toString();

                var startTime = result['Playlist']['OnAir'][0]['CurIns'][0]['StartedTime'];
                var startTime = startTime.toString().split(' ');

                var endTime = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0].$.SchedTime;
                var endTime = endTime.split(' ');

                var nextArtist = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Artist'];
                var nextMusic = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Title'];

                album(music).then(function(nextAlbum){
                    io.emit('updateNext', {
                        'nextArtist': nextArtist.toString(), 
                        'nextMusic': nextMusic.toString(),
                        'nextAlbum': nextAlbum
                    });
                });
                io.emit('updateTime', {
                    'startTime': startTime[1], 
                    'endTime': endTime[1],
                    'maxTime': ''
                });
            });
        });
    }
}

io.on('connection', function(socket){
    aTurn.emitData();
});

http.listen(3000, function(){
    console.log('Client server running on port 3000!');
});

aTurn.startServer();

It is a system that sends a communication in socket whenever tMusic change.

Could someone help me? I would be extremely grateful.

Answer 1

I strongly suggest that you study callbacks and really try to understand how they work. Your code has several setTimeoutwhich should not exist, and their logic for continually reading and updating things can be monstrously simplified by using them.

I fixed the code with that in mind, it went like this:

var express = require('express')();
var http = require('http').Server(express);
var request = require('request');

var io = require('socket.io')(http);
var fs = require('fs');
var xml = require('xml2js');

var parser = new xml.Parser();

express.get('/', function(req, res){
    res.send('Dedicated server of Turn Network running!');
});

var tMusic;
var aTurn = {
    url: {
        history: '../../../Playlist/pgm/Montagem/MusicHistory.xml',
        current: '../../../Playlist/pgm/current.xml'
    },
    startServer: function(){
        aTurn.watchData(function(result) {
            aTurn.updateData(result);
        });
    },
    watchData: function(callback) {
        fs.watchFile(aTurn.url.history, function () {
            fs.readFile(aTurn.url.history, function(err, data){
                parser.parseString(data, function(err, result){
                    tMusic = result['MusicHistory']['Item0'][0].$.Music;

                    if (callback)
                        callback(result);
                });
            });
        });
    },
    updateData: function() {
        aTurn.emitData();
    },
    emitData: function(){
        function album(music){
            return new Promise(function(resolve, reject){
                request('http://source.localhost/provider/saveArt/' + music, function (error, response, result){
                    var data = JSON.parse(result);
                    resolve(data['returnAlbum']);
                });
            });
        }

        fs.readFile(aTurn.url.current, function(err, data){
            parser.parseString(data, function(err, result){
                var music = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Artist'] + ' - ' + result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Title'];

                var startTime = result['Playlist']['OnAir'][0]['CurIns'][0]['StartedTime'];
                var startTime = startTime.toString().split(' ');

                var endTime = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0].$.SchedTime;
                var endTime = endTime.split(' ');

                var nextArtist = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Artist'];
                var nextMusic = result['Playlist']['Next'][0]['NextMusic'][0]['Music'][0]['Title'];

                album(music).then(function(nextAlbum){
                    io.emit('updateNext', {
                        'nextArtist': nextArtist.toString(), 
                        'nextMusic': nextMusic.toString(),
                        'nextAlbum': nextAlbum
                    });
                });

                io.emit('updateTime', {
                    'startTime': startTime[1], 
                    'endTime': endTime[1],
                    'maxTime': ''
                });
            });
        });
    }
}

io.on('connection', function(socket) {
    aTurn.emitData();
});

http.listen(3000, function(){
    console.log('Client server running on port 3000!');
});

aTurn.startServer();

Clearly, as I do not know the format of your files or the expected result of the code, I have no way to test that really works pro you need, but in terms of logic so it gets better structured.

---

Corrections made

The problems were basically all around the functions readData and updateData. The first:

1. setTimeout with the aim of "doing one, then doing the other"

aTurn.readData();

setTimeout(function(){
    aTurn.updateData();
}, 3000);

This is not done.

• First problem: It is not stable. If ever, by chance, your readData it takes longer than 3 seconds to execute, the updateData will run without it having completed.
• Second problem: For basically every case where the timeout actually works, you waste time waiting 3 seconds, when in fact the readData may have finished in 5ms.

Solution

Add a callback to the function readData which is called when it ends. See that one question for more information about what callback is and where it is used.

Our refactored code stays:

aTurn.readData(function() {
    aTurn.updateData();
});

And in the readData:

readData: function(callback) {
    fs.readFile(aTurn.url.history, function(err, data){
        parser.parseString(data, function(err, result){
            tMusic = result['MusicHistory']['Item0'][0].$.Music;

            if (callback)
                callback();
        });
    });
    setTimeout(function(){
        aTurn.readData();
    }, 1000);
},

Note that callback is only useful if you want to run other things besides updateData shortly after the readData, and there is need to run different things for each call of the readData. Otherwise, you can just call the updateData of readData soon:

readData: function() {
    fs.readFile(aTurn.url.history, function(err, data){
        parser.parseString(data, function(err, result){
            tMusic = result['MusicHistory']['Item0'][0].$.Music;
            aTurn.updateData();
        });
    });
    setTimeout(function(){
        aTurn.readData();
    }, 1000);
},

2. setTimeout to read 1 in 1 second file

readData: function() {
    fs.readFile(aTurn.url.history, function(err, data){
        // ...
    });
    setTimeout(function(){
        aTurn.readData();
    }, 1000);
},

Solution

If I understood what you wanted, the idea of this is to read the file every x seconds and update things when it changes, correct?

In this case, you can use the function fs.watchFile:

readData: function(callback) {
    fs.watchFile(aTurn.url.history, function () {
        fs.readFile(aTurn.url.history, function(err, data){
            // ...
        });
    });
},

It is worth noting that the fs.watchFile may not be consistent across all platforms, as the documentation itself states. Carefully read the documentation for this function.

3. Function updateData

    updateData: function(){
        fs.readFile(aTurn.url.history, function(err, data){
            parser.parseString(data, function(err, result){
                var current = result['MusicHistory']['Item0'][0].$.Music;
                var split = current.split(' - ');

                if(tMusic === current){
                }
                else if(tMusic == undefined || tMusic.length <= 0){
                    aTurn.emitData();
                }
                else{
                    aTurn.emitData();
                }
            });
        });
        setTimeout(function(){
            aTurn.updateData();
        }, 1000);
    },

This entire function is redundant with the changes we’ve made to readData to call callback.

Note that the reading of the file may disappear, since we read the file in readData and we can pass this information forward via callback:

readData: function(callback) {
  // ...
  fs.readFile(aTurn.url.history, function(err, data) {
    parser.parseString(data, function(err, result){
        tMusic = result['MusicHistory']['Item0'][0].$.Music;

        if (callback)
            callback(result);
    });
  });
  // ...
}

So we use the data in the updateData, without having to read the file again (which is usually a very expensive operation, by the way):

    startServer: function(){
        aTurn.watchData(function(result) {
            aTurn.updateData(result);
        });
    },
    // ...
    updateData: function(result) {
        var current = result['MusicHistory']['Item0'][0].$.Music;
        var split = current.split(' - ');

        if(tMusic === current){
        }
        else if(tMusic == undefined || tMusic.length <= 0){
            aTurn.emitData();
        }
        else {
            aTurn.emitData();
        }
    }

Now we have one more problem: this if that makes no sense.

see that if (...) {}, without code between the {}, can always be reduced to a simpler code: ``. Yeah, nothing. Add with it.

Added to that, our tMusic is always updated just before the updateData, soon we would never have current != tMusic.

We have:

if (tMusic == undefined || tMusic.length <= 0) {
    aTurn.emitData();
} else {
    aTurn.emitData();
}

Now look how cool, if tMusic == undefined || tMusic.length <= 0, then emitData. If not, emitData too! Why not?

Yeah, that’s not a if, is a aTurn.emitData(); with extra steps.

In the end, our function updateData turn that around:

    updateData: function() {
        aTurn.emitData();
    },

I mean, you don’t need it. Just call emitData straightforward.

Note: we no longer need setTimeout, because the function is called every time the readData is executed, and that function already repeats!

Remarks

• With refactoring, it is clear that the variable tMusic is not used and is useless.
• The function emitData always advances to the next song from the current one (which I imagine is in the archive current), and does not use information from readData and updateData. This leads to believe that, in fact, the functions readData and updateData are unnecessary (in fact, the updateData sure is, as we’ve seen).