Fast prototype code to hook up a webcam to the Azure Emotion API

Question

Last week I had a 5 hours school project deadline based on the Azure Emotion API. Right now I am trying to rewrite the same code in a more clean/elegant way. I am quite new to coding so I will need all the help I can get. I am not really looking for a shorter way to write the same code, but I am looking for a clean/understandable way.

(function () {
  let tempEmotion = ''
  let lastEmotion = ''

  function ShowCam () {
    Webcam.set({
      width: 480,
      height: 360,
      image_format: 'jpeg',
      jpeg_quality: 100
    })
    Webcam.attach('#myCamera')
  }

  function TakeSnapshot () {
    Webcam.snap(function (dataUri) {
      document.getElementById('results').innerHTML = '<img id="base64image" src="' + dataUri + '"/>'
    })
    SaveSnapshot()
  }

  function SaveSnapshot () {
    let file = document.getElementById('base64image').src.substring(23).replace(' ', '+')
    let img = Base64Binary.decodeArrayBuffer(file)
    let ajax = new XMLHttpRequest()

    ajax.addEventListener('load', function (event) {
      UploadComplete(event)
    }, false)
    ajax.open('POST', 'https://api.projectoxford.ai/emotion/v1.0/recognize', 'image/jpg')
    ajax.setRequestHeader('Content-Type', 'application/octet-stream')
    ajax.setRequestHeader('Accept', 'text/html,application/xhtml+xml,application/xml')
    ajax.setRequestHeader('Ocp-Apim-Subscription-Key', 'EXTREMELY_HIDDEN_KEY')
    ajax.send(img)
  }

  function UploadComplete (event) {
    let xmlDoc = event.target.responseXML
    let result = xmlDoc.getElementsByTagName('scores')
    GiveUserFeedback(result)
  }

  function GiveUserFeedback (result) {
    let emotionArray = []
    let tempValue = 0
    let htmlBody = $('body')
    if (result[0]) {
      for (let i = 0; i < result[0].childNodes.length; i++) {
        emotionArray.push(Math.floor((Number(result[0].childNodes[i].textContent) * 100) + 1))
        if (Number(result[0].childNodes[i].textContent).toFixed(2) > tempValue) {
          tempValue = Number(result[0].childNodes[i].textContent).toFixed(2)
          tempEmotion = String(result[0].childNodes[i].nodeName)
          tempEmotion = tempEmotion.substr(0, 1).toUpperCase() + tempEmotion.substr(1)
        }
      }
      for (let i = 0; i < emotionArray.length; i++) {
        $('ul li:nth-child(' + (i + 1) + ') span').css('height', emotionArray[i])
      }
      switch (tempEmotion) {
        case 'Angry':
          htmlBody.css('background-color', '#F44336')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Contempt':
          htmlBody.css('background-color', '#9C27B0')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Disgust':
          htmlBody.css('background-color', '#673AB7')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Fear':
          htmlBody.css('background-color', '#FF9800')
          break
        case 'Happiness':
          htmlBody.css('background-color', '#4CAF50')
          break
        case 'Neutral':
          htmlBody.css('background-color', '#FFFFFF')
          break
        case 'Sadness':
          htmlBody.css('background-color', '#03A9F4')
          break
        case 'Surprise':
          htmlBody.css('background-color', '#FFEB3B')
      }
      lastEmotion = tempEmotion
    }
  }

  window.onload = ShowCam
  window.setInterval(function () {
    TakeSnapshot()
  }, 3000)
})()

Answer 1

Dealing with DOM

document.getElement[s]By*(...) competes with $(...). Do not mix DOM accessing styles. That hurts readability. Either use jQuery everywhere, or native API everywhere. jQuery is kind of out of fashion nowadays, mainly because major browsers got much better in exposing DOM accessing APIs.

Dealing with Http

Again, since your code already depends on jQuery, I don't see why prefer XMLHttpRequest over $.ajax(). Inconsistency of the code is one of the biggest issues when it comes to maintenance, especially in case of new hires.

Separate Data from Logic

Your mood to color scheme relationship is data that can be encapsulated in a separate place (apart from the places where it's being used). Say, you define color scheme like this:

  var schemeOfMood = {
      'Angry': { htmlBackground: '#F44336', htmlColor: '#FFFFFF', spanBackground: '#FFFFFF' },
      'Contempt': { htmlBackground: '#9C27B0', htmlColor: '#FFFFFF', spanBackground: '#FFFFFF' },
      // ...
  };

then the coloration logic becomes trivial, and no ugly switch is needed at all.

  var scheme = schemeOfMood[tempEmotion];
  htmlBody.css('background-color', scheme.htmlBackground);
  htmlBody.css('color', scheme.htmlColor);
  $('span').css('background-color', scheme.spanBackground);

The other good effect is the reduced code repetition.

Move Magic Code into Functions

When I see lines like that they look Magic to me.

let file = document.getElementById('base64image').src.substring(23).replace(' ', '+')

What is subsctring(23)? Why 23 and not 32 (I like powers of 2 more) -- "magic numbers"? Why replace ' ' with '+'?

If you extract this one-liner into a function, you can give it a good name, thus expressing your intent. At the moment, I didn't even try to understand why that manual transformation of a base64 image is needed.

Unclear Intent

When giving user a feedback, there's this if (result[0]) { ... } that makes the code look only into the first result. Why is that?

Put it into a variable, give it a name, and you will avoid the hard-to-read result[0] and explain what's going on.

var lastChronologicalResult = result[0];
if (lastChronologicalResult) {
  // work against lastChronologicalResult
}

---

Very similarly, the code loops through multiple results' child nodes (for (let i = 0; i < result[0].childNodes.length; i++) { ... }) and does two things:

A) populates emotionArray based on each child;

B) calculates the tempEmotion based on the last child which has .textContent.toFixed(2) > tempValue. Nobody knows what textContain holds and what's the tempValue.

So, better naming things and function extraction would clarify what is being done in that part of the code.