search

ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.31k stars 206 forks source link

prevent recursion when errors are thrown in `send()` callback #123

Closed KATT closed 9 years ago

KATT commented 9 years ago

fixes #122

hypesystem commented 9 years ago

Perhaps you could actually extract the try-catch block to a function (like parseAndRespond) -- might make the code more understandable?

KATT commented 9 years ago

Better? Do you want me to squash it?

hypesystem commented 9 years ago

No need to squash! Great code :)

The only thing I can add is that personally I would place the extracted function below where it is used (to me that is a more natural reading order). It's up to you, though :) let me know.

KATT commented 9 years ago

I usually define my helper functions in top of the file / straight after the "class" constructor they belong to / break them out into utils if re-used. Can change it if you want to, but I prefer it like this :)

BTW, I noticed that you return all your errors as strings - wouldn't it be better to return them as proper Error objects?

hypesystem commented 9 years ago

It probably would be better to return them as error objects, but that's a separate issue. Currently some of the error-strings er significant (ie. depended on by clients), so changing it would require a major version bump. I'm trying to gather things like that up for v1 (see https://github.com/ToothlessGear/node-gcm/milestones/v1).

hypesystem commented 9 years ago

I forgot to get you to add yourself to contributors, so I did it. Hope that's okay: https://github.com/ToothlessGear/node-gcm/commit/81b9748526464b51f41fefa1bea577c2d9380419