2
\$\begingroup\$

I have a list of items I pull from SharePoint and want to filter them in my custom list view page. So my function works exactly how I want, but it seems messy and I'm hoping someone could point me to or explain a better way of achieving this or an "optimized" version of what I did.

I'm using a for loop with if to compare the values and display only when true. I have multiple filters and this is where it became tricky to me.

In below code example: incarr array contains the list items. arri is amount of items in list received from SharePoint list:

function displaytable() {                               
    document.getElementById("myHTMLTable").innerHTML = "";
    $("#myHTMLTable").append("<tr align='middle'>" +
  "<th align='left'>"+"<b>Systems Affected</b>"+"</th>" +
  "<th width= 5% align='left'>"+"<b>Incident Date</b>"+"</th>" +                   
    "<th align='left'>"+"<b>Incident Number</b>"+"</th>" +
  "<th align='left'>"+"<b>Root Cause</b>"+"</th>" +
    "<th align='left'>"+"<b>Description</b>"+"</th>" + 
    "<th width= 6% align='left'>"+"<b>Attachments</b>"+"</th>" +                                         
  "</tr>");                 
    var filtery = document.getElementById("dropdownyear").value;
    var filterm = document.getElementById("dropdownmonth").value.toUpperCase();
    var filters = document.getElementById("dropdownsystem").value.toUpperCase();
    var filtersev = document.getElementById("dropdownseverity").value.toUpperCase();                
    for (i=0;i<arri;i++){
        //manual if statements until I can figure our a more effective way                                  
        if (incarr[i][6] == filtery && incarr[i][5] == filterm && incarr[i][0].toUpperCase() == filters && filtersev == incarr[i][7].toUpperCase()){                                                
            $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +  //full filter
        "<td align='left'>"+incarr[i][3]+"</td>" +
        "<td align='left'>"+incarr[i][4]+"</td>" + 
            "<td align='left'>"+incarr[i][8]+"</td>" +
            "</tr>");                       
        } else if (incarr[i][6] == filtery && incarr[i][5] == filterm && filters == "SYSTEM" && filtersev == incarr[i][7].toUpperCase() ) {
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //system filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                                                    
        "</tr>");
        } else if (incarr[i][6] == filtery && filterm == "MONTH" && incarr[i][0].toUpperCase() == filters && filtersev == incarr[i][7].toUpperCase() ){
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //Month filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                           
        "</tr>");
        } else if (incarr[i][6] == filtery && filterm == "MONTH" && filters == "SYSTEM" && filtersev ==  incarr[i][7].toUpperCase() ){
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //Month & System filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                          
        "</tr>");
        } else if (filtery == "YEAR" && filterm == "MONTH" && incarr[i][0].toUpperCase() == filters && filtersev == incarr[i][7].toUpperCase() ){
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //Year & Month filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                           
                "</tr>");
        } else if (incarr[i][6] == filtery && incarr[i][5] == filterm && filters == "SYSTEM" && filtersev == "SEVERITY" ) {
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //system, severity filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                           
        "</tr>");
        } else if (incarr[i][6] == filtery && filterm == "MONTH" && incarr[i][0].toUpperCase() == filters && filtersev == "SEVERITY" ){
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //Month, severity filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                            
        "</tr>");
        } else if (incarr[i][6] == filtery && filterm == "MONTH" && filters == "SYSTEM" && filtersev == "SEVERITY" ){
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +
        "<td align='left'>"+incarr[i][3]+"</td>" +  //Month & System & Severity filter all
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                            
        "</tr>");
        } else if (filtery == incarr[i][6] &&  filterm == incarr[i][5] && incarr[i][0].toUpperCase() == filters && filtersev == "SEVERITY"){                                                                    
                $("#myHTMLTable").append("<tr align='middle'>" +
        "<td align='left'>"+incarr[i][0]+"</td>" +
        "<td align='left'>"+incarr[i][1]+"</td>" +
        "<td align='left'>"+incarr[i][2]+"</td>" +  //Severity filter all
        "<td align='left'>"+incarr[i][3]+"</td>" +
        "<td align='left'>"+incarr[i][4]+"</td>" +
                "<td align='left'>"+incarr[i][8]+"</td>" +                                           
                "</tr>");
        };
    }                           
}

