michbeck100 / pimatic-hap

Pimatic homekit bridge
GNU General Public License v2.0
30 stars 10 forks source link

Some hints about the usage of promises #4

Closed sweetpi closed 8 years ago

sweetpi commented 8 years ago

Great work. I like the plugin very much. However, unfortunately I don't have a ios device for testing.

I did spot some errors in the code, I want to point out to improve it a little bit:

device.changeStateTo(value).then( callback() )

You are calling the callback, before the actions finished, because you are passing the result of the callback call to the then handler. You probably call the callback in the then handler:

device.changeStateTo(value).then( => callback() )

This parses a function, that calls the callback to the handler. Which is the right way.

Further you should handle errors:

device.changeStateTo(value)
  .then( => callback() )
  .catch( (error) => callback(error) )
  .done()

catch catches an error and passes it to the callback. done throws all unhandled errors.


...
        device.turnOff().then(
          device.turnOn().then(
            device.turnOff().then(
            ...

The same problem here. You parsing the result of device.turnOn() to the then handler. However you want to pass a function, that calls turnOn:

...
        device.turnOff().then( =>
          device.turnOn().then( =>
            device.turnOff().then( =>
            ...

You can even flatten the callback chain here:

      device.getState().then( (state) =>
       device.turnOff()
      ).then( =>
        device.turnOn()
      ).then( => 
        device.turnOff()
      ).then( =>
        device.turnOn()
      ).then( =>
        # recover initial state
        device.turnOff() if not state
      )
      .then( => callback() )
      .catch( (error) => callback(error) )
      .done()

As rule of thumb: Always make sure to pass a function => ... to a then handler. Further end each chain of promise class with a .catch( (error) => callback(error) ), if you want to pass the error to a callback and with .done() to not silently discard errors.

michbeck100 commented 8 years ago

Thanks for the lesson. I'm just starting with JS and node so please be patient. I'm still learning. I will use your example to refactor the plugin.

sweetpi commented 8 years ago

No worries. Promises are very hard to get, if you are new to the js development. Everyone struggles with them. If you have further questions or unsure with some statements, feel free to ask or ping me.

michbeck100 commented 8 years ago

I refactored the plugin according to your comments (85981349e88c466fe1c5a1dc9832a4e7982bfc30), please have a look into the changes.

sweetpi commented 8 years ago

Looks good now! Thanks again for creating this great plugin. I think this helps pimatic a lot to get more attraction.