Function Code Review

General
1

Team,

I am new to Java script and Node Red, please can some review the code below and let me know if I made any serous mistakes or did not follow any standerds etc.

This flow will be used for a alarm control.
This function node is fed from a mqtt subscription, states of alarm zones etc are stored in context flow data.


let var_str_zone_reference = msg.topic.substring(msg.topic.lastIndexOf("/") + 1);

let var_arr_zone_references = var_str_zone_reference.split(",");

for (let var_str_zone_reference of var_arr_zone_references) {
    let var_flow_context_name;
    let var_flow_context_value;

    if (msg.payload == "arm" || msg.payload == "disarm"){
        var_flow_context_name = "config_alarm.alarm.zones['" + var_str_zone_reference + "'].state";
        var_flow_context_value = msg.payload + "ed";
        flow.set(var_flow_context_name, var_flow_context_value);

        var_flow_context_name = "config_alarm.alarm.zones['" + var_str_zone_reference + "'].state_at";
        var_flow_context_value = new Date();
        flow.set(var_flow_context_name, var_flow_context_value);
    }
}

if (msg.topic.indexOf("silent") != -1) {
    for (let var_str_zone_reference of var_arr_zone_references){
        const var_zone = flow.get("config_alarm.alarm.zones." + var_str_zone_reference);

        let var_arr_msg = [];
        var_arr_msg[0] = {}
        var_arr_msg[0].topic = "feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/{zone}";
        var_arr_msg[0].payload = "{state}";

        var_arr_msg[1] = {}
        var_arr_msg[1].topic = "feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}";
        var_arr_msg[1].payload = "{state}";

        var_arr_msg[2] = {}
        var_arr_msg[2].topic = "feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_status_group"
        var_arr_msg[2].payload = "alarm - zone - {zone} - {state}";

        for (let obj_msg of var_arr_msg){
            for (const [var_str_key, var_str_value] of Object.entries(var_zone)) {
                obj_msg.topic = obj_msg.topic.replace("{" + var_str_key + "}", var_str_value);
                obj_msg.payload = obj_msg.payload.replace("{" + var_str_key + "}", var_str_value);
            }
            node.send(obj_msg);
        }

    }

    node.done;
}

if (msg.topic.indexOf("normal") != -1) {

}
2

I couldn't get it to work as it did not like the Date() data, also there was no error checking. I ended up with this. (I think it gives the right result)

Edited: to take account of dynamicdave's comment. Note the comment about adding comments to your code. You will thank yourself in six months time when you need to revise the code.

let var_str_zone_reference = msg.topic.substring(msg.topic.lastIndexOf("/") + 1) ?? ''

let var_arr_zone_references = var_str_zone_reference.split(",") ?? []
let currentDate = Date.now()

if (var_str_zone_reference === '' || var_arr_zone_references.length === 0) {
    node.warn('Invalid Zone Data')

    return
    
}

for (let var_str_zone_reference of var_arr_zone_references) {
    let var_flow_context_zone = `config_alarm.alarm.zones.${var_str_zone_reference}`
    let var_flow_context_value = {}

    if (msg.payload == "arm" || msg.payload == "disarm") {
        var_flow_context_value = {state: msg.payload + "ed", 
                                  state_at: currentDate}

        flow.set(var_flow_context_zone, var_flow_context_value)

    }
}

if (msg.topic.indexOf("silent") != -1) {
    const var_zone_data = flow.get('config_alarm.alarm.zones') ?? ''

    if (var_zone_data === '') {
        node.warn ('No Zone Data')

        return

    }

    for (let var_str_zone_reference of var_arr_zone_references) {
        const var_zone = var_zone_data[var_str_zone_reference]
        const var_zone_state = var_zone.state


        let var_arr_msg = [];
        var_arr_msg[0] = {}
        var_arr_msg[0].topic = `feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/${var_str_zone_reference}`;
        var_arr_msg[0].payload = var_zone_state;

        var_arr_msg[1] = {}
        var_arr_msg[1].topic = `feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/${var_str_zone_reference}`;
        var_arr_msg[1].payload = var_zone_state;

        var_arr_msg[2] = {}
        var_arr_msg[2].topic = "feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_status_group"
        var_arr_msg[2].payload = `alarm - zone - ${var_str_zone_reference} - ${var_zone_state}`;

        for (let obj_msg of var_arr_msg) {
            node.send(obj_msg)

        }

    }

    node.done()
}

