Tips on streamlining this function

General
1

Hello all,
I have written a function node which works, but I think there must be a clever way of doing it, rather than individual IF/ELSE statements.

Here is the flow, simply mqtt json data going through the function node, being filtered and sent to appropriate Blynk pin:

[{"id":"764b06c6.78b878","type":"mqtt in","z":"6f68fb89.34bc24","name":"","topic":"BW-1.27_MQTT/message","qos":"1","datatype":"json","broker":"","x":390,"y":780,"wires":[["eb32bac.74dab48"]]},{"id":"eb32bac.74dab48","type":"function","z":"6f68fb89.34bc24","name":"Check incoming and send to appropriate Blynk pin","func":"//POWER\nif (msg.payload.power === true)\n{\nvar msgPower ={\n    payload: \"1\",\n    pin: \"71\" };\n}\nelse {\n    msgPower ={\n    payload: \"0\",\n    pin: \"71\" };\n}\n\n\n\n//FILTER\nif (msg.payload.filter === true)\n{\nvar msgFilter ={\n    payload: \"1\",\n    pin: \"74\" };\n}\nelse\n{\n    msgFilter ={\n    payload: \"0\",\n    pin: \"74\" };\n}\n\n\n\n//HEATING OR HEATER\nif ((msg.payload.heating || msg.payload.heater) === true)\n{\nvar msgHeat ={\n    payload: \"1\",\n    pin: \"75\" };\n}\nelse\n{\n    msgHeat ={\n    payload: \"0\",\n    pin: \"75\" };\n}\n\n\n//CELSIUS/FAHRENHEIT\nif (msg.payload.celsius === true)\n{\nvar msgCelsius ={\n    payload: \"1\",\n    pin: \"76\" };\n}\nelse\n{\n    msgCelsius ={\n    payload: \"0\",\n    pin: \"76\" };\n}\n\n\n//AIR\nif (msg.payload.air === true)\n{\nvar msgAir ={\n    payload: \"1\",\n    pin: \"77\" };\n}\nelse\n{\n    msgAir ={\n    payload: \"0\",\n    pin: \"77\" };\n}\n\n\n//LOCKED\nif (msg.payload.locked === true)\n{\nvar msgLocked ={\n    payload: \"1\",\n    pin: \"79\" };\n}\nelse\n{\n    msgLocked ={\n    payload: \"0\",\n    pin: \"79\" };\n}\n\n\n\nreturn [ [ msgPower, msgFilter, msgHeat, msgCelsius, msgAir, msgLocked ] ];\n","outputs":1,"noerr":0,"x":790,"y":780,"wires":[["e832b721.ad19a8"]]},{"id":"e832b721.ad19a8","type":"blynk-ws-out-write","z":"6f68fb89.34bc24","name":"Blynk (dynamic pin by code)","pin":"0","pinmode":"1","client":"","x":1170,"y":780,"wires":[]}]

image
image757Ă—101 6.66 KB

Here is the MQTT json payload, I am only need some of the topics as per the function node:
image

Here is the function node code, I feel there is some way to cut it down in a clever way:

//POWER
if (msg.payload.power === true)
{
var msgPower ={
    payload: "1",
    pin: "71" };
}
else {
    msgPower ={
    payload: "0",
    pin: "71" };
}



//FILTER
if (msg.payload.filter === true)
{
var msgFilter ={
    payload: "1",
    pin: "74" };
}
else
{
    msgFilter ={
    payload: "0",
    pin: "74" };
}



//HEATING OR HEATER
if ((msg.payload.heating || msg.payload.heater) === true)
{
var msgHeat ={
    payload: "1",
    pin: "75" };
}
else
{
    msgHeat ={
    payload: "0",
    pin: "75" };
}


//CELSIUS/FAHRENHEIT
if (msg.payload.celsius === true)
{
var msgCelsius ={
    payload: "1",
    pin: "76" };
}
else
{
    msgCelsius ={
    payload: "0",
    pin: "76" };
}


