How do I simplify this code? Is there a better way of doing this?

Question

I just got this code working after hours of stress. I am new to Javascript so I am not sure if I did this the most efficient way possible. I am using an API that is provided by IEX. The goal of this code is to output out news when there is some. This isn't completely working as you can tell, but I did get the headline to work. So if I am doing anything wrong let me know please.

<html>
<head>
    <style>
        /* Outter Table <Tbody> Settings*/
        .outtertable tbody {
            vertical-align: top;
        }

        /* Innertable Table data settings */
        .innertable tr > td {
            vertical-align: top;
        }

        /* Div Article Holder Settings */
        .divBorder {
            margin-bottom: 10px;
            border: solid; 
            border-color: #c4ef8b; 
            border-width: 4px 0px 0px 0px;
        }

        /* Article Image settings */
        .articleImg {
            height:50px; 
            width: 50px;
        }

        /* DivBorder Mouse Hover */
        .divBorder:hover {
            cursor: pointer;
            background-color: #f3ffe5;
        }
    </style>
</head>

<body>
    <table class="outterTable" id="newsContent"></table>
</body>

<script>
    var request = new XMLHttpRequest();
    request.open ('GET', 'https://api.iextrading.com/1.0/stock/spy/news')

    //on request load
    request.onload = function() {
        //VARIABLES
        var newsContainer = document.getElementById("newsContent");

        var JSONData = JSON.parse(request.responseText);
        var articleAmount = JSONData.length;
        var rowAmount = articleAmount / 3;
        var rowAmountRoundDown= Math.trunc(rowAmount); 
        var rowAmountRoundUp = (Math.trunc(rowAmount) + 1);
        var remainder = (rowAmount - Math.floor(rowAmount)).toFixed(2); //.00, .67, or .33;

        //=== TABLE CREATOR =============================================
        //Create an "<tbody>" element
        let tbodyHTML = document.createElement('tbody');

        //"Assembler" inside is "createTable()"
        tbodyHTML.innerHTML = createTable();

        //FUNCTION Create Table
        function createTable() {
            var tData = '';
            var index = 0; 

            //========= First Table Part Row Loop ===========================================================
            for (var i = 1; i <= rowAmountRoundDown; i++) {         
                //Row Start
                tData = tData + `
                    <tr>
                `;

                //Content: <td> <div> <table> <tr> <td>
                for (var c = 1 + index; c < 4 + index; c++) {
                    tData = tData + `
                        <td style="width: 33.33%; padding: 0px 25px">
                            <div class="divBorder">
                                <table class="innerTable">
                                    <tbody>
                                        <tr>
                                            <td>
                                                <img class="articleImg" src="images/seeking-alpha-badge.png" id="image${c}">

                                            </td>
                                            <td style="padding-left: 5px">
                                                <h3 id="headline${c}"></h3>
                                            </td>
                                        </tr>
                                    </tbody>
                                </table>        
                            </div>
                        </td>
                    `;
                }

                //Row End
                tData = tData + `
                    </tr>
                `;

                index = index + 3;
            }

            //========= Second table part =====================================================================
            //If remainder is .67 create 2 <td>
            if (remainder == 0.67) {
                //Row Start
                tData = tData + `
                <tr>
            `;

            //Content: <td> <div> <table> <tr> <td> 
            for (var c2 = (1 + index); c2 < (3 + index); c2++){
                tData = tData + `
                    <td style="width: 33.33%; padding: 0px 25px">
                        <div class="divBorder">
                            <table class="innerTable">
                                <tbody>
                                    <tr>
                                        <td>
                                            <img class="articleImg" src="images/seeking-alpha-badge.png" id="image${c2}">

                                        </td>
                                        <td style="padding-left: 5px">
                                            <h3 id="headline${c2}"></h3>
                                        </td>
                                    </tr>
                                </tbody>
                            </table>        
                        </div>
                    </td>
                `;
            }               

            //row End
            tData = tData + `
                </tr>
            `;

            //If remainder is .33 create 1 <Td>
            } else if (remainder == 0.33) {
                //Row Start
                tData = tData + `
                    <tr>
                `;

            //Content: <td> <div> <table> <tr> <td> 
            for (var c = (1 + index); c < (2 + index); c++){
                tData = tData + `
                    <td style="width: 33.33%; padding: 0px 25px">
                        <div class="divBorder">
                            <table class="innerTable">
                                <tbody>
                                    <tr>
                                        <td>
                                            <img class="articleImg" src="images/seeking-alpha-badge.png" id="image${c}">

                                        </td>
                                        <td style="padding-left: 5px">
                                            <h3 id="headline${c}"></h3>
                                        </td>
                                    </tr>
                                </tbody>
                            </table>        
                        </div>
                    </td>
                `;

            }               

            //row End
            tData = tData + `
                </tr>
            `;

            //Anything else dont do anything
            } else {
                tData = tData;
            }

            return tData;
        }

        //Inject into the HTML
        newsContainer.appendChild(tbodyHTML);
        //===============================================================
        var red = (JSONData.length + 1)
        console.log(red);

        //Output data to html
        for (var l = 1; l < red; l++){
            console.log("l: " + l);
            spyOutputToHTML(JSONData, l);
        }

    };

    function spyOutputToHTML(data, i) {
        //get current variables in this HTML page x3
        var offset = i - 1;
        var headline = document.getElementById(`headline${i}`);

        //Get Content From the JSON file ex: ".latestPrice"
        var JSONHeadline = data[offset].headline;

        //Inject data into HTML
        headline.innerHTML = JSONHeadline;
    }

    request.send()
