3
\$\begingroup\$

I've got the following single page web app, bundled with Webpack, that simply makes a XMLHttpRequest to get some data and output it to the page. It then provides four buttons to filter the results.

This is my first attempt at MVC in JavaScript and I'm pretty sure I've got a few areas that could be made better. I would appreciate if you could take a look and see what you could suggest improving.

One of my main gripes is that the data returned by the request is then set to a global variable when the promise is resolved.

I can't think of a way to access that data when the buttons are clicked without being able to access it globally?

app.js

const css = require('./app.scss');
const url = './dist/data/shows.json'; 
const model = require ('./model.js');
const view = require ('./view.js');

/* Fetch Data */
model.get(url).then(data => {
    globalData = JSON.parse(data);
  view.renderData();
}).catch(err => {
  view.handleError(err.statusText);
});

view.js

const showList = document.getElementById("shows");

window.btn = (context, x) => {
    handleButtonState(context);
    renderData({key: x.key, value: x.value});
}

function filterObject(filterBy) {
    if(typeof filterBy === 'undefined') return globalData;
        return globalData.filter((item) => {
            return item[filterBy.key] == filterBy.value;
    })
}

function renderData(filterBy) {
    resetState();
    let shows = filterObject(filterBy);
    for (let show of shows) {
    showList.innerHTML += showListStyle(show);
  }
}

function showListStyle(show) {
    return `
        <li class="show_item rounded d-block border">
            <div class="show_date border-right d-block float-left text-uppercase font-weight-bold text-center">
                <span class="d-block title">
                    ${show.date.dayOfMonth}
                </span>
                <span class="text-uppercase">
                    ${show.date.month}
                </span>
            </div>
            <div class="show_primary d-block float-left font-weight-bold">
                <span class="text-uppercase month">${show.date.dayOfWeek}</span>
                <span class="title d-block">${show.location}</span>
            </div>
            <img class="arrow_right" src="dist/img/arrow.svg">
        </li>
    `
}

function resetState() {
    showList.innerHTML = "";
}

function handleButtonState(context) {
    let x =  document.getElementsByClassName("active");
    [...x].forEach(el => el.classList.remove("active"));
    context.classList.add("active");
}

function handleError(status) {
    resetState();
    showList.innerHTML = "We couldn't retrieve your shows :(";
}

module.exports = {renderData, handleError};

model.js

/* Request Data */
function get (url) {
  return new Promise((resolve, reject) => {
    let xhr = new XMLHttpRequest();
    xhr.open('GET', url);

    xhr.onload = function () {
      if (this.status == 200) {
        resolve(xhr.response);
      } else {
        reject({
          status: this.status,
          statusText: xhr.statusText
        });
      }
    };

    xhr.onerror = function () {
      reject({
        status: this.status,
        statusText: xhr.statusText
      });
    };
    xhr.send();
  });
}

module.exports = {get};

index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
    <title></title>
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" integrity="sha384-rwoIResjU2yc3z8GV/NPeZWAv56rSmLldC3R/AZzGRnGxQQKnKkoFVhFQhNUwEyJ" crossorigin="anonymous">
    <link rel="stylesheet" type="text/css" href="dist/app.css">
</head>
<body>

<div class="container">
    <div class="row">
        <div class="col-12">
            <div class="hero_container">
                <div class="hero">
                    <div class="hero_inner">
                        <img id="header_image" src="dist/img/header.jpg">
                        <img id="header_image_logo" src="dist/img/logo.svg">
                        <h2 id="hero_headline">Find a Show</h2>
                    </div>
                </div>
            </div>
        </div>
    </div>

    <div class="row justify-content-md-center">
        <div class="col col-12 col-md-8 col-lg-6">
            <div id="filter_list" class="btn-group btn-group-lg btn-block" role="group">
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" onclick="btn(this, {key: 'genre', value: 'rock'})">Rock</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" onclick="btn(this, {key: 'genre', value: 'folk'})">Folk</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" onclick="btn(this, {key: 'genre', value: 'jazz'})">Jazz</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button active" onclick="btn(this,'all')">All</button>
            </div>
        </div>
        <div class="col col-12 col-md-8 col-lg-6">
            <ul id="shows">
                Loading Shows
            </ul>
        </div>
    </div>
</div>

</body>
<script src="dist/bundle.js"></script>
</html>

json response

