2
\$\begingroup\$

I'm not sure if this is the right question but didn't come up with something else. I have this huge for{} loop which prints out some HTML Tables with some infos from some devices.

This code looked totally wrong to me and it had bugs. I tried to make it look as idiomatic to JavaScript as possible (started learning JS 4 months ago) but I don't know if what I did is ok.

I know my code has several lines, but they are repeating in most cases.

Can anybody tell me if this code can be rewritten (or some parts of it) in a more proper way? (PS: the code is running as it is now, but I'm just trying to improve my JavaScript coding style)

for (controller in k) {
    controller = k[controller];
    try {
        newMessage['args']['info'][controller]['data']['dynamic']['temperature'] = newMessage['args']['info'][controller]['data']['dynamic']['temperature'] / 10;
        newMessage['args']['info'][controller]['data']['static']['di_ia_descr'] = newMessage['args']['info'][controller]['data']['static']['di_ia_descr'].split('*');

        if (!$('#controls_' + controller).length) {
            $('#controls').html($('#controls').html() + '<center id="controls_' + controller + '"></center>');
        }

        if ($('#' + controller + '_di').length) {
            $('#di_' + controller + '_head').html('');
            $('#di_' + controller + '_head').html('<th>' + newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][6] + '</th><th>' + newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][7] + '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][8] + '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][9] + '</th>');

            $('#di_' + controller + '_body').html('');
            var data = newMessage.args.info[controller].data.dynamic;
            var $target = $('#di_' + controller + '_body');

            if (controller === "lan_control_1") {
                if (data.phase1 === "up") color1 = '#D60B0B';
                else color1 = '#14B514';

                if (data.phase2 === "up") color2 = '#D60B0B';
                else color2 = '#14B514';

                if (data.phase3 === "up") color3 = '#D60B0B';
                else color3 = '#14B514';

                if (data.phase4 === "up") color4 = '#000000';
                else color4 = '#000000';
            } else {
                if (data.phase1 === "up") color1 = '#14B514';
                else color1 = '#D60B0B';

                if (data.phase2 === "up") color2 = '#14B514';
                else color2 = '#D60B0B';

                if (data.phase3 === "up") color3 = '#14B514';
                else color3 = '#D60B0B';

                if (data.phase4 === "up") color4 = '#000000';
                else color4 = '#000000';
            }

            var html = '<tr>' +
                '<td class="text-center"><font color="' + color1 + '">' + data.phase1 + '</font></td>' +
                '<td class="text-center"><font color="' + color2 + '">' + data.phase2 + '</font></td>' +
                '<td class="text-center"><font color="' + color3 + '">' + data.phase3 + '</font></td>' +
                '<td class="text-center"><font color="' + color4 + '">' + data.phase4 + '</font></td>' +
                '</tr>';

            $target.html(html);
        } else {
            $('#controls_' + controller).html($('#controls_' + controller).html() +
                '<span style="display: inline-table;"><table id="' +
                controller +
                '_di" class="table table-condensed table-bordered" width="100%"><thead><tr><th colspan="5"><center>Voltage (' +
                controller +
                ')</center></th></tr><tr id="di_' +
                controller +
                '_head"><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][6] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][7] +
                '</th><th>' + newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][8] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][9] +
                '</th></tr></thead><tbody id="di_' +
                controller +
                '_body"><tr><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['phase1'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['phase2'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['phase3'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['phase4'] +
                '</td></tr></tbody></table></span>');
        }

        if ($('#' + controller + '_do').length) {
            $('#do_' + controller + '_head').html('');
            $('#do_' + controller + '_head').html(newMessage['args']['info'][controller]['data']['static']['out0_descr']);

            console.log("here15" + newMessage['args']['info'][controller]['data']['static']['out0_descr']);
            console.log('\nPause\n');
            $('#do_' + controller + '_body').html('');

            var $target1 = $('#do_' + controller + '_body');

            if (controller === "lan_control_0") {
                if (['on', 'off'][newMessage['args']['info'][controller]['data']['dynamic']['digital_out_0']] === "off") color5 = '#14B514';
                else color5 = '#D60B0B';
            } else {
                if (['on', 'off'][newMessage['args']['info'][controller]['data']['dynamic']['digital_out_0']] === "off") color5 = '#000000';
                else color5 = '#000000';
            }
            console.log("here16" + newMessage['args']['info'][controller]['data']['dynamic']['digital_out_0']);
            console.log('\nPause\n');

            var html1 = '<tr>' +
                '<td class="text-center"><font color="' + color5 + '">' + ['on', 'off'][newMessage['args']['info'][controller]['data']['dynamic']['digital_out_0']] + '</font></td>' +
                '</tr>';
            $target1.html(html1);
        } else {
            $('#controls_' + controller).html($('#controls_' +
                    controller).html() +
                '<span style="display: inline-table;"><table id="' +
                controller +
                '_do" class="table table-condensed table-bordered" width="100%"><thead><tr><th colspan="2"><center id="do_' +
                controller +
                '_head">' +
                newMessage['args']['info'][controller]['data']['static']['out0_descr'] +
                ' (' +
                controller +
                ')</center></th></tr><tr><th>Value</th></tr></thead><tbody id="do_' +
                controller +
                '_body"><tr><td>' + ['on', 'off'][newMessage['args']['info'][controller]['data']['dynamic']['digital_out_0']] +
                '</td></tr></tbody></table></span>');
        }

        if ($('#' + controller + '_t').length) {
            $('#t_' + controller + '_head').html('');
            if (controller === "lan_control_0") {
                $('#t_' + controller + '_head').html('<th>Temp_Amb<sup>Lab Et.-1</sup></th><th>Humidity<sup>Lab Et.-1</sup> [ % ]</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][0] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][1] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][2] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][3] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][4] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][5] +
                    '</th>');
            } else {
                $('#t_' + controller + '_head').html('<th>Temp_Amb<sup>Lab Et.1</sup></th><th>Humidity<sup>Lab Et.1</sup> [ % ]</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][0] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][1] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][2] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][3] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][4] +
                    '</th><th>' +
                    newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][5] +
                    '</th>');
            }

            $('#t_' + controller + '_body').html('');
            $('#t_' + controller + '_body').html('<tr><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['temperature'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['humidity'] / 10 +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t0'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t1'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t2'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t3'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t4'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t5'] +
                '</td></tr>');
        } else {
            $('#controls_' + controller).html($('#controls_' + controller).html() +
                '<span style="display: inline-table;"><table id="' +
                controller +
                '_t" class="table table-condensed table-bordered" width="100%"><thead><tr><th colspan="8"><center>Temperature (' +
                controller +
                ')</center></th></tr><tr id="t_' +
                controller + '_head"><th>T</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][0] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][1] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][2] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][3] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][4] +
                '</th><th>' +
                newMessage['args']['info'][controller]['data']['static']['di_ia_descr'][5] +
                '</th></tr></thead><tbody id="t_' +
                controller +
                '_body"><tr><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['temperature'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t0'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t1'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t2'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t3'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t4'] +
                '</td><td class="text-center">' +
                newMessage['args']['info'][controller]['data']['dynamic']['t5'] +
                '</td></tr></tbody></table></span>');
        }

        var timestamp = new Date(0);
        timestamp.setUTCSeconds(parseInt(newMessage['args']['info'][controller]['data']['dynamic']['timestamp']));
        if ($('#' + controller + '_state').length) {
            $('#' + controller + '_state').html('');
            $('#' + controller + '_state').html('<p><a href="' + newMessage['args']['info'][controller]['url'] + '">' + controller + '</a>:: ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_days'] + ' days, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_hours'] + ' hours, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_minutes'] + ' minutes, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_seconds'] + ' seconds; ' + '</p>');
        } else {
            $('#controls_status').html($('#controls_status').html() +
                '<span id="' +
                controller +
                '_state"><p><a href="' +
                newMessage['args']['info'][controller]['url'] +
                '">' +
                controller +
                '</a>:: ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_days'] + ' days, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_hours'] + ' hours, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_minutes'] + ' minutes, ' +
                newMessage['args']['info'][controller]['data']['dynamic']['uptime_seconds'] + ' seconds; ' +
                timestamp + '</p></span>');
        }
    } catch (err) {
        console.log('[MAINWEBSOCKET] on message error: ' + err);
    }
}