when selecting a filter it must only display items matching that column, and I must be able to select multiple filters to filter the filtered list (if this makes sense?)

\$\endgroup\$
0
\$\begingroup\$

From a short review;

  • filtery, filterm etc. are unfortunate names, just go for filterYear, filterMonth, etc. It would make the code so much more readable and parseable. At the very least it should follow lowerCamelCase -> filterY and filterM
  • This piece of code has been copy pasted a number times, a mortal sin in code review. Please have a function that can be called.

        $("#myHTMLTable").append("<tr align='middle'>" +
          "<td align='left'>"+incarr[i][0]+"</td>" +
          "<td align='left'>"+incarr[i][1]+"</td>" +
          "<td align='left'>"+incarr[i][2]+"</td>" +  //full filter
          "<td align='left'>"+incarr[i][3]+"</td>" +
          "<td align='left'>"+incarr[i][4]+"</td>" + 
          "<td align='left'>"+incarr[i][8]+"</td>" +
         "</tr>");  
    
  • Your code seems to assume that incarr[i][6] will always contain the year. If you had a yearColumn constant (with the value of 6), you would just access incarr[i][yearColumn]. If one day the year is another column, then you only need to update the yearColumn constant. Look up 'Dont Repeat Yourself'
  • Your code has about 8 lines of comment, of which 6 are the same copy pasted ones
  • Variables like "MONTH" should be constants defined up top
  • If you are using jQuery, then you might as well use more of the feature's, check out
  • Accessing incarr[i] so many times is hard on the reader, and also is a bit taxing on the JavaScript engine. Why not assign that to a variable in the beginning of the loop?

All in all, that would lead me to an (obviously untested) alternative that looks like this:

function displaytable() {                               
    $("myHTMLTable").html("<tr align='middle'>" +
  "<th align='left'>"+"<b>Systems Affected</b>"+"</th>" +
  "<th width= 5% align='left'>"+"<b>Incident Date</b>"+"</th>" +                   
    "<th align='left'>"+"<b>Incident Number</b>"+"</th>" +
  "<th align='left'>"+"<b>Root Cause</b>"+"</th>" +
    "<th align='left'>"+"<b>Description</b>"+"</th>" + 
    "<th width= 6% align='left'>"+"<b>Attachments</b>"+"</th>" +                                         
  "</tr>");                 

    var filterYear = $("#dropdownyear").val();
    var filterMonth = $("#dropdownmonth").val().toUpperCase();
    var filterSystem = $("#dropdownsystem").val().toUpperCase();
    var filterSeverity = $("#dropdownseverity").val().toUpperCase();                
    var yearColumn = 6;
    var monthColumn = 5;
    var systemColumn = 0;
    var severityColumn = 7;

    for (i = 0; i < arri; i++){
      var incident = incarr[i];

      if(filterYear != "YEAR" && incident[yearColumn] != filterYear)
        continue;

      if(filterMonth != "MONTH" && incident[monthColumn] != filterMonth)
        continue;

      if(filterSystem != "SYSTEM" && incident[systemColumn] != filterSystem)
        continue;

      if(filterSeverity != "SEVERITY" && incident[severityColumn] != filterSeverity)
        continue;

      $("#myHTMLTable").append("<tr align='middle'>" +
                "<td align='left'>" + incident[0] + "</td>" +
                "<td align='left'>" + incident[1] + "</td>" +
                "<td align='left'>" + incident[2] + "</td>" +  //full filter
                "<td align='left'>" + incident[3] + "</td>" +
                "<td align='left'>" + incident[4] + "</td>" + 
            "<td align='left'>" + incident[8] + "</td>" +
            "</tr>");                       
    }                           
}
\$\endgroup\$
  • \$\begingroup\$ this is exactly the type of answer I where looking for, thanks for the suggestions Im going to test this out now and give feedback. sorry for the bad coding im self taught and still learning new concepts every day. \$\endgroup\$ – SqueakyDeath Jul 16 at 6:04
  • \$\begingroup\$ thanks konijn i tested the code and its working perfectly. do you have a article i can read regarding the lowerCamelCase you suggested and when to use it? for example should it only be used for variables of specific type or is it for functions as well? i also read an article about using while loop instead of using for loop to improve performance do you know if this helps at all and if it will make much of a difference? \$\endgroup\$ – SqueakyDeath Jul 16 at 7:06
  • \$\begingroup\$ This is my favourite coding standard: github.com/airbnb/javascript It has all that covered. \$\endgroup\$ – konijn Jul 16 at 12:44
