2
\$\begingroup\$

I created a graph to show the change in the percentage of questions answered over time and the first version fetched data from chat transcripts using PHP. The sub-domain I have registered for my webspace doesn't offer HTTPS so the calls to file_get_contents() would fail when run there. Because of this, I started looking at alternatives.

I looked at using other scripting languages and found that github offered github pages and noted that it was secure but didn't allow any server-side scripting. So tried using client-side JavaScript with the fetch API. Initially there were CORS issues and after a little research I found this post, which mentioned a Heroku page that would act as a proxy. I used the proxy to get the data, and I have been interested in the new ES-8 async functions and the await operator so I utilized those.

How does the code look? What would you change about it?

Can you help make the chart results go up by providing a good answer? And remember to vote on any answer that is helpful.

const url = 'https://chat.stackexchange.com/search?q=%22RELOAD!+There+are%22&user=125580&room=8595&pagesize=150';
const pUrl = 'https://cors-escape.herokuapp.com/' + url;
const chartOptions = {
  title: {
    display: true,
    text: 'Answered percentage in recent months'
  },
  scales: {
    xAxes: [{
      display: true,
      scaleLabel: {
        display: true,
        labelString: 'Date'
      }
    }],
    yAxes: [{
      display: true,
      scaleLabel: {
        display: true,
        labelString: 'Percentage'
      }
    }]
  }
};
const dataset = {
  label: 'Answered Percentage',
  backgroundColor: window.chartColors.red,
  borderColor: window.chartColors.red,
  fill: false
};

async function getGraphData() {
  const loadingImage = document.getElementById('loading');
  try {
    const response = await fetch(pUrl);
    const container = document.createElement('div');
    container.innerHTML = await response.text();
    const timestamps = container.getElementsByClassName('timestamp');
    const contents = container.getElementsByClassName('content');

    const labels = [],
      data = [];
    if (contents.length === timestamps.length) {
      let i = 0;
      for (contentElement of contents) {
        const matches = contentElement.innerHTML.match(/\d{2}.\d{4}/);
        if (matches.length) {
          data.unshift(matches[0]);
          labels.unshift(timestamps[i++].innerHTML);
        }
      }
      if (data.length) {
        loadingImage.style.display = 'none';
        drawChart(data, labels);
      }
    }
  } catch (err) {
    loadingImage.parentNode.innerHTML = 'Error loading data';
  }
}

document.addEventListener('DOMContentLoaded', getGraphData)
const drawChart = (data, labels) => {
  Chart.defaults.global.legend.display = false;
  const config = {
    type: 'line',
    data: {
      labels,
      datasets: [Object.assign(dataset, {data})]
    },
    options: chartOptions
  };
  const ctx = document.getElementById('canvas').getContext('2d');
  window.myLine = new Chart(ctx, config);
};
h1 {
  font-family: Arial, "Helvetica Neue", Helvetica, sans-serif;
}
<script src="//www.chartjs.org/dist/2.7.2/Chart.bundle.js"></script>
<script src="//www.chartjs.org/samples/latest/utils.js"></script>
<h1>
  Graph of Answered Question Percentage on Code Review SE
</h1>
<div style="width:75%;">
  <canvas id="canvas"></canvas>
  <img src="https://loading.io/spinners/bars/index.progress-bar-facebook-loader.gif" id="loading" />
</div>

\$\endgroup\$
1
\$\begingroup\$

An error and a logic problem.

The code would fail if you ran it as a module or in strict mode due to undefined variable. There is also a problem when parsing the HTML, if there is missing data the timestamps and the percentages will go out of sync because you fail to increase i.

Async functions handle errors by assigning them to the returned promise catch callback. You have wrapped a try catch around code that may throw unrelated development error which you would see as "Error loading data" and not differentiate a legit error from source/logic error.

It is unclear what you intend to happen when fetched data is bad (eg 404 page) asd this will not throw error and you ignore such errors without giving any feedback.

A clearer separation of roles with fetch and parsing as functions rather than the single async function.

Data is also all over the place with the graph settings declared at the top and then more in the function that creates the graph.

