Blog-Archiv

Dienstag, 21. Juni 2016

Refactoring JS List Filters, Part One

This is the first follower Blog of "JS List Filter Checkboxes" where I will refactor the JavaScript code of that page. In a first step I will try to ....

  • introduce separation of concerns
  • improve lacking reusability
  • fix any bad naming
  • get rid of any error-prone implementations
  • split any big function into smaller ones.

Reuse String Functions

String functions are first-class candidates for reuse, in every programming language. We should have simple and small functions that do just one thing, not having side-effects.

Following is the old function that I want to improve: 

  var toArrayWithoutEmpty = function(csv, splitChar) {
    var rawSearchPatterns = csv.split(splitChar);
    var searchPatterns = [];

    for (var i = 0; i < rawSearchPatterns.length; i++) {
      var searchPattern = rawSearchPatterns[i].trim();

      if (searchPattern !== '') {
        var lastChar = searchPattern.charAt(searchPattern.length - 1);
        if (lastChar === '!' || lastChar === '?' || lastChar === '.' || lastChar === ',')
          searchPattern = searchPattern.substring(0, searchPattern.length - 1);

        searchPatterns.push(searchPattern);
      }
    }
    return searchPatterns;
  };

What I don't like here:

  • parameter csv is named wrongly, because it is not always used for comma-separated values; don't underestimate wrong names, it makes source as unreadable as abbreviations do
  • the if (lastChar == ...) condition is too complex, and we want to maintain these characters more easily
  • removing the last character is reprogrammed here, but should be done by reusing a string-library function
  • error: a punctuation-character between two separator-characters would be pushed as empty string here, which we want to avoid.
    var PUNCTUATION_CHARACTERS = ',:?!.;';

    var isPunctuation = function(character) {
      for (var i = 0; i < PUNCTUATION_CHARACTERS.length; i++)
        if (PUNCTUATION_CHARACTERS.charAt(i) === character)
          return true;
      return false;
    };

    var lastChar = function(string) {
      return string.charAt(string.length - 1);
    };

    var removeLastChar = function(string) {
      return string.substring(0, string.length - 1);
    };

    var toArrayWithoutEmpty = function(toSplit, splitCharacter) {
      var rawWords = toSplit.split(splitCharacter);
      var words = [];

      for (var i = 0; i < rawWords.length; i++) {
        var rawWord = rawWords[i].trim();

        while (rawWord.length > 0 && isPunctuation(lastChar(rawWord)))
          rawWord = removeLastChar(rawWord);

        rawWord = rawWord.trim();

        if (rawWord.length > 0)
          words.push(rawWord.toUpperCase());
      }
      return words;
    };

Most likely you can dismiss the first three functions and reuse the string-library of your project instead. When not, suggest your team to start a string-library and put these functions there for reuse.

The obscure csv parameter has been renamed to what it really is: the text to split.

The code has become better readable, even when there are more lines of code now. You can easily verify that these functions will work correctly, which was not the case before.

Split Big Functions

Big functions is a very frequent (and bad) programmer habit. It is as widespread as copy & paste coding. Digging into the problem while typing, and solving the problem by typing more and more lines, without considering already existing solutions, without breaking the problem down into smaller parts, without solving it by simple and reusable solutions - that's generating big maintenance efforts.

Here is the big function to split: 

  var filter = function() {
    var table = document.getElementsByTagName('table')[0];
    var checkboxes = document.getElementsByTagName('input');
    
    var allSearchWords = '';
    var searchWords = '';
    var othersIsChecked = false;
    
    for (var i = 0; i < checkboxes.length; i++) {
      var checkbox = checkboxes[i];
      var thisSearchWords = checkbox.getAttribute('searchWords');
      if (thisSearchWords) {
        allSearchWords += ', '+thisSearchWords;
        if (checkbox.checked)
          searchWords += ', '+thisSearchWords;
      }
      else if (checkbox.checked && checkbox.getAttribute('id') === 'Others')
        othersIsChecked = true;
    }
    
    var searchPatterns = toArrayWithoutEmpty(searchWords, ',');
    var allSearchPatterns = toArrayWithoutEmpty(allSearchWords, ',');
    
    var tbody = table.children[0];
    for (var row = 0; row < tbody.children.length; row++) {
      var tableRow = tbody.children[row];
      var label = tableRow.children[1].children[0].childNodes[0];
      var labelText = label.textContent.trim();
      var labelWords = toArrayWithoutEmpty(labelText, ' ');
      var match = false;
      
      if (searchPatterns.length > 0 || othersIsChecked && allSearchPatterns.length > 0) {
        if (othersIsChecked)
          match = ! matches(allSearchPatterns, labelText, labelWords);
          
        match = match || matches(searchPatterns, labelText, labelWords);
      }
      else
        match = true;
      
      tableRow.style.display = match ? '' : 'none';
    }
  };