One problem that I could have is that if I ever would like to modify those tables, I won't be able without rewriting everything again.

To make yourself an idea of how the HTML looks like, here is an image:

image description could be put here

\$\endgroup\$
  • 1
    \$\begingroup\$ As we all want to improve our code, it is better to write a title that says something about what your code does. I have made an edit to it, feel welcome to review it. I hope you get some good answers! \$\endgroup\$ – Phrancis Feb 1 '16 at 14:30
  • 1
    \$\begingroup\$ Got it ! Thanks @Phrancis. You're right, will keep that in mind. \$\endgroup\$ – Grajdeanu Alex. Feb 1 '16 at 14:31
  • \$\begingroup\$ I have rolled back Rev 5 to 4. Please read What to do when someone answers. \$\endgroup\$ – 200_success Feb 1 '16 at 17:46
1
\$\begingroup\$

The first thing you should do is use a reference to that long property chain newMessage['args']['info'][controller]['data']. You already do it in one instance:

var dynamicData = newMessage['args']['info'][controller]['data']['dynamic'];

Also use the dot notation instead of the bracket notation when you have a string (containing a JS identifier) as the property. The first line then becomes much more readable

dynamicData.temperature = dynamicData.temperature / 10;

You should avoid appending to .html(). This is terribly slow, as this requires the browser the build a string from the DOM and then turn the whole thing back into a DOM structure. When adding a new element use .append() instead.

