JSFiddle - React, Tailwind, and code Playground

codility - 4 - code review

by Csaba Hellinger

HTML

There are lot of code smells and one bug in this. Change innerHtml to innerHTML to make it work.

<div id="messages">
</div>

JavaScript

const fetch = url => Promise.resolve(JSON.stringify([
	{text: 'message1', public: true},
  {text: 'message2', public: false},
  {text: '<button onclick="javascript:alert(\'boom!\');">X</button>', public: true}
]));

const API_BASE_URL = '';

// this function does two different things: fetch & render ui. these should be separated.
function populateMessages() {
  // could be const
  // what if it's missing?
  let messagesDiv = document.getElementById('messages');
  // the promise chain should be returned in case of the caller wants to continue the chain.
  fetch(API_BASE_URL + '/messages')
    .then(json => JSON.parse(json))
    .then(messages => {
      // what if it has a million items?
      messages.forEach(message => {
        // early return, or message.filter would be more readable
        // unnecessary double equal, simple truthyness check would be enough
        if (message.public == true) {
          // we could use some templating language instead of creating elements by hand
          // messageDiv would be a better name
          const div = document.createElement('div');
          // message text should be sanitized
          // should be "innerHTML"
          div.innerHtml = message.text;
          // appending each child individually is much slower then building the whole list outside the DOM then appending it once.
          messagesDiv.appendChild(div);
        }
      });
    })
    // no reason to put this in a separate `then`
    // should return messages in the previous `then` to make it accessible here
    .then(messages => {
      console.log('processed messages:', messages);
    });
    // missing catch
}

populateMessages();