0
\$\begingroup\$

One different approach could be to have the filtering be just toggling a CSS class so you can build the DOM once, and then just selectively show (/hide) rows. Speaking of CSS, all those inline style definitions should definitely go to a stylesheet.

Multiple filters on the other hand are just ANDing them together.

// a couple of helpers to shorten code
const elem = (type, attrs = {}) =>
  Object.assign(document.createElement.bind(document)(type), attrs)
const append = (parent, child) => (parent.appendChild(child), parent)

// NOTE adapt to your date format
const yearPart = date => date.slice(0, 4)
const monthPart = date => date.slice(5, 7)

document.addEventListener('DOMContentLoaded', () => {
  const incidents = document.getElementById('incidents')
  const filters = document.forms.filters

  // reduce the data into <select> <option>s and <tbody>
  const {options, tbody} = data.reduce((acc, ary) => {
    let [system, date] = ary
    let year = yearPart(date)
    let month = monthPart(date)

    append(acc.tbody, ary.reduce((tr, datum) =>
      append(tr, elem('td', {textContent: datum}))
    , elem('tr')))

    acc.options.system.add(system)
    acc.options.year.add(year)
    acc.options.month.add(month)

    return acc
  }, {
    options: { system: new Set(), year: new Set(), month: new Set() }
    , tbody: elem('tbody')
  })

  // populate the <select>s
  Object.entries(options).forEach(([key, values]) =>
    Array.from(values).sort().reduce((select, option) =>
      append(select, elem('option', {textContent: option}))
    , filters[key]))

  // populate the <table>
  append(incidents, tbody)

  // or just disregard the above if the <table> and <select>s are
  // pre-populated.
  // actual filtering
  filters.addEventListener('change', evt => {
    const rows = incidents.querySelectorAll('tbody tr')

    rows.forEach(row => row.classList.remove('hidden'))

    // NOTE that this expects the order of the <select>s to match
    // the order of the fields (with the exception of date)
    Array.prototype.filter.call(rows, ({cells}) =>
      !Array.from(document.forms.filters).reduce((bool, {value}, i) => {
        if(!value)
          return bool && true

        switch(i) {
          case 1: return bool && value == yearPart(cells[1].textContent)
          case 2: return bool && value == monthPart(cells[1].textContent)
          default: return bool && value == cells[i].textContent
        }
      }, true)
    ).forEach(row => row.classList.add('hidden'))
  })
})
table { width: 100%; border-collapse: collapse; table-layout: fixed }
td:nth-child(5) { width: 20%; overflow: hidden; white-space: nowrap }
th { text-align: left }
th, td { padding: .5ex; vertical-align: baseline }
.hidden { display: none }
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
  </head>

  <body>
    <form id="filters">
      <label><span>system</span>
        <select id="system">
          <option>
        </select>
      </label>

      <label><span>year</span>
        <select id="year">
          <option>
        </select>
      </label>

      <label><span>month</span>
        <select id="month">
          <option>
        </select>
      </label>

    </form>

    <table id="incidents">
      <thead>
        <tr>
          <th>Systems affected</td>
          <th>Incident date</td>
          <th>Incident number</th>
          <th>Root cause</th>
          <th>Description</th>
          <th>Attachments</th>
        </tr>
      </thead>

    </table>
  </body>
</html>

\$\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.