if (msg.topic.indexOf("normal") != -1) {

}

I used this to test it

[{"id":"252c652c3f74ac99","type":"function","z":"8428164b1645b4b8","name":"function 1","func":"let var_str_zone_reference = msg.topic.substring(msg.topic.lastIndexOf(\"/\") + 1) ?? ''\n\nlet var_arr_zone_references = var_str_zone_reference.split(\",\") ?? []\nlet currentDate = Date.now()\n\nif (var_str_zone_reference === '' || var_arr_zone_references.length === 0) {\n    node.warn('Invalid Zone Data')\n\n    return\n    \n}\n\nfor (let var_str_zone_reference of var_arr_zone_references) {\n    let var_flow_context_zone = `config_alarm.alarm.zones.${var_str_zone_reference}`\n    let var_flow_context_value = {}\n\n    if (msg.payload == \"arm\" || msg.payload == \"disarm\") {\n        var_flow_context_value = {state: msg.payload + \"ed\", \n                                  state_at: currentDate}\n\n        flow.set(var_flow_context_zone, var_flow_context_value)\n\n    }\n}\n\nif (msg.topic.indexOf(\"silent\") != -1) {\n    const var_zone_data = flow.get('config_alarm.alarm.zones') ?? ''\n\n    if (var_zone_data === '') {\n        node.warn ('No Zone Data')\n\n        return\n\n    }\n\n    for (let var_str_zone_reference of var_arr_zone_references) {\n        const var_zone = var_zone_data[var_str_zone_reference]\n        const var_zone_state = var_zone.state\n\n\n        let var_arr_msg = [];\n        var_arr_msg[0] = {}\n        var_arr_msg[0].topic = `feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/${var_str_zone_reference}`;\n        var_arr_msg[0].payload = var_zone_state;\n\n        var_arr_msg[1] = {}\n        var_arr_msg[1].topic = `feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/${var_str_zone_reference}`;\n        var_arr_msg[1].payload = var_zone_state;\n\n        var_arr_msg[2] = {}\n        var_arr_msg[2].topic = \"feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_status_group\"\n        var_arr_msg[2].payload = `alarm - zone - ${var_str_zone_reference} - ${var_zone_state}`;\n\n        for (let obj_msg of var_arr_msg) {\n            node.send(obj_msg)\n\n        }\n\n    }\n\n    node.done()\n}\n\n// Comment on what this does\nif (msg.topic.indexOf(\"normal\") != -1) {\n\n}","outputs":1,"timeout":0,"noerr":0,"initialize":"","finalize":"","libs":[],"x":440,"y":1040,"wires":[["cee2a6d8e4e09979"]]},{"id":"88a534fa9281a76e","type":"inject","z":"8428164b1645b4b8","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"silent/once,twice","payload":"arm","payloadType":"str","x":200,"y":1040,"wires":[["252c652c3f74ac99"]]},{"id":"cee2a6d8e4e09979","type":"debug","z":"8428164b1645b4b8","name":"debug 320","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"true","targetType":"full","statusVal":"","statusType":"auto","x":730,"y":1040,"wires":[]}]
1 Like
3

Looking at the last few lines of your code... I would change it to this...

    node.done();  // Call done function << added brackets
}

if (msg.topic.indexOf("normal") != -1) {
    // Add logic here if needed  << added a comment here to show extra code can be added
}
1 Like
4

Node-RED's idea is a low-code environment. This script looks quite convoluted, replacing text like it is a template is kind of weird to be honest.

Can you show an example of data that is received and what you expect to produce as output data ? and describe what the ultimate goal is ? I think this can be solved with standard nodes.

1 Like
5

So the idea behind this is to replace my current alarm system written in python.

The system has 3 main functions.
Arm / Disarm - One multiple or all zones
Status - One multiple or all zones
Trigger

Each action receives a mqtt message and the reaction would be a mqtt messages.

The system gets inputs from several devices, these are received in form of mqtt messages.
Most messages would look like this
Topic
"iincoming/alarm/zone/triggered/01" where all characters after the last /. would indicate the zone.

