2
\$\begingroup\$

A server sends JSON to my webpage via a socket and the webpage creates or updates a "card" which displays the information from the JSON data. It's a one-way/read-only system.

Flow is:

  1. Thing sends data to Server
  2. Server receives data and does some things/logic
  3. Server sends data via websocket to webpage
  4. User opens webpage and connects to server via websocket
  5. Webpage receives the message and converts the Json to an object
  6. Webpage now passes the data object to some functions to display the data

Here's the general flow for the data: a global array of elements is maintained with all the objects being displays if the object exists, it's updated with new data, if it doesn't, it's created and added to the array. The array it then sorted and then appened to the DOM.

Here's the javascript for step 6

// JavaScript source code
var properties = ["AlarmNumber", "ActiveProgram",
                  "prop1", "prop2", "prop3", "prop4"];
var currentWidget;
var widgets = [];
//this is the function called when data is received from socket.
function CreateBlock(widgetData)
{

    var widgetID = widgetData.MachineID.replace(/ /g,"_");
    var myWidget = document.getElementById('widget-' + widgetID);
    if (myWidget == null)
    {
        myWidget = CreateCard(widgetID);
        UpdateCard(myWidget,widgetData);
    }
    else        
    {
        UpdateCard(myWidget,widgetData);//no card to updatre becuase it doesn't exist yet 
    }
    widgets.push(myWidget);
    DrawWidgets();

}

function DrawWidgets()
{
    var parent = document.getElementById("Cards");
    var currentCard;
    widgets.sort(compare)

    for (i = 0; i<=widgets.length;i++)
    {
        currentCard = widgets[i];
        var oldWidget = document.getElementById(currentCard.id);
        if (oldWidget !=null)
        {
            oldWidget.remove();
            //document.removeChild(oldWidget);
        }
        parent.appendChild(currentCard);
    }
}

function compare(a,b) {
    var aValue = a.children.cardbody.children.title.innerText
    var bValue = b.children.cardbody.children.title.innerText;

    if (aValue < bValue)
        return -1;
    if (aValue > bValue)
        return 1;
    return 0;
}

//create the card
function CreateCard(cardID) //as object
{

    var parent 
    var newdiv
    var cardElement = document.createElement("div");
    cardElement.className = "card";
    cardElement.id = "widget-" + cardID; 
    cardElement.style = "height:500px;";
    parent=cardElement;

    newdiv = document.createElement("div");
    newdiv.className = "card-header";
    parent.appendChild(newdiv);

    newdiv = document.createElement("div");
    newdiv.className = "card-body";
    newdiv.id = "cardbody";
    parent.appendChild(newdiv);
    parent=newdiv;

    newdiv = document.createElement("div");
    newdiv.className = "card-title";
    newdiv.id = "title";
    newdiv.textContent = "title";
    parent.appendChild(newdiv);

    newdiv = document.createElement("div");
    newdiv.className = "card-sub-title";
    newdiv.id = "subtitle";
    newdiv.textContent = "subtitle";
    parent.appendChild(newdiv);

    newdiv = document.createElement("div");
    parent.appendChild(newdiv);

    return cardElement;

}


//Add a data element
function AddDataElement(myWidget, title, value, showTitle = true)
    {
        var cardElement = myWidget; 
        var cardElementBody = cardElement.children.cardbody; 
        var dataElement = cardElementBody.children[title];
        if (dataElement == null)
        {
            dataElement = document.createElement("div");
            dataElement.id = title;
            dataElement.className = "card-item";
        }

        var output = showTitle == true ? title + ": " + value : value;
        dataElement.innerText = output;
        cardElementBody.appendChild(dataElement);
    }

    //update the card and set the formatting
    function UpdateCard(myWidget, widgetData) //myWidget is by reference
    {
        var card = myWidget;

        //some logic with inputs
        if (widgetData.AlarmNumber != 0) {
            card.style.backgroundColor = "red"; //".color-primary-0"; //how to apply CSS reference?
            card.className += " " + "blink-me";
            //card.style += ";background-color:.color-primary-0";
        }
        else if (widgetData.ExecutionMode !="Running"){
            card.className = "card";
            card.style.backgroundColor = "Orange";
        }
        else {
            card.style.backgroundColor = null;
            card.className = "card";
        }

        //now populate the data

        var currentDate = new Date();
        var day = currentDate.getDay();
        var month = currentDate.getMonth(); //Be careful! January is 0 not 1
        var year = currentDate.getFullYear();
        var hour = currentDate.getHours();
        var min = currentDate.getMinutes();
        var sec = currentDate.getSeconds();

        var dateString = year + "-" + ZeroPad(month + 1, 2) + "-" + ZeroPad(day,2) + " " + ZeroPad(hour, 2) + ":" + ZeroPad(min, 2) + ":" + ZeroPad(sec, 2);

        //"MachineID", "RunningMode",
        AddDataElement(card, "title", widgetData.MachineID,false);
        AddDataElement(card, "subtitle", widgetData.ExecutionMode,false);

        var data;
        for (let i = 0; i < properties.length; i++) {
            data = widgetData[properties[i]];
            AddDataElement(card, properties[i], data);
        }
        AddDataElement(card, "Timestamp", dateString);

    }

    function ZeroPad(num, places)
    {
        var zero = places - num.toString().length + 1;
        return Array(+(zero > 0 && zero)).join("0") + num;
    }