Also don't repeatedly get the same elements with jQuery. Store a reference and use that instead.

if (!$('#controls_' + controller).length) {
   $('#controls').html($('#controls').html() + '<center id="controls_' + controller + '"></center>');
}

becomes

var $controllerControls = $('#controls_' + controller);
if ($controllerControls.length === 0) {
    $controllerControls = $('<center id="controls_' + controller + '"></center>');
    $('#controls').append($controllerControls);
}
// From here on use $controllerControls instead of $('#controls_' + controller)

BTW, center is deprecated. Use a div (or preferably another suitable element) and use CSS to center it.


Instead of hard coding the colors in your code consider defining them in your style sheet and setting classes in your JS. This also gets rid of the font elements which are also deprecated.

Also, use loops where possible and avoid string concatenations, since they are also slow.

For example:

 var html = '<tr>' +
     '<td class="text-center"><font color="' + color1 + '">' + data.phase1 + '</font></td>' +
     '<td class="text-center"><font color="' + color2 + '">' + data.phase2 + '</font></td>' +
     '<td class="text-center"><font color="' + color3 + '">' + data.phase3 + '</font></td>' +
     '<td class="text-center"><font color="' + color4 + '">' + data.phase4 + '</font></td>' +
     '</tr>';
$target.html(html);

becomes something like:

var html = ["<tr>"];

for (var i = 1; i <= 4; i++) {
    html.push('<td class="text-center ', data["phase" + i], i === 4 ? " black" : "",  '">', data.phase1, '</td>');                
}

html.push("</tr>")

$target.html(html.join(''));        

With the CSS:

.up {
    color: #14B514;
}
.dn {
    color: #D60B0B
}
.up.black, .dn.black {
    color: black !important;
}
#di_lan_control_1_body .up {
    color: #D60B0B;
}
#di_lan_control_1_body .dn {
    color: #14B514;
}

(This isn't an exact copy. It strong depends things I don't know about. For example, black is a bad choice for the class name, it should be a word describing why phase4 is different from the others.)


<span style="display: inline-table;"><table>...

This doesn't make much sense. For one, you aren't allowed to put a table inside a span. If you need an inline table, put display: inline-table directly on the table itself - best would be in the stylesheet.


Generally: consider using a templating library to generate the HTML. It always best to keep the JS separate from the HTML.


(That's all I have time for right now.)

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.