Conversation
…ster' into SammyIsra/master
Add a node if pubsubs are on sale that day (but only check once a day)
SammyIsra
left a comment
SammyIsra
left a comment
There was a problem hiding this comment.
One thing that I should be changed definitely is the "random" part. My guess is that it is a breaking issue because one will always try to get json[undefined] which in turn returns undefined.
From the other two, one is mostly an opinion and another one a thought for the future.
| msg.send( (JSON.parse(body).data.children)[Math.floor(Math.random()*(JSON.parse(body).data.children).length)].data.url); | ||
| var json = JSON.parse(body).data.children; | ||
|
|
||
| msg.send( json[random].data.url); |
There was a problem hiding this comment.
I don't think random exists here. You set it in a for loop in a different function (line 39), on a scope we don't have access to here.
|
|
||
|
|
||
| if robot.brain.get('pubsubCheckDate') isnt (new Date()).toDateString() | ||
| robot.brain.set('pubsubCheckDate', (new Date()).toDateString()) |
There was a problem hiding this comment.
This is more a matter of opinion, but wouldn't it be better to assign today's date to a variable instead of calling (new Date()).toDateString() twice? It's not a performance thing tho, more a readability thing.
|
|
||
|
|
||
|
|
||
| if robot.brain.get('pubsubCheckDate') isnt (new Date()).toDateString() |
There was a problem hiding this comment.
Since now we have two different places where we do the same thing (get pubsub stuff) it would probably be best to have a function somewhere else that is used both here and in scripts/pubsub.js.
Buuut we may want to hold off this specific change until we deal with #24
2 participants
No description provided.