note: the webpage uses some CSS from grapeJS.

The code is working, but looking for some feedback regarding flow/process or any issues. Does this code follow some best practices?

\$\endgroup\$
2
\$\begingroup\$

JavaScript conventions.

Block delimiting

Javascript has the opening { on the same line as the statement

// Conventional JS
if (foo === bar) {

// Un-conventional JS
if (foo === bar) 
{

Capitalizing

Unlike C syntax like languages, JavaScript does not capitalize functions (methods).

The convention is functions start with lowercase

The function AddDataElement should be named addDataElement and the same for all the other function you have capitalized

Only objects that are to be instantiated with the new token should be named with a capital.

eg

function MyObj(data) { this.data = data }
const obj = new MyObj("foo");

To add to the complexity you can create functions that can be instantiated with or without the new token, commonly known as factory functions. The convention is somewhat ambiguous in this case.

Equality operators

In JS variables are cast automatically. This presents a problem when using the equality operators as it is often the case that you want to test the type as well. To solve this JS has 4 types of equality operators.

  • Equality == coerces the right side to the same type as the left
  • InEquality != coerces the right side to the same type as the left
  • Strict Equality === There is no type coercion
  • Strict Inequality !== There is no type coercion

As type coercion in Javascript is a little complex newbies can get caught out using Equality operators.

The convention is to err on the side of type and use the strict version of equality === and inequality !== . Its a little hard getting used to but you very seldom need to use == or != in JavaScript.

Declaration type.

Good to see appropriate use of var (for function scope) and let (for block scope) variables. But you have neglected const for constants, which are block scoped. Variables reference Arrays and Objects so unless you are reassigning the reference its is conventional to use const for references.

When JS is parsed function scoped variables are automatically moved to the top of their scope (hoisted). It is conventional to also do this in the source.

Commenting your source

Rather than list out all the other points I have taken one function and modified it with comments and code to better match best practice

"use strict"; // For better code quality and improved performance 
              // Add this directive to the top (first line) of the function
              // or the first line of a javascript file or script element

function updateCard(widget, widgetData) {  // myWidget ?? better as widget or card
    // Functions can be scoped to functions and helps with encapsulation so
    // zeroPad would be best here if this function is the only place you use it
    // they are also hoisted and belong at the top of their scope.
    function zeroPad(num, places) {
       var zero = places - num.toString().length + 1;
       return Array(+(zero > 0 && zero)).join("0") + num;
    }

    // You can also use arrow syntax as a const
    const zeroPad = (num, places) => {
       const zero = places - num.toString().length + 1;
       return Array(+(zero > 0 && zero)).join("0") + num;
    }

    // Yet why not just use String.padStart
    // and a default value for places that is assigned if places is undefined
    const zeroPad = (num, places = 2) => ("" + num).padStart(places,"0");
    // the ("" + num) coerces num to a string

    // However you should be using locale format for date and time.


    // function scoped vars hoisted to top of function
    // var data; // but this should be a const in the for loop

    // card as a constant
    const card = widget;  // do you really need an alias. Why not name the argument card?

    if (widgetData.AlarmNumber !== 0) {  // strict inequality
        card.style.backgroundColor = "red"; 
        card.classList.add("blink-me");  // can use element.classList
                                         //  https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
    } else if (widgetData.ExecutionMode !== "Running")  { // else on same line as closing }
        card.className = "card";
        card.style.backgroundColor = "Orange";
    } else {
        card.style.backgroundColor = null; // or assign "" and then use a ternary rather than two statements
        card.className = "card";
    }

    /*
    Would be preferable to use locale for date and time or the date/time can
    be incorrectly interpreted.
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString
    */
    const dateTime = new Date();
    /* Use template literals
       https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals */
    const dateString = `${dateTime.toLocaleDateString()} "${dateTime.toLocaleTimeString()}`;


    addDataElement(card, "title", widgetData.machineID, false); // spaces after commas
    addDataElement(card, "subtitle", widgetData.executionMode, false);

    // You are not using the index i apart from indexing the property
    /*
    for (let i = 0; i < properties.length; i++) {
        data = widgetData[properties[i]];
        addDataElement(card, properties[i], data);
    }
    */

    // use a for of loop
    for (const property of properties) {
        /* const data = widgetData[property];  // data better as const rather than function scoped */
        addDataElement(card, property, widgetData[property]); // or just add data directly
    }


    addDataElement(card, "Timestamp", dateString);

}


// You can assign properties via Object.assign
newdiv = document.createElement("div");
newdiv.className = "card-title";
newdiv.id = "title";
newdiv.textContent = "title";
parent.appendChild(newdiv);

// can be
parent.appendChild(
    Object.assign(document.createElement("div"), {
    className : "card-title",
    id : "title",
    textContent : "title",
}));

// Better yet as you repeat the same code over and over use some functions
const appendTo = (el, tag, props) => el.appendChild( Object.assign(document.createElement(tag), props))
const props = (className, id, textContent = id) => ({className, id, textContent});

// and your source code complexity is reduced

var parent= document.createElement("div");
parent.className = "card";
parent.id = "widget-" + cardID; 
parent.style = "height:500px;";
appendTo(parent, "div", props("card-header", "")); 
const card = parent;
parent = appendTo(parent, "div", props("card-body", "cardbody", "")); 
appendTo(parent, "div", props("card-title", "title"));
appendTo(parent, "div", props("card-sub-title", "subtitle"));    



// Pulling out the repeated code
const createDiv = () => document.createElement(tag);
const appendTo = props => parent.appendChild( Object.assign(createDiv(), props));
const props = (className, id, textContent = id) => ({className, id, textContent});

var parent = createDiv();
const card = parent;
parent.className = "card";
parent.id = "widget-" + cardID; 
parent.style = "height:500px;";
appendTo(props("card-header", "")); 
parent = appendTo(props("card-body", "cardbody", "")); 
appendTo(props("card-title", "title"));
appendTo(props("card-sub-title", "subtitle"));    
parent.appendChild(createDiv());

return card;

Without the comments and copied code sections the above updateCard function becomes

"use strict"; 
function updateCard(card, data) {  

    if (data.alarmNumber !== 0) {  
        card.style.backgroundColor = "red"; 
        card.classList.add("blink-me");  
    } else {
        card.className = "card";
        card.style.backgroundColor = data.executionMode !== "Running" ? "Orange" : "";
    }

    addDataElement(card, "title", data.machineID, false);
    addDataElement(card, "subtitle", data.executionMode, false);

    for (const property of properties) {
        addDataElement(card, property, data[property]); 
    }

    const dateTime = new Date();
    addDataElement(
        card, 
        "Timestamp", 
        `${dateTime.toLocaleDateString()} ${dateTime.toLocaleTimeString()}`
    );
}
\$\endgroup\$
  • \$\begingroup\$ This thorough answer is much appreciated. Followup question: in this line: card.style.backgroundColor = "red"; How can I replace "red" with a color defined in my CSS file? in my css file i have this: .color-primary-0 { color: #2800C0 } \$\endgroup\$ – GisMofx Aug 30 '18 at 20:26
  • 1
    \$\begingroup\$ @GisMofx Add the class to the className via classListfor example card.classList.add("color-primary-0") \$\endgroup\$ – Blindman67 Aug 30 '18 at 23:50
  • \$\begingroup\$ I see. In my case, I need to replace color with background-color in the css class. \$\endgroup\$ – GisMofx Aug 31 '18 at 14:12
  • \$\begingroup\$ I'm having cross-browser issues with IE11. "use strict", for (const property... loop, etc. What do you recommend as a solution? Would JQuery or some other package solve some the browser difference issues? I've update the code, such that it works for IE11, but just wondering what's best to do here. \$\endgroup\$ – GisMofx Sep 21 '18 at 20:00

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.