0
\$\begingroup\$

I am fetching a list of messages from some JSON and appending each message as an LI within a UL. I have created the message template within jQuery to append each message to the document. My code works well but I can't help but feel it could be improved (I am a beginner JS programmer). From what I have seen online moving the templating to handlebars.js would be a better solution but I can't be sure.

$(document).ready(function() {
  var broadcastMessagesJsonURL = "https://api.myjson.com/bins/sadap";
  $.getJSON(broadcastMessagesJsonURL, function(data) {
  $.each(data.broadcastMessages, function(i, item) {

  //All items from the json
  var broadcastMessageID = (item.ID);
  var checkBroadcastMessageRead = (item.read ? " broadcast__message__read__state--unread" : "");
  var broadcastMessageSubject = (item.subject);
  var broadcastMessageGroup = (item.group);
  var broadcastMessageDateSent = (item.dateSent);
  var checkBroadcastFeatureImage = (item.featureImage ? " broadcast__message__image--active" : "");
  var checkBroadcastForm = (item.form ? " broadcast__message__form--active" : "");
  var checkBroadcastAttachments = (item.attachments ? " broadcast__message__attachment--active" : "");
  var broadcastMessageContent = (item.content);

  var broadcastMessageTemplate = ('<li class="broadcast__message__list__item" data-broadcast-message-ID="' + broadcastMessageID + '"> \
                                    <div class="broadcast__message__wrapper"> \
                                      <div class="broadcast__message__read__state' + checkBroadcastMessageRead + '"></div> \
                                      <div class="broadcast__message__subject">' + broadcastMessageSubject + '</div> \
                                      <div class="broadcast__message__group">' + broadcastMessageGroup + '</div> \
                                      <div class="broadcast__message__date__time__stamp" title="' + broadcastMessageDateSent + '">' + broadcastMessageDateSent + '</div> \
                                      <div class="broadcast__message__snippet">' + broadcastMessageContent + '</div> \
                                    </div> \
                                  </li>');
  $(".broadcast__messages__list").append(broadcastMessageTemplate);
});
  }).fail(function() {
    console.log("broadcastMessages json cannot be loaded");
  });
});

CODEPEN LINK

\$\endgroup\$
4
\$\begingroup\$

Rewrite

Below is a rewrite with all my further suggestions applied.

const messagesURL = 'https://api.myjson.com/bins/sadap';

const messagesLoadError = () => console.error('broadcastMessages json cannot be loaded');

const forEachMessage = (_, message) => {
  const messageRead  = message.read         ? ' broadcast__message__read__state--unread' : '',
        featureImage = message.featureImage ? ' broadcast__message__image--active'       : '',
        form         = message.form         ? ' broadcast__message__form--active'        : '',
        attachments  = message.attachments  ? ' broadcast__message__attachment--active'  : '';

  const template = `<li class="broadcast__message__list__message" data-broadcast-message-ID="${message.ID}">
    <div class="broadcast__message__wrapper">
      <div class="broadcast__message__read__state${messageRead}"></div>
      <div class="broadcast__message__subject">${message.subject}</div>
      <div class="broadcast__message__group">${message.group}</div>
      <div class="broadcast__message__date__time__stamp" title="${message.dateSent}">${message.dateSent}</div>
      <div class="broadcast__message__snippet">${message.content}</div>
    </div>
  </li>`;

  $('.broadcast__messages__list').append(template);
};

$.getJSON(messagesURL, data => $.each(data.broadcastMessages, forEachMessage)).fail(messagesLoadError);
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"
        integrity="sha384-fj9YEHKNa/e0CNquG4NcocjoyMATYo1k2Ff5wGB42C/9AwOlJjDoySPtNJalccfI"
        crossorigin="anonymous">
</script>

Remarks

API security

Your API endpoint (https://api.myjson.com/bins/sadap) uses insecure and archaic SSL configuration. Actually, I couldn't connect to it at all, since I have tweaked up browser configuration. Few key points about your API's security:

See Qualys' test result.

Unused callback's parameter

You have the following line in your code:

$.each(data.broadcastMessages, function(i, item) {

but i is never used. In such cases, it's good to replace it with underscore (_).

Naming issues

As Leon Bambrick added on top of Phil Karlton's quote:

There are only two hard things in Computer Science: cache invalidation, naming things, and off-by-one errors.

„Some” of your names are overly verbose (word broadcast seem to be everywhere) and other are not descriptive enough, like item.

Wall of text

This

var broadcastMessageID = (item.ID);
var checkBroadcastMessageRead = (item.read ? " broadcast__message__read__state--unread" : "");
var broadcastMessageSubject = (item.subject);
var broadcastMessageGroup = (item.group);
var broadcastMessageDateSent = (item.dateSent);
var checkBroadcastFeatureImage = (item.featureImage ? " broadcast__message__image--active" : "");
var checkBroadcastForm = (item.form ? " broadcast__message__form--active" : "");
var checkBroadcastAttachments = (item.attachments ? " broadcast__message__attachment--active" : "");
var broadcastMessageContent = (item.content);

would be much easier to read, if you would combine vars and align equality signs, question marks and colons:

var broadcastMessageID         = item.ID,
    broadcastMessageSubject    = item.subject,
    broadcastMessageGroup      = item.group,
    broadcastMessageDateSent   = item.dateSent,
    broadcastMessageContent    = item.content,
    checkBroadcastMessageRead  = item.read         ? ' broadcast__message__read__state--unread' : '',
    checkBroadcastFeatureImage = item.featureImage ? ' broadcast__message__image--active'       : '',
    checkBroadcastForm         = item.form         ? ' broadcast__message__form--active'        : '',
    checkBroadcastAttachments  = item.attachments  ? ' broadcast__message__attachment--active'  : '';

Variables

Most of your variables are unnecessary. If you would rename ambiguous item to message, you could use message.ID instead of doing var broadcastMessageID = (item.ID).

Also, bear in mind that the following three variables are unused: checkBroadcastFeatureImage, checkBroadcastForm, checkBroadcastAttachments.

Use template literals

var broadcastMessageTemplate could become much more easily readable if you used template literals to declare it. Mind however, that this is still not the „most vanilla” method to create DOM elements either. Take a look at createElement().

Fail notification

You should use console.error() instead of console.log() in your fail's callback.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Pro tip from a person who used to align variable names and ternaries - they're readable, true. But they're a hassle to maintain, especially when another, longer-named variable comes in. Commits become noisy because of alignment adjustments. \$\endgroup\$ – Joseph Apr 28 '17 at 22:13
  • 1
    \$\begingroup\$ Thank you for an excellent answer! The only part that I don't really understand is but i is never used. In such cases, it's good to replace it with underscore (_). Why would I still need the (_)? \$\endgroup\$ – 2ne Apr 29 '17 at 12:41
  • 1
    \$\begingroup\$ @2ne: Because in your callback where i appears, you need to access only it's second parameter. As such, you must name somehow all the preceding parameters, as well. The first one is never actually used, so by convention it should be named _. \$\endgroup\$ – Przemek Apr 29 '17 at 15:31
  • 1
    \$\begingroup\$ @JosephtheDreamer: Somehow valid, although I think that in this case pros would top the cons. But it's a matter of personal preference, of course. \$\endgroup\$ – Przemek Apr 29 '17 at 15:34

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.