</script>

Don't mix and match HTML strings with DOM element creation. Just use the DOM and, in particular, the [Table API](https://developer.mozilla.org/en-US/docs/Web/API/HTMLTableElement) — Scott Marcus, Feb 07 '19 at 01:19
i have implemented this code into my site. you can see it in action here: http://www.yieldsync.com/ i still need to fix it and try to integrate what the wonderful user commented below — Aleem Ahmed, Feb 07 '19 at 18:26

score 1 · Answer 1 · answered Feb 07 '19 at 07:13

First of all, great job! This is impressive work for a newbie in javascript

Few things could definitely be improved, but I don't see you're doing anything wrong. Maybe, only the logic with remainder is too confusing. I bet there should be an easier way

Readability

Your code would be undoubtedly easier to read and understand if you had the view (templating) logic, the request logic, and the data "massaging" logic separated

The view logic

Generally, constructing HTML structures "by hand" (createElement, appendChild) takes more effort and is, arguably, more confusing as opposed to rendering a string with a template function (kind of like you did) and injecting the result where you need it. Mixing these approaches is even more error-prone than doing everything "by hand". So, I would suggest you have one view/ template function that would take data and return a string

function renderTable(data) {
    var result = '<div>';
    // build the result string...
    result += '</div>';
    return result;
}

// and then...
targetEl.innerHTML = renderTable(data);

You may also want to leverage micro-templating. One or another kind of templating would be a must-have for a larger application. Make yourself familiar with templating engines. For your project, building strings with javascript is fine. Although, there is a more advanced technique you can consider

The data "massaging" logic

Well, that comes down to having your template function not being "smart" about its context (basic separation of concerns), and only consuming the data. Not "cooking" it, only eating it :)

So, instead of doing this

function renderTable(data) {
    var result = '<div>';
    var something = data.abc * data.xyz;
    // do something with something here
    result += '</div>';
    return result;
}

targetEl.innerHTML = renderTable(data);

... you do this

function adaptResponsePayloadData(data) {
    var result = { ...data };
    result.something = result.abc * result.xyz;
    return result;
}

function renderTable(data) {
    // ...
}

targetEl.innerHTML = renderTable(adaptResponsePayloadData(data));

This is an example of the so-called Adapter design pattern. This is the right case for using it. There are many other design patterns and I strongly recommend to dedicate time to make yourself familiar with them

The request logic

Another concern separation here. You can have the request logic in a separate function, similarly how we separated "massaging" from the view above

const API_ENDPOINT_URL = 'https://api.iextrading.com/1.0/stock/spy/news';

function fetchData(callback) {
    var request = new XMLHttpRequest();
    request.open('GET', API_ENDPOINT_URL);
    request.onLoad(() => {
        var data = JSON.parse(request.responseText);
        callback(data);
    });
    request.send();
}

// ... and then
fetchData(data => {
    // ...
    targetEl.innerHTML = renderTable(adaptResponsePayloadData(data));
});

Note on execution order

There is the rule of thumb to make it clear when your code is run. This is totally a nit-pick, but it is possible to separate what from when on the code level

function goOn() { // there are many conventional names for this, like `main`
    fetchData(data => {
        document.body.innerHTML = renderTable(adaptResponsePayloadData(data));
    });
}

window.onload = goOn;

Notes on HTML and CSS

You don't really need <tbody>. It is not needed unless you want to highlight something with CSS
Avoid using inline styles, like <td style="width: 33.33%; padding: 0px 25px">. You can express that with CSS
You don't need the divBorder class. Add padding to the parent td and border to the child table

Other minor notes

Conventionally, names with the first capital letter are object constructors or classes. Simply, make regular var names lowerCamelCase, like jsonHeadline

JSON is a term for notation we know. When we parse a string of that notation, it simply becomes data or contextData, you got it... Then, what's inside that data becomes simply headline

Do your best at naming variables so that you don't have to write comments to understand what you meant. Find other good tips here

Make sure your production code has no console.log statements

let keyword is safer than var. You'd never pollute the global scope with it

Please mind that there is Code Review on StackExchange where you can learn many other aspects of how to write great code

You did really great. Good luck to you on your journey! :)

How do I simplify this code? Is there a better way of doing this?

1 Answers1