//AIR
if (msg.payload.air === true)
{
var msgAir ={
    payload: "1",
    pin: "77" };
}
else
{
    msgAir ={
    payload: "0",
    pin: "77" };
}


//LOCKED
if (msg.payload.locked === true)
{
var msgLocked ={
    payload: "1",
    pin: "79" };
}
else
{
    msgLocked ={
    payload: "0",
    pin: "79" };
}



return [ [ msgPower, msgFilter, msgHeat, msgCelsius, msgAir, msgLocked ] ];

Any information is appreciated!

2

There is an error here, if you hit the else, you haven't defined the variable. Define the variable before the if let msgPower = {} or const msgPower = {} though then you will need to set the entries individually like msgPower.payload = 0. In your case, if you only have the two options, this would be better:

let msgPower = {
    payload: "0",
    pin: "71" };
}

if (msg.payload.power === true) {
    msgPower = {
        payload: "1",
        pin: "71" 
    };
}

Also, do you really want to set the properties to strings when they actually contain numbers? If not, lose the quotes.

Also, as a matter of style - that will help you avoid unnecessary errors, it is good to be really consistent with where you place brackets and how you use spacing. Notice the differences between my code and yours. Once you build up muscle memory, you will find yourself making fewer mistakes. It also makes code a LOT easier to read and you should remember that you read code far more than write it.

Similarly issues with the other if/else statements.

I would also put the pin numbers into a separate object variable with logical names. That way, you are less likely to make mistakes when setting the pin number.

const pins = {
    power: 71,
    filter: 74,
    heat: 75,
    celcius: 76,
    air: 77,
    locked: 79,
}

...

let msgLocked = {
    payload: 0,
    pin: pins.locked,
}
1 Like
3

You can use trinary operator to slim it down...

E.g...

Becomes...

var msgPower = {
    payload: msg.payload.power === true ? 1 : 0,
    pin: 71 
};

Apart from that...

  • Why are you specifying strings instead of numbers?
  • Don't enter var inside an if then use it out side. It works only because of hoisting (let and const would fail)
2 Likes
4

Indeed, I would use:

const msgPower = {
    payload: msg.payload.power === true ? 1  :  0,
    pin: pins.power,
}

Using const and let rather than var wherever possible saves on annoying bugs.

Note as well that in all my code, I do not use trailing ; (they are rarely actually needed and just create visual noise that can make things harder to read). Also, I always include trailing comma's since it means a lot less work and fewer errors when you want to rearrange lists (though maybe that's just my OCD!). However, these are style things - again, the important thing is to be consistent. Not only does that save you masses of thinking effort as you are reading your code, it means fewer errors and where you do have errors they can be easier to spot.

1 Like
5

Wow I knew it could be improved, but did not realize how much so! It might show that I am new to Arduino code, and even newer to Javascript.

Thank you both for the tips, it will take some time to absorb, and I may ask follow up questions if that's ok. I don' t know where to start! I like the trinary operator, very nice.

PS - yes they should be numbers, not strings - oops.

6

No problem, we all have to start at the beginning.

And we are suckers for giving people our opinions on things :sunglasses:

1 Like
7

Thank you both very much!

I've gone through it now and it makes sense much more. And it's working of course!

I notice you put a comma after "pins.power", I thought that the end of a list had to have no comma? As least when I am editing json config files I thought I had errors in the past due to that.

But it works and I'm sure you know what you're doing :smile:

8

JSON is not JavaScript. The naming is confusing. JSON is based on a simplified and much stricter version of JavaScript object notation. There are lots of strict rules for JSON but JavaScript objects are much more flexible.

So you can't have trailing comma's in JSON but you certainly can in JavaScript. It is one of those little things that makes coding just a tiny bit less stressful. I hate writing a list and then having to reorder it and finding that I forgot to change over the comma from the last entry. So I just always include one - sorted!

