Refactor to Reduce Repetitive Code

One of the principles of software development I've been taught is to keep your code DRY. DRY stands for "Don't Repeat Yourself", and the idea is to minimize repetitive lines of code.

The Clunky Code

In my previous post, we added a feature to our bug tracking app to filter our list of issues by priority level. Here is what I coded to get this initially working:

document.getElementById('all').addEventListener('click', e => {
  renderCurrentData();
})

document.getElementById('low').addEventListener('click', e => {
  document.getElementById('issuesList').innerHTML = "";
  readFilteredIssues(e);
})

document.getElementById('medium').addEventListener('click', e => {
  document.getElementById('issuesList').innerHTML = "";
  readFilteredIssues(e);
})

document.getElementById('high').addEventListener('click', e => {
  document.getElementById('issuesList').innerHTML = "";
  readFilteredIssues(e);
})

function readFilteredIssues(e) {    

 issuesRef.orderByChild('priority').equalTo(e.target.innerHTML).on("value", function(snapshot) {
    snapshot.forEach(snap => {
      const issue = snap.val();

      document.getElementById("issuesList").innerHTML +=
      //  HTML code for each issue element
    })
  }
)}

As you can see, this is extremely repetitive! Whenever you notice repeated lines of code, you'll know that it's time to do some refactoring, which is what we'll do here now.

The Cleaner Code

The first thing I notice here is that we are firing off the same function with the same click event. The only difference is the priority level (or filter option) that is being passed into the readFilteredIssues() function. We will want to query the div that holds all the buttons for filtering options, grab the value of the innerHTML, and pass that through to readFilteredIssues():

const filterButtons = document.querySelector('.filterButtons');

filterButtons.addEventListener('click', filterIssues);

function filterIssues(e) {
  document.getElementById('issuesList').innerHTML = "";
  const priorityLevel = e.target.innerHTML;
  readFilteredIssues(priorityLevel);
}

That alone has already reduced our lines of code significantly! Now we just need to make minor changes to the readFilteredIssues() function.

First, instead of passing e.targetinnerHTML to the equalTo Firebase method, since we're already getting the innerHTML from the previous filterIssues() function, we just need to pass in the same argument being passed into the function itself (which can be named anything). For our purposes, I will rename it to filterOption, just because it would make a little more sense.

The other change we will make is to add a conditional statement for the filter option of "All", which isn't a priority level. This is just an option to show all issues in our database. Before doing anything else in this function, we will check to see if the parameter passed in is equal to "All", and if it is then we will just render all data (which is a function we already have). Otherwise, we will get the filtered data to render.

The code will look like this:

function readFilteredIssues(filterOption) { 
  if (filterOption === "All") renderCurrentData();

 issuesRef.orderByChild('priority').equalTo(filterOption).on("value", function(snapshot) {
    snapshot.forEach(snap => {
      const issue = snap.val();

      document.getElementById("issuesList").innerHTML +=
      //  HTML code for each issue element
    })
  }
)}

And that's it! Our filtering function is working just as it was before, and we aren't repeating lines of code unnecessarily.