The output messages I wanted to easily be able to change hence the use of who context do do this.

The system also needed to keep track of various properties of a zone as it could potential have different reactions or output messages.

Since writing the original post I managed to get the system up and running.
Even though this works I don't know if any fundamental mistakes has been made.

Here is the new code

Arm Function Node

let var_obj_message_templates = flow.get("alarm_messages.alarm.arm");

let var_str_alarm_zones = msg.topic.substring(msg.topic.lastIndexOf("/") + 1);

if (var_str_alarm_zones == "00") {
    var_str_alarm_zones = Object.keys(flow.get("alarm_zones.alarm.zones")).toString();
}

let var_arr_alarm_zones = var_str_alarm_zones.split(",");

for (let var_str_alarm_zone of var_arr_alarm_zones) {
    let var_obj_alarm_zone = flow.get("alarm_zones.alarm.zones." + var_str_alarm_zone);

    if (msg.payload == "arm" || msg.payload == "disarm") {
        var_obj_alarm_zone.state = msg.payload + "ed";
        var_obj_alarm_zone.state_at = "x x x x x";
    }
}

if (msg.topic.indexOf("silent") != -1) {
     var_obj_message_templates = flow.get("alarm_messages.alarm.arm.silent");
} else {
     var_obj_message_templates = flow.get("alarm_messages.alarm.arm.normal");;
}

for (let var_str_alarm_zone of var_arr_alarm_zones) {
    let var_obj_alarm_zone = flow.get("alarm_zones.alarm.zones." + var_str_alarm_zone);

    for (let var_obj_message_template of Object.values(var_obj_message_templates)) {
        let var_obj_message = Object.assign({}, var_obj_message_template);
        for (let [var_str_key, var_str_value] of Object.entries(var_obj_alarm_zone)) {
            var_obj_message.topic = var_obj_message.topic.replace("{" + var_str_key + "}", var_str_value);
            var_obj_message.payload = var_obj_message.payload.replace("{" + var_str_key + "}", var_str_value);
        }
        node.send(var_obj_message)
    }
}
node.done;

Trigger Function Node

let var_obj_message_templates = flow.get("alarm_messages.alarm.triggered");

let var_str_alarm_zone = msg.topic.substring(msg.topic.lastIndexOf("/") + 1);

let var_obj_alarm_zone = flow.get("alarm_zones.alarm.zones." + var_str_alarm_zone);;

if (var_obj_alarm_zone.state == "armed") {
    var_obj_alarm_zone.triggered = "triggered";
} else {
    var_obj_alarm_zone.triggered = "dtriggered";
}

var_obj_alarm_zone.triggered_at = "x x x x x";

if (var_obj_alarm_zone.state == "armed") {
    var_obj_message_templates = flow.get("alarm_messages.alarm.triggered.armed");
} else {
    var_obj_message_templates = flow.get("alarm_messages.alarm.triggered.disarmed");;
}

for (let var_obj_message_template of Object.values(var_obj_message_templates)) {
    let var_obj_message = Object.assign({}, var_obj_message_template);

    if (var_obj_message.payload.indexOf("buz,{zone},0.1") != -1) {
        if (flow.get("alarm_config.alarm.beep_zones.beep_zones") == "off") {
            var_obj_message.payload = var_obj_message.payload.replace("{zone}", "1");
        }
    }

    for (let [var_str_key, var_str_value] of Object.entries(var_obj_alarm_zone)) {
        var_obj_message.topic = var_obj_message.topic.replace("{" + var_str_key + "}", var_str_value);
        var_obj_message.payload = var_obj_message.payload.replace("{" + var_str_key + "}", var_str_value);
    }
    node.send(var_obj_message)
}
node.done;

State Function Node

let var_obj_message_templates = flow.get("alarm_messages.alarm.state");

let var_str_alarm_zones = msg.topic.substring(msg.topic.lastIndexOf("/") + 1);

if (var_str_alarm_zones == "00") {
    var_str_alarm_zones = Object.keys(flow.get("alarm_zones.alarm.zones")).toString();
}

let var_arr_alarm_zones = var_str_alarm_zones.split(",");

for (let var_str_alarm_zone of var_arr_alarm_zones) {
    let var_obj_alarm_zone = flow.get("alarm_zones.alarm.zones." + var_str_alarm_zone);

    for (let var_obj_message_template of Object.values(var_obj_message_templates)){
        let var_obj_message = Object.assign({}, var_obj_message_template);
        for (let [var_str_key, var_str_value] of Object.entries(var_obj_alarm_zone)){
            var_obj_message.topic = var_obj_message.topic.replace("{" + var_str_key + "}", var_str_value);
            var_obj_message.payload = var_obj_message.payload.replace("{" + var_str_key + "}", var_str_value);
        }
        node.send(var_obj_message)
    }
}

node.done;

And here is the flow context data

{"alarm":{"beep_zones":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/beep_zones","payload":"{beep_zones}","retain":true}},"arm":{"silent":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/{zone}","payload":"{state}","retain":true},"message_02":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}","payload":"{state}","retain":true},"message_03":{"topic":"feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_status_group","payload":"alarm - zone - {zone} - {state}","retain":false}},"normal":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/{zone}","payload":"{state}","retain":true},"message_02":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}","payload":"{state}","retain":true},"message_03":{"topic":"feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_status_group","payload":"alarm - zone - {zone} - {state}","retain":false},"message_04":{"topic":"feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_group","payload":"alarm - zone - {zone} - {state}","retain":""},"message_05":{"topic":"feeds/thing-x/process-clock-x/incoming/clock/message/scroll","payload":"{\"message\" : \"alarm zone - {zone} - {state}\", \"delay\" : \"0.1\", \"repeat\" : \"1\"}","retain":false}}},"state":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/state/{zone}","payload":"{state}","retain":true},"message_02":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}","payload":"{state}","retain":true}},"triggered":{"armed":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}","payload":"{triggered}","retain":true},"message_02":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/timestamp/{zone}","payload":"{triggered_at}","retain":true},"message_03":{"topic":"feeds/thing-x/process-telegram-x/incoming/wcx_io_alarm_group","payload":"alarm zone - {zone} - {triggered}","retain":false},"message_04":{"topic":"feeds/thing-x/process-clock-x/incoming/clock/message/scroll","payload":"{\"message\" : \"alarm zone - {zone} - {triggered}\", \"delay\" : \"0.1\", \"repeat\" : \"1\"}","retain":false},"message_05":{"topic":"feeds/thing-x/process-alarm-x/incoming/io/switch/09","payload":"buz,{zone},0.1","retain":false}},"disarmed":{"message_01":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/{zone}","payload":"{triggered}","retain":true},"message_02":{"topic":"feeds/thing-x/process-alarm-x/outgoing/alarm/zone/triggered/timestamp/{zone}","payload":"{triggered_at}","retain":true}}}}}

{"alarm":{"zones":{"10":{"zone":"10","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"00":{"zone":"00","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"01":{"zone":"01","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"02":{"zone":"02","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"03":{"zone":"03","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"04":{"zone":"04","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"05":{"zone":"05","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""},"07":{"zone":"07","state":"armed","state_at":"x x x x x","triggered":"","triggered_at":""}}}}
6

I would looove to use just plain nodes but honestly I dont know how this can be done.
Have a look at my post below it explains the whole process.

There are several output messages that I require to be template messages.

Thanks for your input

7 
Can you show an example of data that is received and what you expect to produce as output data

^^
It is very hard to reverse engineer what you are doing here.

Are you in control of the topics ?

incoming/alarm/zone/triggered/01
I would change this, to something like zone/1/x - x would become the alarm or state type and "triggered" would become a payload to begin with.

8

Each of these seems to be a relatively straightforward process but I can't make head or tail of your code, sorry.

For example, it looks like you are storing status etc as objects in flow context, yet when you come to manipulate them you generate arrays of keys to iterate through.
Are you then using the wrong data structures? I can't tell.

But what counts is how easy it is for you to tell what's going on.
Will you still be able to tell in a year's time why the code does this?
If you will, then all is fine :grinning: If you aren't sure: comment, comment comment!

for (let [var_str_key, var_str_value] of Object.entries(var_obj_alarm_zone)) {
            var_obj_message.topic = var_obj_message.topic.replace("{" + var_str_key + "}", var_str_value);
            var_obj_message.payload = var_obj_message.payload.replace("{" + var_str_key + "}", var_str_value);
        }
1 Like