This takes too much time to read and understand. We want to improve that.

Extract DOM Access

Keep DOM access apart from the functions that process the elements (resulting from DOM access). In my hacky script there are three DOM-access places: querying the ...

  • table rows to filter
  • text of a table row
  • filter checkboxes

For these I would suggest following code: 

    var getRowsToFilter = function() {
      var table = document.getElementsByTagName('table')[0];
      var tbody = table.children[0];
      return tbody.children;
    };

    var getTextFromRow = function(row) {
      return row.children[1].children[0].childNodes[0];
    };

    /* Initialization */
    
    var searchCheckboxes = document.body.querySelectorAll('input[type = checkbox]');

    for (var i = 0; i < searchCheckboxes.length; i++)
      searchCheckboxes[i].addEventListener('change', filter);

Here I provide two functions that (1) retrieve the table-rows from the HTML document as getRowsToFilter(), and (2) get the text from a row as getTextFromRow(row). Later, when turning this into a module, both functions could be provided overrideable, for web-pages having a different environment.

Finally I query all checkboxes that serve as filters, and store them into a global array. In a loop over that array I install change-listeners, so that the according onclick HTML-attribute on each checkbox-element has become dispensable.

One Function, One Purpose

As we see, the filter() function serves two purposes:

  • extracting search-words from checkboxes
  • searching theses words in table row texts

So I extract the retrieval of the search-words as new function, which I will name getSearchConditions(). Mind that the searchCheckboxes array is a global variable. 

    var SEARCH_WORDS_ATTRIBUTE = 'search-words';
    var OTHERS_ID = 'others-checkbox';

    var getSearchConditions = function() {
      var allSearchWords = '';
      var searchWords = '';
      var revertResult = false;

      for (var i = 0; i < searchCheckboxes.length; i++) {
        var checkbox = searchCheckboxes[i];
        var thisSearchWords = checkbox.getAttribute(SEARCH_WORDS_ATTRIBUTE);

        if (thisSearchWords) {
          allSearchWords += ', '+thisSearchWords;

          if (checkbox.checked)
            searchWords += ', '+thisSearchWords;
        }
        else if (checkbox.checked && OTHERS_ID !== undefined) {
          if (checkbox.getAttribute('id') === OTHERS_ID)
            revertResult = true;
        }
      }

      return {
        allSearchWords: allSearchWords,
        searchWords: searchWords,
        revertResult: revertResult
      };
    };

The getSearchConditions() function loops the checkboxes and builds together two comma-separated string-collections. The allSearchWords serves as negative filter for the "Others" checkbox, the searchWords serves as positive filter, containing just the "checked" search-terms.

This function returns an object containing all information needed from the checkboxes. The caller will have to know about the nature of that object. In Java you would need an inner class for that return, in JS it is written by just a few lines.

Here comes the new implementation of filter(). 

    var filter = function() {
      var searchConditions = getSearchConditions();
      var searchPatterns = toArrayWithoutEmpty(searchConditions.searchWords, ',');
      var allSearchPatterns = toArrayWithoutEmpty(searchConditions.allSearchWords, ',');
      var revertResult = searchConditions.revertResult;

      var rows = getRowsToFilter();
      for (var i = 0; i < rows.length; i++) {
        var row = rows[i];

        var label = getTextFromRow(row);
        var labelText = label.textContent.trim().toUpperCase();
        var labelWords = toArrayWithoutEmpty(labelText, ' ');
        var match = false;

        if (searchPatterns.length > 0 || revertResult && allSearchPatterns.length > 0) {
          if (revertResult)
            match = ! matches(allSearchPatterns, labelText, labelWords);

          match = match || matches(searchPatterns, labelText, labelWords);
        }
        else {
          match = true;
        }

        row.style.display = match ? '' : 'none';
      }
    };

The filter() function does what it did before, but it performs only the filtering loop now. All other work it delegates to getSearchConditions(), toArrayWithoutEmpty(), getRowsToFilter() and getTextFromRow().

This function now is focused on the filtering-logic only. We could also extract the loop, but essentially that would not give any reusable code, or be valuable for overriding. Maybe it would make sense to extract the CSS-action row.style.display = .... for using other ways of hiding table rows.

