Cleanup jobs modules #85
Conversation
|
@FlorianSW, |
|
@FlorianSW I tried to check your PR but I have merge conflicts. |
|
Hi @marziman. It seems, that the branch is mergeable with the openhab/openhab-cloud@master, so probably you made some changes to your copy locally? |
|
@FlorianSW the branch works fine for me. |
|
@FlorianSW got it! |
| var openhab = openhabs[i]; | ||
|
|
||
| if (openhab.last_email_notification && openhab.last_email_notification > date3DaysAgo) { | ||
| continue; |
There was a problem hiding this comment.
@FlorianSW wow great! The code is much more readable and cleaner now!
Great job! I will deep more into this after merging your other PR.
| new Date() | ||
| ], | ||
| function (err, result) { | ||
| logger.info('openHAB-cloud: every5min statistics collection job finished'); |
There was a problem hiding this comment.
@FlorianSW Same here. Thanks for your effort.
Will test this asap on my instance.
jobs/every5minstat.js
Outdated
|
|
||
| // number of invocations of the finishSent() function to know, when all requests are finished to track multiple | ||
| // async actions | ||
| expectedFinishCount = 6; |
There was a problem hiding this comment.
So instead of tracking the responses like this, would using promises and Promise.all() be a more standard approach?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all
There was a problem hiding this comment.
So i posted this 2 days ago, but did not click the "start review" button, that's twice that new feature from github has bit me.
There was a problem hiding this comment.
Haha, I thought mongoose can't return a promise (that's why I implemented this tracking with which I'm not very happy myself at all). Now I checked this a bit more and it seems, exec returns a promise, so I'll take a look into this, thanks for the note!
There was a problem hiding this comment.
That is a way better solution, thanks again for pointing me to it! :)
FlorianSW
force-pushed
the
general/code-readability-jobs
branch
from
e9aad92
to
eb11531
Compare
The following changes was made to scripts in the jobs directory of this project: * Changed big if/else control structure for error management to if + early return This makes the code a bit easier to read as the indention level is kept as low as possible * removed commented code * removed unused imports/requires and unused declared variables * for a consistent use: changed all " to ' * merged multiple var definition blocks * moved variable declarations to the top of modules/functions * Changed usage of type-converting comparsion (equality, ==) to strict comparsion (strict equality, ===) where usage of type-converting comparsion doesn't seem to be explicitly needed. The same applies for non-equality comparsions (!= -> !==) In this commit, the every5minstat.js script is rewritten to fulfill the things above and make the code a lot smaller.
FlorianSW
force-pushed
the
general/code-readability-jobs
branch
from
eb11531
to
245b096
Compare
| promises.push(Invitation.count({used:false}, countCallback.bind('invitationUnusedCount')).exec()); | ||
| promises.push(UserDevice.count({}, countCallback.bind('userDeviceCount')).exec()); | ||
|
|
||
| Promise.all(promises).then(saveStats); |
There was a problem hiding this comment.
@FlorianSW LGTM, I just need to run it on my test instance tonight or tomorrow.
There was a problem hiding this comment.
no hurry, thanks for your review! :)
|
Thanks! |
The following changes was made to scripts in the jobs directory of this project:
return This makes the code a bit easier to read as the indention level is
kept as low as possible
equality, ===) where usage of type-converting comparsion doesn't seem to be explicitly
needed. The same applies for non-equality comparsions (!= -> !==)
In this commit, the every5minstat.js script is rewritten to fulfill the things above and
make the code a lot smaller.