Merging JSON and inserting into the DOM

Question

I'm an aspiring junior dev. This is modified version of the code I wrote for a technical interview I did today (Didn't pass. Sad.). The problem was to fetch two json files, and display their data on a table. No JQuery, only a simple un-editable html file.

I'm looking for:

• Ways I could have made the code more readable, improved variable names, commenting.
• Security problems with injecting data into the DOM
• Is the pattern of trying to normalize the data correct?
• Is it good to curry functions like this?
• Should I have memoized the user data in a store?
• (Only if it's allowed on CR) Tips for an interview question like this.

const apiAddress = 'https://api.myjson.com/bins/'
const tableSchema = {short_name: 'User', logins: 'Logins', attempts: 'Attempts'}
// JSON data stored at
// https://api.myjson.com/bins/14pl91
// https://api.myjson.com/bins/xkdzp

/**
 * Fetch should be polyfilled if supporting older browsers
 * 
 * (String) => Object
 * @param {String} path The localpath and filename of the json file
 * @returns {Object}
 */
const fetchData = (path) => {
  return fetch(apiAddress + path)
    .then(res => {
      return res.json()
    }).catch(console.error)
}

/**
 * Use this: https://github.com/paularmstrong/normalizr instead
 * 
 * (String) => (Array) => Object
 * @param {string} key the key used to tell if different objects are representing the same thing
 * @param {[Object<{key: String}>]} objectArray an array of objects, each object must contain the key as a property
 * @returns {Object<{[key: String]: Object}>} Returns an object that contains the input objects mapped by the key
 */
const normalizer = (key) => (objectArray) => {
  return objectArray.reduce((acc, cur) => {
      if (cur.hasOwnProperty(key)) {
        return Object.assign(acc, {[cur[key]]: {...cur}})
      }
    }, {})
}

/**
 * ([Object]) => Object
 * @param {[Object<{[key: String]: Object}>]} objectArray An array of keyed (normalized) objects that will be merged by said key
 * @returns {Object<{[key: String]: Object}>} The merged object
 */
const mergeObjects = (objectArray) => {
  return objectArray.reduce((acc, cur) => {
    Object.keys(cur).map((key) => {
      acc[key] = Object.assign(cur[key], acc[key])
    })
    return acc
  }, {})
}

/**
 * Should place these table functions within a class, prototypes, component
 * 
 * (schema: Object<ITableSchema>) => Table
 * @param {Object<ITableSchema>} schema The schema the table will conform to
 * @returns {Table} A new table instance
 */
const createTable = (schema) => {
  const table = document.createElement('table')
  const headerRow = table.createTHead().insertRow()
  
  // Add the titles in each column
  obj2Arr(schema).forEach((title) => {
    const th = document.createElement('th')
    th.innerText = title
    headerRow.appendChild(th)
  })
  
  return table
}

/**
 * (table: Table, fields: [String]) => (entry: Object<any>) => void
 * @param {Table} table Adds rows to a table reference
 * @param {[String]} fields Fields is an ordered array of keys to get from the entry data
 * @param {Object<any>} entry Data that is to be inserted into the row cells
 */
const createRowInTable = (table, fields) => (entry) => {
  const row = table.insertRow()
  fields.forEach((field) => {
    const cell = row.insertCell()
    cell.innerText = entry[field]
  })
}

/**
 * Utils
 * Makes everything below a bit cleaner and easier to follow
 * Would like to place these in separate files or use lodash
 *
 * @param {Object} object A mapped object
 * @returns {Array} An array without mappings
 */
const obj2Arr = (object) => Object.keys(object).map((key) => object[key])

/**
 * Filters objects that don't contain all keys
 *
 * @param {[String]} keys Keys to check exist in the object
 * @param {Object} object Object that will be checked contains keys
 * @returns {Boolean}
 */
const objFilter = (keys, object) =>
  keys.reduce((acc, cur) => {
    return acc ? object.hasOwnProperty(cur) : false
  }, true)

/**
 * init() main() <- for people that cmd+f
 */
Promise.all([
  fetchData('14pl91'),
  fetchData('xkdzp')
]).then(res => {

  // Manually strip the response, ugly but can't think of a better way
  const strippedResponse = res.map((ele) => {
    if (ele.hasOwnProperty('users')) return ele.users
    if (ele.hasOwnProperty('user_stats')) return ele.user_stats
  })

  // Process the response
  const normalizedUsers = strippedResponse.map(normalizer('id'))
  const users = mergeObjects(normalizedUsers)

  // Filters users without the requisite fields
  const columnFilter = objFilter.bind({}, Object.keys(tableSchema)) // To show PA instead of currying
  const filteredUsers = obj2Arr(users).filter(columnFilter)

  // Build the table
  const table = createTable(tableSchema)
  
  // Put the filtered users inside the table
  const createUserRow = createRowInTable(table, Object.keys(tableSchema))
  filteredUsers.forEach(createUserRow)

  // Find the table's injection point
  const container = document.getElementsByClassName('json-table')[0]
    .lastElementChild
    .lastElementChild

  // Place table inside DOM
  container.appendChild(table)
})

<!DOCTYPE html>
<html>
<head>
 <script src="script.js"></script>
</head>
<body>
  <div class="json-table">
    <h3>JSON Table</h3>
    <div class="module-border">
      <div class="module-content">
      </div>
    </div>
  </div>
</body>
</html>

Note: I've removed all references to the company and mutated the data while keeping the structure and problem identical.

EDIT

Question Specification

• Fetch the contents of the JSON users.json and stats.json
• The purpose of the table is to show user stats. If a user does not have stats, don't show them.
• Parse the JSON files and display their data using a table as shown below, using the short_name, logins, and attempts fields.

Answer 1

const fetchData = (path) => {
  return fetch(apiAddress + path)
    .then(res => {
      return res.json()
    }).catch(console.error)
}

The problem with this is that the caller of fetchData will not be aware of the error, since catch did its job - catch the error. Don't catch, and if it's for logging, catch and rethrow. Allow the caller to deal with the rejection.

---

// Manually strip the response, ugly but can't think of a better way
const strippedResponse = res.map((ele) => {
  if (ele.hasOwnProperty('users')) return ele.users
  if (ele.hasOwnProperty('user_stats')) return ele.user_stats
})

This makes the code aware of the presence of "users" and "user_stats". You could push this off to the part of the code that was still specific, moving it away from the more generic parts. For instance, tack on another then on the fetch that extracts the property.

---

// Process the response
const normalizedUsers = strippedResponse.map(normalizer('id'))
const users = mergeObjects(normalizedUsers)

You did not have to normalize both arrays. What ended up happening was you turned an array to an object, but then unpacked the object into an array with mergeObjects. Lots of packing and unpacking going on. You could have turned only one of them as a lookup, while you map through the other.

---

// Build the table
const table = createTable(tableSchema)

// Put the filtered users inside the table
const createUserRow = createRowInTable(table, Object.keys(tableSchema))
filteredUsers.forEach(createUserRow)

// Find the table's injection point
const container = document.getElementsByClassName('json-table')[0]
  .lastElementChild
  .lastElementChild

// Place table inside DOM
container.appendChild(table)

This was entirely unnecessary. True, this would be The Right Way™ to do it in JS, but since the requirement was for one-time render, you could have just constructed the table as a string and used innerHTML.

---

In summary, nothing is wrong with the code. This is what I would have done initially, laying out the things as them come into mind (the grouping, the merging, the filtering, the rendering). It's when you take a second look, and once you have taken into account other factors, is when you start to optimize.

const container = document.getElementById('stats')
const users = fetch('https://api.myjson.com/bins/14pl91').then(r => r.json()).then(r => r.users)
const userStats = fetch('https://api.myjson.com/bins/xkdzp').then(r => r.json()).then(r => r.user_stats)

Promise.all([users, userStats]).then(([users, userStats]) => {
  // Convert stats into a lookup
  const statsLookup = userStats.reduce((c, v) => (c[v.id] = v, c), {})

  const userData = users
    // Remove those users with no stats
    .filter(u => statsLookup.hasOwnProperty(u.id))
    // Grab the data we need
    .map(u => ({
      short_name: u.short_name,
      logins: statsLookup[u.id].logins,
      attempts: statsLookup[u.id].attempts
    }))

  // Render
  container.innerHTML = `
    <table border="1">
      <thead>
        <tr>
          <th>User</th>
          <th>Logins</th>
          <th>Attempts</th>
        </tr>
      </thead>
      <tbody>
        ${userData.map(u => `
          <tr>
            <td>${u.short_name}</td>
            <td>${u.logins}</td>
            <td>${u.attempts}</td>
          </tr>
        `).join('')}
      </tbody>
    </table>
  `
}).catch(e => {
  // Something went wrong. Do something.
})

<div id="stats"></div>