Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Some hints about the usage of promises #4

Closed
sweetpi opened this issue Nov 9, 2015 · 4 comments
Closed

Some hints about the usage of promises #4

sweetpi opened this issue Nov 9, 2015 · 4 comments

Comments

@sweetpi
Copy link
Contributor

sweetpi commented Nov 9, 2015

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
Copy link
Owner

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
Copy link
Contributor Author

sweetpi commented Nov 10, 2015

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 added a commit that referenced this issue Nov 10, 2015
- change events for dimlevel and switch state are reported back to iOS devices, now
@michbeck100
Copy link
Owner

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

@sweetpi
Copy link
Contributor Author

sweetpi commented Nov 12, 2015

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

@sweetpi sweetpi closed this as completed Nov 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants