Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete refactor #14

Merged
merged 15 commits into from
Sep 9, 2017
Merged

Complete refactor #14

merged 15 commits into from
Sep 9, 2017

Conversation

AlexGustafsson
Copy link
Owner

  • Rewrote most of the code to comply with a stricter code standard as well as clean code concepts. Added xo as dev dependency to enforce these rules.
  • Removed dependency on net-ping and added ping to hopefully remove the need for sudo rights.
  • Improved the handling of config. It is now more robust and allows for greater customization such as disabling of logs all together.

Fixes #11, #10 as well as some other issues related to sudo rights permissions have has been closed due to workarounds (sudo homebridge).

Complete code refactor.
Moved to new pinging library to hopefully eliminate permission issues
(#8, #9).
Fixed #11.
lib/computer.js Outdated
const wol = require('wake_on_lan');
const Pinger = require('./pinger');

let Service;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you acn factor out the global variables as well.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite find a feasible solution to that. Since homebridge provides the api via a function parameter, I need to get Service etc. from that parameter. One solution would be to put the class declaration in a function that takes Service etc. as parameters and return the class. I am opposed to that since the class declaration should be in its own and not in a function. Do you have any suggestions?

lib/computer.js Outdated
mac: null
}, config);

this.log = (...args) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit redundant, can't you just assign log to this.log if this.config.logging === true?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is redundant and I do partly agree with you. I do however think that checking if logging is turned on or not before every call to log would introduce too much "noise" in the code.

Consider the following case:

With the current solution:

if(error)
  log(error)
else
  doStuff();

Without the solution:

if(error){
  if(shouldLog)
    log(error)
} else {
  doStuff();
}

I argue that the first is more readable and cleaner, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this.log = this.config.logging ? log : (...args) => {}; does the same thing, it short circuits calls to this.log if logging is disabled.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood your initial suggestion. Yes, that is a more appropriate solution.

lib/computer.js Outdated
log(...args);
};

this.online = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you an make this some kind of enumeration, and assign it to this.status, because the online, waking up and shutting down states should be mutually exclusive.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll look into it.

Update:
Great suggestion. Makes much more sense from every stand point. Makes it much easier to think of the plugin as a state machine and therefore helps further debugging.

Went from using several variables to declaring a single variable,
status. Added enum to handle states.

Minor fixes such as position of logging inside functions were not
consistent.
@AlexGustafsson
Copy link
Owner Author

It seems as if the move to ping from net-ping has introduced a problem where the status of the accessories can "flicker" - the pinger returns false negatives when pinging was unsuccessful. The probable cause is that ping simply pings once per call potentially making it a bit less accurate than net-ping.

How should this be handled?

My idea:
Store the last n (i.e. 5) pings in the Pinger class and only callback a change if all of the stored pings are the same value, otherwise return the latest value where all values were the same. If there are less than n pings (aka, just started pinging) the current value should be returned instead.

@AlexGustafsson
Copy link
Owner Author

@blubber Do you have the time to try the new version of homebridge-wol? I'd like to ensure that it's stable and ready for merging.

@AlexGustafsson AlexGustafsson merged commit 6e05929 into master Sep 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expects config values to exist, doesn't handle missing configuration values
2 participants