I strongly suggest that you study callbacks and really try to understand how they work. Your code has several setTimeout
which 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).
what is the need for
setTimeout(function(){ ... }, 3000)
and then executeupdateData()
andreadData()
??– George Neto
So that before they were executed, they had already read the proper files. Because without setTimeout was giving Undefined.
– Paulo Sérgio Filho
In that case you should use the callback of
fs.readFile
to perform the next step only after the reading is finished. Use thesetTimeout
That’s a tough trick.– GBrandt
hahaha, it was right in the game, I’m very new at this.
– Paulo Sérgio Filho
start using Fs.readFileSync and Fs.writeFileSync to get rid of this set timeout
– Rodolfo Patane