[{
  "date": {
    "dayOfWeek": "Thursday",
    "dayOfMonth": 13,
    "month": "Oct"
  },
  "location": "Whitechapel",
  "genre": "rock"
}, {
  "date": {
    "dayOfWeek": "Thursday",
    "dayOfMonth": 13,
    "month": "Oct"
  },
  "location": "Hoxton",
  "genre": "jazz"
}, {
  "date": {
    "dayOfWeek": "Friday",
    "dayOfMonth": 14,
    "month": "Oct"
  },
  "location": "Kensington",
  "genre": "rock"
}, {
  "date": {
    "dayOfWeek": "Saturday",
    "dayOfMonth": 15,
    "month": "Oct"
  },
  "location": "Elephant and Castle",
  "genre": "folk"
}, {
  "date": {
    "dayOfWeek": "Sunday",
    "dayOfMonth": 16,
    "month": "Oct"
  },
  "location": "Southbank",
  "genre": "rock"
}, {
  "date": {
    "dayOfWeek": "Monday",
    "dayOfMonth": 17,
    "month": "Oct"
  },
  "location": "Camden",
  "genre": "jazz"
}, {
  "date": {
    "dayOfWeek": "Tuesday",
    "dayOfMonth": 18,
    "month": "Oct"
  },
  "location": "Hackney",
  "genre": "folk"
}]
\$\endgroup\$
  • \$\begingroup\$ are you using any tools to compile/deploy the files on the server-side, e.g. gulp, grunt, bower, etc.? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 3 '17 at 22:21
  • \$\begingroup\$ @SamOnela good point, should have mentioned I'm using webpack to compile. \$\endgroup\$ – Liondedan Oct 3 '17 at 22:29
2
\$\begingroup\$

Feedback

The code looks decent - I don't see many repetitive DOM lookups and style updates (refer to Stop Writing Slow Javascript for more context). I also like the use of the spread operator and functional programming techniques (e.g. [...x].forEach(el => el.classList.remove("active"));). Though actually I do see that handleButtonState() function does a DOM query each time to set the active class on the clicked item and clear it on the other buttons. I would advise using event delegation to look for clicks on the buttons.

Instead of adding an onclick attribute to each button, use data attributes like in the HTML below. That way the logic to filter items is taken out of the markup.

<button data-key="genre" data-value="rock" type="button" class="btn btn-outline-success btn-block filter_list_button">Rock</button>
<button data-key="genre" data-value="folk" type="button" class="btn btn-outline-success btn-block filter_list_button">Folk</button>
<button data-key="genre" data-value="jazz" type="button" class="btn btn-outline-success btn-block filter_list_button">Jazz</button>
<button type="button" class="btn btn-outline-success btn-block filter_list_button active">All</button>

Then use document.addEventListener() to listen for the DOMContentLoaded event, then listen for clicks.

let filterButtons;
document.addEventListener('DOMContentLoaded', DOMLoaded);

function DOMLoaded() {
    filterButtons = document.getElementsByClassName('filter_list_button');
    document.addEventListener('click', clickHandler);
}
function clickHandler(clickEvent) {
    const target = clickEvent.target;
    if (target.classList.contains('filter_list_button')) {
        handleButtonState(target);
        let filterBy;
        if (target.dataset.key) {
            filterBy = {key: target.dataset.key, value: target.dataset.value};
        }
        renderData(filterBy);
    }
}

That way handleButtonState() doesn't have to do a DOM query each time.

function handleButtonState(context) {
    [...filterButtons].forEach(el => el.classList.remove("active"));
    context.classList.add("active");
} 

MVC and the globalData variable

Considering MVC design patterns, it feels like the view.js file is a controller specifically for the view - A.K.A. a ViewController. Have you worked with any frameworks besides jQuery like Angular, Vue, etc.? I don't see any tags with those framework names in your SO profile so I am guessing not.

It seems like the data returned from the XHR might be best stored on the model object. It could store the data locally, store it after successful XHR load and have a function to get that data:

let data = [];
/* Request Data */
function get (url) {
  return new Promise((resolve, reject) => {
    let xhr = new XMLHttpRequest();
    xhr.open('GET', url);

      xhr.onload = function () {
        if (this.status == 200) {
          data = JSON.parse(xhr.response);
          resolve(data);
    ...
function getData() {
    return data;
}

then the view can request that data from the model:

const model = require('./model.js');
function filterObject(filterBy) {
    if(typeof filterBy === 'undefined') return model.getData();
        return model.getData().filter((item) => {
            return item[filterBy.key] == filterBy.value;
    })
}

A demo can be seen below, with the bundled JS output.

!function(r){function e(n){if(t[n])return t[n].exports;var o=t[n]={i:n,l:!1,exports:{}};return r[n].call(o.exports,o,o.exports,e),o.l=!0,o.exports}var t={};return e.m=r,e.c=t,e.d=function(r,t,n){e.o(r,t)||Object.defineProperty(r,t,{configurable:!1,enumerable:!0,get:n})},e.n=function(r){var t=r&&r.__esModule?function(){return r["default"]}:function(){return r};return e.d(t,"a",t),t},e.o=function(r,e){return Object.prototype.hasOwnProperty.call(r,e)},e.p="",e(e.s=1)}([
/**model.js: */ (function(module, exports) {
/* Request Data */
let data;

function get (url) {
  return new Promise((resolve, reject) => {
      //ajax simulation
      setTimeout(() => {
          data = JSON.parse(getDataFromUrl(url))
          resolve(data)
      }, 1400);
  });
}
function getData() {
    return data;
}

module.exports = {get, getData};
function getDataFromUrl(url) {
    return '[{"date":{"dayOfWeek":"Thursday","dayOfMonth":13,"month":"Oct"},"location":"Whitechapel","genre":"rock"},{"date":{"dayOfWeek":"Thursday","dayOfMonth":13,"month":"Oct"},"location":"Hoxton","genre":"jazz"},{"date":{"dayOfWeek":"Friday","dayOfMonth":14,"month":"Oct"},"location":"Kensington","genre":"rock"},{"date":{"dayOfWeek":"Saturday","dayOfMonth":15,"month":"Oct"},"location":"Elephant and Castle","genre":"folk"},{"date":{"dayOfWeek":"Sunday","dayOfMonth":16,"month":"Oct"},"location":"Southbank","genre":"rock"},{"date":{"dayOfWeek":"Monday","dayOfMonth":17,"month":"Oct"},"location":"Camden","genre":"jazz"},{"date":{"dayOfWeek":"Tuesday","dayOfMonth":18,"month":"Oct"},"location":"Hackney","genre":"folk"}]';
}

/***/ }),
/* app.js */
/***/ (function(module, exports, __webpack_require__) {

//const css = require('./app.scss');
const url = './dist/data/shows.json'; 
const model = __webpack_require__ (0);
const view = __webpack_require__ (2);

/* Fetch Data */
model.get(url).then(data => {
  view.renderData();
}).catch(err => {
  view.handleError(err.statusText);
});

/***/ }),
/* view.js */
/***/ (function(module, exports, __webpack_require__) {

const model = __webpack_require__(0);
const showList = document.getElementById("shows");
let filterButtons;
document.addEventListener('DOMContentLoaded', DOMLoaded);

function DOMLoaded() {
    filterButtons = document.getElementsByClassName('filter_list_button');
    document.addEventListener('click', clickHandler);
}
function clickHandler(clickEvent) {
    const target = clickEvent.target;
    if (target.classList.contains('filter_list_button')) {
        handleButtonState(target);
        let filterBy;
        if (target.dataset.key) {
            filterBy = {key: target.dataset.key, value: target.dataset.value};
        }
        renderData(filterBy);
    }
}

function filterObject(filterBy) {
    if(typeof filterBy === 'undefined') return model.getData();
        return model.getData().filter((item) => {
            return item[filterBy.key] == filterBy.value;
    })
}

function renderData(filterBy) {
    resetState();
    let shows = filterObject(filterBy);
    for (let show of shows) {
    showList.innerHTML += showListStyle(show);
  }
}

function showListStyle(show) {
    return `
        <li class="show_item rounded d-block border">
            <div class="show_date border-right d-block float-left text-uppercase font-weight-bold text-center">
                <span class="d-block title">
                    ${show.date.dayOfMonth}
                </span>
                <span class="text-uppercase">
                    ${show.date.month}
                </span>
            </div>
            <div class="show_primary d-block float-left font-weight-bold">
                <span class="text-uppercase month">${show.date.dayOfWeek}</span>
                <span class="title d-block">${show.location}</span>
            </div>
        </li>
    `
}

function resetState() {
    showList.innerHTML = "";
}

function handleButtonState(context) {
    [...filterButtons].forEach(el => el.classList.remove("active"));
    context.classList.add("active");
}

function handleError(status) {
    resetState();
    showList.innerHTML = "We couldn't retrieve your shows :( " +status;
}

module.exports = {renderData, handleError};

/***/ })
/******/ ]);
<link href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-alpha.6/css/bootstrap.min.css" rel="stylesheet"/>

<div class="container">
    <div class="row">
        <div class="col-12">
            <div class="hero_container">
                <div class="hero">
                    <div class="hero_inner">
                        <!--<img id="header_image" src="dist/img/header.jpg">
                        <img id="header_image_logo" src="dist/img/logo.svg">-->
                        <h2 id="hero_headline">Find a Show</h2>
                    </div>
                </div>
            </div>
        </div>
    </div>

    <div class="row justify-content-md-center">
        <div class="col col-12 col-md-8 col-lg-6">
            <div id="filter_list" class="btn-group btn-group-lg btn-block" role="group">
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" data-key="genre" data-value="rock">Rock</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" data-key="genre" data-value="folk">Folk</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button" data-key="genre" data-value="jazz">Jazz</button>
              <button type="button" class="btn btn-outline-success btn-block filter_list_button active">All</button>
            </div>
        </div>
        <div class="col col-12 col-md-8 col-lg-6">
            <ul id="shows">
                Loading Shows
            </ul>
        </div>
    </div>
</div>

\$\endgroup\$
  • 1
    \$\begingroup\$ Amazing response thank you so much for you effort. You've literally cleared so much up in my head with this explanation! \$\endgroup\$ – Liondedan Oct 4 '17 at 19:11

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.