Here is the complete refactored source code (click left-side arrow to unfold).

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
    var SEARCH_WORDS_ATTRIBUTE = 'search-words';
    var OTHERS_ID = 'others-checkbox';
    var PUNCTUATION_CHARACTERS = ',:?!.;';

    var isPunctuation = function(character) {
      for (var i = 0; i < PUNCTUATION_CHARACTERS.length; i++)
        if (PUNCTUATION_CHARACTERS.charAt(i) === character)
          return true;
      return false;
    };

    var lastChar = function(string) {
      return string.charAt(string.length - 1);
    };

    var removeLastChar = function(string) {
      return string.substring(0, string.length - 1);
    };

    var toArrayWithoutEmpty = function(toSplit, splitChar) {
      var rawWords = toSplit.split(splitChar);
      var words = [];

      for (var i = 0; i < rawWords.length; i++) {
        var rawWord = rawWords[i].trim();

        while (rawWord.length > 0 && isPunctuation(lastChar(rawWord)))
          rawWord = removeLastChar(rawWord);

        rawWord = rawWord.trim();

        if (rawWord.length > 0)
          words.push(rawWord.toUpperCase());
      }
      return words;
    };

    var matches = function(searchWords, labelText, labelWords) {
      for (var i = 0; i < searchWords.length; i++) {
        var searchWord = searchWords[i];

        if (searchWord.indexOf(' ') > 0) { /* when searchWord contains a space, search in text */
          if (labelText.indexOf(searchWord) >= 0)
            return true;
        }
        else if (labelWords.indexOf(searchWord) >= 0) { /* else search in word-array */
          return true;
        }
      }
      return false;
    };

    var getSearchConditions = function(checkboxes) {
      var allSearchWords = '';
      var searchWords = '';
      var revertResult = false;

      for (var i = 0; i < checkboxes.length; i++) {
        var checkbox = checkboxes[i];
        var thisSearchWords = checkbox.getAttribute(SEARCH_WORDS_ATTRIBUTE);

        if (thisSearchWords) {
          allSearchWords += ', '+thisSearchWords;

          if (checkbox.checked)
            searchWords += ', '+thisSearchWords;
        }
        else if (checkbox.checked && OTHERS_ID !== undefined) {
          if (checkbox.getAttribute('id') === OTHERS_ID)
            revertResult = true;
        }
      }

      return {
        allSearchWords: allSearchWords,
        searchWords: searchWords,
        revertResult: revertResult
      };
    };

    var filter = function() {
      var searchConditions = getSearchConditions(searchCheckboxes);
      var searchPatterns = toArrayWithoutEmpty(searchConditions.searchWords, ',');
      var allSearchPatterns = toArrayWithoutEmpty(searchConditions.allSearchWords, ',');
      var revertResult = searchConditions.revertResult;

      var rows = getRowsToFilter();
      for (var i = 0; i < rows.length; i++) {
        var row = rows[i];

        var label = getTextFromRow(row);
        var labelText = label.textContent.trim().toUpperCase();
        var labelWords = toArrayWithoutEmpty(labelText, ' ');
        var match = false;

        if (searchPatterns.length > 0 || revertResult && allSearchPatterns.length > 0) {
          if (revertResult)
            match = ! matches(allSearchPatterns, labelText, labelWords);

          match = match || matches(searchPatterns, labelText, labelWords);
        }
        else {
          match = true;
        }

        row.style.display = match ? '' : 'none';
      }
    };
    
    var getRowsToFilter = function() {
      var table = document.getElementsByTagName('table')[0];
      var tbody = table.children[0];
      return tbody.children;
    };

    var getTextFromRow = function(row) {
      return row.children[1].children[0].childNodes[0];
    };

    /* Initialization */
    
    var searchCheckboxes = document.body.querySelectorAll('input[type = checkbox]');

    for (var i = 0; i < searchCheckboxes.length; i++)
      searchCheckboxes[i].addEventListener('change', filter);

Did I achieve my goals?

  • introduce separation of concerns: separate DOM access from string operations, reading input fields from filtering table rows
  • improve lacking reusability: reusable string operations have been implemented
  • fix any bad naming: names of local variables and parameters have been improved
  • get rid of any error-prone implementations: fixed the extraction of search-words and row texts
  • split any big function into smaller ones: filter() and toArrayWithoutEmpty() have been split.

There is some more work to do, pending for my next Blog:

  • encapsulation into a reusable module.



Keine Kommentare:

Kommentar veröffentlichen

Abonnieren Kommentare zum Post (Atom)