9

Ah I get it now, now I see why you always add the comma!
Thanks again, much appreciated!

10

You could also use destructuring to read the msg.payload properties to avoid repeating it for each distinct msg you create. Also in this case it seems you're dealing with boolean type values for the conditions, so I would maybe leave the explicit === true check off. E.g:

const {power, filter, heating, heater, celsius, air, locked} = msg.payload;
const pins = {
   // ...
};

const msgPower = {
  payload: power ? 1 : 0,
  pin: pins.power
};

// alternatively as the code is now shorter:
const msgPower = {payload: power ? 1 : 0, pin: pins.power};

// etc.

As an example to make the point of matter of taste on some things, I've used two spaces indentation (I prefer the code less wide), semicolons (they give me a clear indication of end of an expression as I've used to seeing them) and I haven't used the "dangling comma" (as I think it looks messy to my OCD).

1 Like
11

Thank you, I’ll look at this when I’m back in front of my PC :+1:

12

@ristomatti I've just been looking at your code and it does indeed work without the === true .

I'm wondering when that check is actually needed (I'm guessing to compare to a value), and how it would be written?

const {power, filter, heating, heater, celsius, air, locked} = msg.payload;
const pins = {power: 71, filter: 74, heat: 75, celsius: 76, air: 77, locked: 79};


const msgPower = {
    payload: power ? 1  :  0,
    pin: pins.power
}

const msgFilter = {
    payload: filter ? 1  :  0,
    pin: pins.filter
}

const msgHeat = {
    payload: (heating || heater) ? 1  :  0,
    pin: pins.heat
}

const msgCelsius = {
    payload: celsius ? 1  :  0,
    pin: pins.celsius
}

const msgAir = {
    payload: air ? 1  :  0,
    pin: pins.air
}


const msgLocked = {
    payload: locked ? 1  :  0,
    pin: pins.locked
}


return [ [ msgPower, msgFilter, msgHeat, msgCelsius, msgAir, msgLocked ] ];

It's certainly more streamlined, which is what I asked for! But for a beginner like me maybe too much streamlining makes it harder to understand :smile:

2 Likes
13

Exactly. Readability is an important part of understanding the code later and making it easier to maintain. But just for the heck of it, here's a yet more streamlined version with as little repetition I can come up now:

// nested destructuring of msg
const {payload: {heater, heating}} = msg;
msg.payload.heat = heater || heating;

const pins = {power: 71, filter: 74, heat: 75, celsius: 76, air: 77, locked: 79};

const msgs = Object.keys(pins).map(pin => {
  const payload = msg.payload[pin] ? 1 : 0;
  return {payload, pin}; // shortened version
});

// msgs should now contain an array of messages in the order of pins
return [msgs];

I didn't test this and wrote it on my tablet but I think it should do the job. Not very easy to read for a beginner anymore though. :laughing:

1 Like
14

Somehow that works as well, I am officially tapping out now :upside_down_face: :smile:
Thanks :+1:

1 Like
15

No room for 1 more?

msg.payload.heat = msg.payload.heater || msg.payload.heating;
return [ [["power",71], ["filter", 74], ["heat",75], ["celsius",76], ["air",77], ["locked",79]].map(e => { 
    return {payload: msg.payload[e[0]] ? 1 : 0, pin: e[1] };
}) ];
2 Likes
16

Now that is difficult to comprehend! :rofl:

1 Like
17

Just a small adjustment:

return {payload: Number(msg.payload[e[0]]), pin: e[1]};
2 Likes
18

But streamlined :wink:

1 Like
19

Ok this is getting silly now :joy:

20

My advice is to ALWAYS use xxxx === true.

Because JavaScript is notorious at making things "truthy" or "falsy" without actually being a boolean true/false.

This creates really hard to track down bugs. That is certainly a case of over-streamlining.