General points

  • You use an undeclared variable contentElement in getGraphData
  • Why the if (matches.length) { Will that ever happen??? And if it does doesn't that mean that i as index to timeStamps no longer matches the the for loop position?

  • window is the default object and not needed for direct references. If you are defining globals, clearly define them in the global scope. If they are external use comments to indicate that they external globals.

  • Why assign to a variable that is never used? window.myLine = new Chart(

  • Keep it DRY. Axis object could be a function, also several other parts can be reduced using functions.

  • Reduce number of lines of source code by avoiding single use variable declarations.
  • Avoid using innerHTML when possible as it is much slower than textContent.
  • Use direct element id referencing when you are in control of content.
  • Only wrap the relevant code in try catch. ECMAScript 2018 lets you drop the error object after the catch token. Better yet separate the async part of the function and use promise catch function.
  • Use the shortest form when possible. Eg you used datasets: [Object.assign(dataset, {data})] that can be datasets: [{...dataset, data}]

  • Why the if (contents.length === timestamps.length) and no warning if the data is bad?.

  • Double/same line open [{, ({, or [[... etc should be followed by single indent and matching double close. See rewrite.

  • Redundant repeated X axis label content " 12:00 AM" can be removed

  • Graph heading and page heading are different. Maybe assign the graph heading from the page heading??

Possible rewrite

This rewrite reduces the number of lines and encapsulates most of the requirements within a single function reducing the global namespace pollution.

Data vetting was a little unclear so just added that to the graphing function to show a warning if no data found.

"use strict";
// uses chartColors defined in ???.js

document.addEventListener("DOMContentLoaded", displayAnswerRate);

function displayAnswerRate() {
    const CORS_PROXY = "https://cors-escape.herokuapp.com/"; 
    const url = "https://chat.stackexchange.com/search?q=%22RELOAD!+There+are%22&user=125580&room=8595&pagesize=150";
       
    const showError = message => {
        errorEl.style.display = "block";
        errorEl.textContent = message;
    }
    getDataURL(url)
        .then(parseDataAndChart)
        .catch(() => {
            loading.style.display = "none";
            showError("Error loading data");
        });

    async function getDataURL(url) { return (await fetch(CORS_PROXY + url)).text() }    

    function parseDataAndChart(markup){        
        const elsByClass = cName => [...html.querySelectorAll("." + cName)].reverse();
        const html = document.createElement('div');
        html.innerHTML = markup;
        drawChart(
            elsByClass("content").map(el => el.textContent.split("(")[1].split("%")[0]),
            elsByClass("timestamp").map(el => el.textContent.replace(" 12:00 AM",""))
        );
        loading.style.display = "none";
    }

    function drawChart(data, labels) {
        const axis = name => [{display: true, scaleLabel: {display: true, labelString: name}}];      
        if (data.length > 0) {
            Chart.defaults.global.legend.display = false;
            new Chart(canvas.getContext('2d'), {
                type: 'line',
                data: {
                    labels,
                    datasets: [{
                        label: 'Answered Percentage',
                        backgroundColor: chartColors.red,
                        borderColor: chartColors.red,
                        data,
                        fill: false
                    }]
                },
                options: {
                    title: {display: true, text: heading.textContent},
                    scales: { xAxes: axis("Date"), yAxes: axis("Percentage") }
                }
            });
        } else {
            showError("No data found???");
        }
    }    
}
body {
  font-family: Arial, "Helvetica Neue", Helvetica, sans-serif;
}
h2 {
  text-align : center;
}
#errorEl { 
  display : none;
  color: red;
}
<script src="//www.chartjs.org/dist/2.7.2/Chart.bundle.js"></script>
<script src="//www.chartjs.org/samples/latest/utils.js"></script>

<div style="width:75%;">
  <h2 id="heading">Answered percentage in recent months</h2>
  <div id="errorEl"></div>
  <canvas id="canvas"></canvas>
  
  <img src="https://loading.io/spinners/bars/index.progress-bar-facebook-loader.gif" id="loading" />
</div>

\$\endgroup\$

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.