search

zeyneloz / onesignal-node

A Node.js Library for OneSignal push notification service
MIT License
207 stars 48 forks source link

Add behavior tests for client and notification #4

Closed fecaps closed 6 years ago

fecaps commented 6 years ago

I have added behavior tests for client and notification.

zeyneloz commented 6 years ago

There is really no need for credentialLen = ALLOWED_CREDENTIALS.length part. There are also some problems with indentation in client.js. Thanks.

fecaps commented 6 years ago

The credentialLen part is used to don't calculate the array length at every time the function is called. As part of this PR I added .editorconfig, with this the indentation in client.js will likely be ok. And to complement it, I changed the file indentation type.

Anyway, thanks for the work you have done with this package.

zeyneloz commented 6 years ago

The length is not calculated at every iteration, every modern engine will realize that the array length is not changing inside the loop and will optimize it by caching. Even if the engines do not optimize the loop, it really doesn't worth to define a variable to cache the length for such a small array, it also reduces readability of the code. As soon as you remove credentialLen i will merge it. Thanks again for your work.

fecaps commented 6 years ago

I have checked and every modern engine do cache when it realizes array length hasn't changed. I updated the file with that. Thanks.