Initial Commit for Wake On Lan #4

tmrobert8 · 2017-06-05T12:44:20Z

Adding support for wake on lan magic packets

neophob

Thanks @tmrobert8 for this preview.

I did a review and gave you feedback about some minor issues.

neophob · 2017-06-06T11:27:24Z

device/WakeOnLan/controller.js

+const PORT_NBR = 9;
+const DEFAULT_IPADDRESS = '255.255.255.255';
+
+let wolMap = "wol.map";


two comments here

use const instead let (you wont change this name)

why don't you use a JSON file to parse (much easier to parse) and/or support passing parameters via command line or env variable

Although I'd prefer JSON myself, I've dealt with enough users to know that non-programmers hate json/xml formats and to keep setup/config type files as simple as possible (like a simple x=y type of lines). Since a majority will try this stuff without knowing how to program - I chose that format for ease. If you feel strongly that it should be json - I'll switch to json (pretty easy)

Also - let is appropriate here since I can potentially change the name in the setMapPath function (although that reminds me I need to change the name of the variable since it's a path to the map file)

neophob · 2017-06-06T11:28:13Z

device/WakeOnLan/controller.js

+ */
+module.exports.onButtonPressed = function (deviceid, name) {
+    console.log(`WOL button pressed for ${name}`);
+    var addr = parseAddr(name);


try to use const/let instead var. this makes code easier to read

neophob · 2017-06-06T11:29:45Z

device/WakeOnLan/wol.map

+# Please note that if the MACAddress appears twice (or MAC Address/IP Address combo),
+# the NEEO app will complain of a duplicate when trying to add a device.
+#####################################################################################
+


json example

{
"name": "devicename",
"mac": "macaddress",
"ip": "optional ip address"
}

neophob · 2017-06-06T11:30:40Z

device/WakeOnLan/controller.js

+            macAddress: addr.substring(0, idx),
+            ipAddress: addr.substring(idx + 1),
+        }
+    } else {


obsolete else {} - as you already returned

??? not sure what you mean by this ???

your code:

if (something()) { return 'a'; } else { return 'b'; }

Easier to read version:

if (something()) { return 'a'; } return 'b';

neophob · 2017-06-06T11:31:40Z

device/WakeOnLan/controller.js

+
+    var max = lines.length;
+    for (var i = 0; i < max; i++) {
+        var line = lines[i].endsWith('\r') ? lines[i].slice(0, -1) : lines[i];


json would render this code obsolete, as you could access the attributes directly

neophob · 2017-06-06T11:32:40Z

device/WakeOnLan/controller.js

+const net = require('net');
+const dgram = require('dgram');
+
+const PORT_NBR = 9;


dont use abbreviation in const names

tmrobert8 · 2017-06-06T19:24:37Z

Just pushed a new version. Made most the changes (except the json one) and added a few more (mainly implemented Promises now that I'm more familiar with them).

neophob · 2017-06-06T21:04:44Z

device/WakeOnLan/controller.js

+    console.log(`WOL button pressed for ${name}`);
+
+    parseAddr(name)
+        .then(function (addr) {


hint use the es6 arrow functions (shorter to write and this is attached to the function scope), instead .then(function (addr) { you can use .then((addr) => {

Nice - about time they implemented something like that (I haven't even looked at the ES6 spec in detail - so hints like this are great!)

neophob · 2017-06-06T21:06:46Z

device/WakeOnLan/controller.js

+ * @param {any} addr the address field to parse
+ */
+function parseAddr(addr) {
+    return new BluePromise(function (resolve, reject) {


you don't need to wrap all functions in promise

Yeah - just exploring using promises (haven't used them before - too old school still!)

tmrobert8 · 2017-06-07T12:48:19Z

Just committed with arrow functions - I'll be changing my other PR for this as well

neophob · 2017-06-12T09:46:21Z

device/WakeOnLan/controller.js

+ * @param {any} macAddress the max address
+ */
+function getMacBuffer(macAddress) {
+    return new BluePromise((resolve, reject) => {


no need to return a promise here, just return the plain result

neophob · 2017-06-12T09:49:32Z

device/WakeOnLan/controller.js

+            return Promise.all([addr, getMacBuffer(addr.macAddress)])
+        })
+        .then((results) => {
+            let magic = Buffer.alloc(6, 0xff);


why do you allocate a buffer with 6 bytes and the concatinate it 16 times. just init a buffer with the correct size.

and you can also create a buffer object from a string: Buffer.from(string, 'utf8');

neophob · 2017-06-12T09:50:34Z

device/WakeOnLan/controller.js

+            return sendPacket(results[0].ipAddress, results[1]);
+        })
+        .then((results) => {
+            console.log("WOL packet sent");


I would use the debug package to output debug information, see https://github.com/visionmedia/debug.
I would add this output before the return sendPacket... so you can omit the additional then branch

neophob · 2017-06-12T09:53:11Z

device/WakeOnLan/controller.js

+            if (err === null || err === undefined) {
+                resolve(true);
+            } else {
+                reject("Exception writing the magic packet: " + err);


reject should wrap the error message in a error object, like reject(new Error('TEXT')), in your case you could just do a reject(err).

to make it more readable you can

if (err) { reject(err); } else { resolve(); }

neophob · 2017-06-12T09:53:57Z

device/WakeOnLan/controller.js

+
+        readline.createInterface({ input: fs.createReadStream(wolMap) })
+            .on('line', (line) => {
+                if (!line.startsWith("#")) {


i would extract the text parsing in its own function, to make the code more readable

tmrobert8 force-pushed the NEEO-WOL branch 2 times, most recently from c990704 to 7a6dd60 Compare June 5, 2017 13:35

neophob requested changes Jun 6, 2017

View reviewed changes

tmrobert8 force-pushed the NEEO-WOL branch from 7a6dd60 to fec71f4 Compare June 6, 2017 19:23

neophob reviewed Jun 6, 2017

View reviewed changes

tmrobert8 force-pushed the NEEO-WOL branch from fec71f4 to 31191cb Compare June 7, 2017 12:47

tmrobert8 force-pushed the NEEO-WOL branch from 31191cb to bf9866f Compare June 7, 2017 12:51

neophob approved these changes Jun 12, 2017

View reviewed changes

neophob reviewed Jun 12, 2017

View reviewed changes

Initial Commit for Wake On Lan

dfbbeab

tmrobert8 force-pushed the NEEO-WOL branch from bf9866f to dfbbeab Compare July 28, 2017 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Commit for Wake On Lan #4

Initial Commit for Wake On Lan #4

tmrobert8 commented Jun 5, 2017

neophob left a comment

neophob Jun 6, 2017

tmrobert8 Jun 6, 2017

tmrobert8 Jun 6, 2017

neophob Jun 6, 2017

neophob Jun 6, 2017

neophob Jun 6, 2017

tmrobert8 Jun 6, 2017

neophob Jun 6, 2017

neophob Jun 6, 2017

neophob Jun 6, 2017

tmrobert8 commented Jun 6, 2017

neophob Jun 6, 2017

tmrobert8 Jun 6, 2017

neophob Jun 6, 2017

tmrobert8 Jun 6, 2017

tmrobert8 commented Jun 7, 2017

neophob Jun 12, 2017

neophob Jun 12, 2017

neophob Jun 12, 2017

neophob Jun 12, 2017

neophob Jun 12, 2017

Initial Commit for Wake On Lan #4

Are you sure you want to change the base?

Initial Commit for Wake On Lan #4

Conversation

tmrobert8 commented Jun 5, 2017

neophob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmrobert8 commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmrobert8 commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment