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:

Sonntag, 12. Juni 2016

JS List Filter Checkboxes

One of the basic needs of a user is to filter information. From a table-of-contents of hundreds of articles I want to see just those that contain information about, let's say, JavaScript. I assume the article's title contains enough information to decide whether it's about JavaScript, so that filtering can happen on the client-side browser without contacting the server.

Mostly filtering is provided by one or more text-fields where the user can input a search-pattern. But using the keyboard is getting tedious nowadays when being on a mobile phone. Patterns might be even misunderstood (wildcards), moreover most words are not precise without context. It could also happen that the user inputs "JavaScript", but the title of the searched article contains the synonym "JS".

To make filtering easy and quick I came up with a set of checkboxes that describe most of the articles' contents. That means the search-patterns have been pre-defined, and the user chooses from a restricted set of filterings. This moves the responsibility to define search-terms to the page-author.

Example

The following list contains links to articles about JavaScript, CSS, HTML, Java and LINUX. If a title contains "JS", "JavaScript" or "jQuery", it is expected to be about JavaScript, "HTML" and "Page" lead to HTML, "CSS" to CSS, and so on. Should a title contain both "JavaScript" and "CSS", it would be in both search results. Should we want to see none of these categories, we can click the "Others" checkbox, this excludes all the mentioned search terms, and displays just those titles that do not refer to any of them.

Blog Archive Contents

104 The JS Function-in-Loop Bug 2016-06-07
103 JS jQuery $(this) 2016-06-06
102 LINUX Terminal ls colors 2016-06-06
101 HTML Elements and Dimensions 2016-06-04
100 JS Browser Reflow and Repaint 2016-05-29
99 JS Semicolons 2016-05-21
98 CSS Width 100% and Position Absolute or Fixed 2016-05-15
97 JS Map Key Gotcha 2016-04-30
96 JS Revealing Module Pattern 2016-04-24
95 What's HTML label for? 2016-04-18
94 Three Steps Toward Progress 2016-04-13
93 JS Get / Set Element Width / Height 2016-04-11
92 JS / CSS Tabcordion 2016-04-03
91 JS Responsive Breakpoint Notifications 2016-03-28
90 JS / CSS Tabs 2016-03-13
89 JS Table Layout Adjustment: DIV Tables 2016-03-06
88 JS Table Layout Adjustment: Predefined Widths 2016-02-29
87 JS Element Dimensions 2016-02-21
86 JS Table Layout Adjustment: Elastic Column 2016-02-14
85 JS Table Layout Adjustment: Sizing 2016-02-07
84 JS Table Layout Adjustment: Naming 2016-02-06
83 JS clientWidth and CSS width 2016-01-25
82 Space in HTML Breaks Layout 2016-01-19
81 JS Titled Border 2016-01-18
80 HTML Input Focus Tab Order 2016-01-16
79 JS Keyboard Events 2016-01-13
78 JS Sticky Bar 2016-01-07
77 Pure CSS Push Menu 2015-12-30
76 Replacement for CSS fixed position 2015-12-29
75 Pure CSS Slide Menu 2015-12-27
74 Protruding CSS inline elements 2015-12-26
73 Receiving CSS Events 2015-12-19
72 CSS BorderLayout, yet another one 2015-12-14
71 CSS height 100% shows Browser Scrollbar 2015-12-13
70 JS Browser Coordinates 2015-12-07
69 JS Sticky Table-of-Contents 2015-12-03
68 CSS Layout Test Page 2015-11-30
67 Text Outline with GIMP 2015-11-14
66 Iterator in Java and JS 2015-10-14
65 JS Natural Sort Order 2015-10-13
64 Natural Sort in Java, Performance Tuned 2015-10-11
63 Natural Sort in Java, First Performance Tuning 2015-10-07
62 Natural Sort Order in Java 2015-10-05
61 JS Light Bulb Moments 2015-10-03
60 JS Poor Developer's IDE 2015-09-19
59 Visitor Pattern for Test Data in Java 2015-09-17
58 Three Notorious Software Developer Habits 2015-09-09
57 Videotized on LINUX 2015-08-16
56 JS Visibility Detection 2015-08-14
55 LINUX Root Password Confusion 2015-08-01
54 CSS Fixed Table Header 2015-07-20
53 CSS Position Property 2015-07-19
52 JS Animated Expand and Collapse 2015-07-12
51 Pure CSS Menu 2015-07-11
50 JS Asynchronous Waiting 2015-06-20
49 JS Swipe Gesture 2015-06-11
48 The Immortal AWK Language 2015-05-27
47 JS Document Treeification 2015-05-15
46 Adjust Screen Coordinates in LINUX with Xfce 2015-05-09
45 UNIX Shell Control Structures 2015-05-01
44 vi Manual 2015-04-29
43 JS Table of Contents 2015-04-16
42 JS Overrides 2015-04-07
41 Extract Google Blog Export using Java 2015-04-04
40 JS Treetable 2015-03-31
39 Remote Desktop from LINUX to WINDOWS 2015-03-25
38 JS Slide Show Aftermath 2015-03-24
37 JS Slide Show 2015-03-14
36 Yet Another JavaScript AMD Loader 2015-02-28
35 JS Requires Dependency Management 2015-02-21
34 Interrupted LINUX Upgrade to Ubuntu 14.04 2015-02-13
33 JS Folding 2015-02-10
32 Many LINUX on board 2015-01-24
31 Installing LINUX on a DELL laptop besides WINDOWS 8.1 2015-01-12
30 Installing LINUX without CD or USB Stick 2015-01-05
29 A JS Framework for Rich Text Tooltips 2014-12-29
28 Preserve Inputs across Page Reload via JS 2014-12-25
27 The Self-Displaying Page 2014-12-20
26 jQuery for Beginners 2014-12-07
25 Object Relational Mapping with Inheritance in Java 2014-11-30
24 The State Pattern in Multiple Selection 2014-11-26
23 Good Documentation 2014-11-17
22 Sass Over CSS 2014-11-15
21 The Shape of Content as CSS 2014-11-06
20 How to Read Cascading Style Sheets 2014-11-04
19 A JS Starter Kit for Beginners 2014-10-29
18 JS Modules 2014-10-24
17 JS Functional Inheritance 2014-10-19
16 JS Inheritance contra Encapsulation 2014-10-16
15 Running JS Scripts from Java with HtmlUnit 2014-10-01
14 JS got cha 2014-09-30
13 This JS new ROFLCOPTER 2014-09-27
12 JavaScript Best Practices 2014-09-21
11 JS and the Forgotten Types 2014-09-20
10 Omigosh, JavaScript! 2014-09-19
9 Unbeloved Constructors, Beloved Anemic Objects 2014-09-01
8 Responsive Layout without CSS media-query 2014-08-22
7 The Modular Homepage Story 2014-07-22
6 Scrum 2010-04-22
5 Scala Considerations 2010-01-12
4 Personal Problems 2008-04-25
3 Fashion 2008-03-12
2 Building Blocks 2008-03-07
1 Things Are Changing 2008-02-26

This table-of-contents has been generated by my BlogSaver Java application, which I wrote about in a passed Blog.

HTML

Here is how checkbox-filtering works. 

  <p>
    <label><input type='checkbox' search-words='JS, JavaScript, jQuery' onclick='filter();'/> JavaScript </label>
    <label><input type='checkbox' search-words='CSS, Cascading Style Sheets' onclick='filter();'/> CSS </label>
    <label><input type='checkbox' search-words='HTML, Page' onclick='filter();'/> HTML </label>
    <label><input type='checkbox' search-words='Java, Constructors' onclick='filter();'/> Java </label>
    <label><input type='checkbox' search-words='LINUX, UNIX, vi, AWK, GIMP' onclick='filter();'/> LINUX </label>
    <label><input type='checkbox' id='Others' onclick='filter();'/> Others </label>
  </p>

  <table>
    <tr>
      <td>104</td>
      <td><a href='The_JS_Function_in_Loop_Bug.html'>The JS Function-in-Loop Bug</a></td>
      <td>2016-06-07</td>
    </tr>
    ........
    <tr>
      <td>1</td>
      <td><a href='Things_Are_Changing.html'>Things Are Changing</a></td>
      <td>2008-02-26</td>
    </tr>
  </table>

This is a set of checkboxes that all call the JS function filter() on click. Each of these has an attribute search-words that contains the terms that must occur in the according article title. This is what the user normally would have to input, but with these attributes I can give all synonyms in one place.

There are two types of search-words, one contains no space, the other is a phrase and thus contains space: "Cascading Style Sheets" goes with "CSS". Thus the terms are comma-separated.

Another difficulty is that the occurrence of "JavaScript" in a title must not lead to finding this as "Java" article. We need a search with word-boundaries.

The table element is (an outline of) the list to filter.

JavaScript

Here is the (preliminary) source code of the filter() function. 

 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
<script type='text/javascript'>
  
  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;
  };
  
  var matches = function(searchPatterns, labelText, labelWords) {
    for (var i = 0; i < searchPatterns.length; i++) {
      var searchPattern = searchPatterns[i];
      if (searchPattern.indexOf(' ') > 0) {
        if (labelText.indexOf(searchPattern) >= 0)
          return true;
      }
      else if (labelWords.indexOf(searchPattern) >= 0) {
        return true;
      }
    }
    return false;
  };
  
  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';
    }
  };
  
  </script>

Yes, you are right, this is hacker code! It contains significant code-smells, and I better should not show this here, because it could ruin my reputation as a software developer :-!

Problems are:

  • big function filter()
  • lacking reusability (DOM access has not been encapsulated in functions, source specializes on checkboxes, ...)
  • missing encapsulation (all functions are globally visible)
  • no parametrization (does not allow to use another attribute-name than search-words)
  • bad naming (parameter csv, although not always dealing with comma-separated-values)
  • error-prone implementations (upper/lower case characters are not covered, inline String operations)
  • separation of concerns violation (event-listener installation is done in HTML, not in JS)
  • documentation is absent

Thus I won't explain this source here and now line by line, because it is not worth. Moreover I will describe how to refactor this in my next Blog, and then it should also become clear how it works.

So don't copy this source code, you will have problems making it work in your page. It's just here to demonstrate how freshly written source code looks like. Find the refactored code of this in follower-Blog(s), and thereby learn how to refactor JavaScript to a reusable module.




Keine Kommentare:

Dienstag, 7. Juni 2016

The JS Function-in-Loop Bug

There is a nasty little thing in JavaScript that goes wrong all the time: putting functions into loops and accessing outer closure-variables from there. Code like this: 

        for (var i = 0; i < document.body.children.length; i++) {
          var element = document.body.children[i];
          element.addEventListener("click", function() {
            clicked(element);
          });
        }

This code loops all direct children of document.body and adds a click-listener to them. The anonymous listener-function calls a function clicked() and passes the clicked element as parameter to it. Doing this, the anonymous function accesses the outer closure variable element.

But this code does not work. I call it the Function-in-Loop Bug. Don't nest functions into loops, or make sure they do not access outer closure variables.

Test Page

Here is a HTML page for trying this out. 

 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
<!DOCTYPE HTML>
<html>

<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1"/>
  <title>How to install a listener in a loop</title>
</head>
  
<body>

  <p>PARAGRAPH</p>
  <span>SPAN</span><br>
  <div>DIV</div>

  <script type="text/javascript">
  var clickListener = (function() {
      var currentElement;
      var self = {};

      self.clicked = function(element) {
        if (currentElement)
          currentElement.style["background-color"] = ""; // reset selection color
          
        currentElement = element;
        
        element.style["background-color"] = "LightBlue"; // set new selection color
      };
      
      return self;
  })();
  </script>

  <script type="text/javascript">
  (function() {
      // initialization goes here ....
  })();
  </script>

</body>
</html>

Three direct children of document.body exist in this page. The first SCRIPT-tag contains a small singleton module that exposes a clicked(element) function, to be used to change the background-color of an element. We would have to call

clickListener.clicked(element);

to do that.

Then there is an empty second SCRIPT-tag, containing an IIFE that will be implemented now to install click-listeners to all body-children.

Wrong

Put the following to where // initialization goes here .... is. 

      var init = function(parent) {
        for (var i = 0; i < parent.children.length; i++) {
          var element = parent.children[i];
          
          if (element.tagName !== "BR" && element.tagName !== "SCRIPT")
            element.addEventListener("click", function() {
              clickListener.clicked(element);
            });
        }
      };

      init(document.body);

This code sets up an init() function which loops the body-children by index and installs a click-callback to them. The init() is called finally to really do that work. Within the loop, we avoid to install the listener also to SCRIPT or BR tags, because this makes no sense. Then there is a nested function which calls clickListener.clicked(element) with the currently looped element.

When you load this page into a web-browser, and you debug the JS code, you will see that the listener actually is called each time you click, but the parameter passed to the clicked() function is wrong, it is always the last SCRIPT tag of the document. Setting a color onto a SCRIPT element has no effect :-!

Why is this wrong? Didn't we avoid to install the listener on SCRIPT tags?

The element within the anonymous function is evaluated only when the function is actually executed, when the user clicks an element. And then the element variable will hold the last looped element as value (which is not what we would expect). The element variable survived the execution of init() because it is in the closure of the anonymous listener function.

How to fix that?

Right

Replace the above implementation now with the following. 

      var installClickListener = function(element) {
        element.addEventListener("click", function() {
          clickListener.clicked(element);
        });
      };
      
      var init = function(parent) {
        for (var i = 0; i < parent.children.length; i++) {
          var element = parent.children[i];
          if (element.tagName !== "BR" && element.tagName !== "SCRIPT")
            installClickListener(element);
        }
      };

      init(document.body);

The problem is to "freeze" the loop-variable element for the call to clickListener.clicked(element). That can be achieved by inserting an installClickListener() function between the loop and the listener-call. The questionable element variable is passed to the installClickListener() function, and as side-effect its value is "freezed", because JS copies the values of parameters when it calls functions with parameters. (Mind that it copies just the pointer, not the content!)

When you try this out now, you will see that any clicked element turns blue, while the previously selected element loses its blue. Which is what we wanted to achieve.

Resume

In Java, all closure-variables have to be final, that means they are real constants and not variables. That way they never could hold a wrong value at a later time when they are accessed by some event-callback. But the rubber-language JavaScript is much nearer to C than to Java, so don't expect technological advance from it.




Keine Kommentare:

Montag, 6. Juni 2016

JS jQuery $(this)

One of the most mysterious things around jQuery is the expression

$(this)

especially in context of the jQuery loops

  • $.each()
  • $(cssSelector).each()

I had a closer look at that, and wrote some test code for this Blog.

jQuery is a function

jQuery is a normal JS function. It has $ as alternate name. So calling 

$(".myCssClass")

is exactly the same as calling 

jQuery(".myCssClass")

jQuery accepts up to two parameters, see API description.
Mind that you can pass a lot of different things as first parameter!

this is a JS constant

The JavaScript "this" keyword represents a constant pointing to the caller of the currently executed function. That means when you have an object "bar" with a function "foo" ... 

var bar = {
  foo: function() {
    return this === bar;
  }
};
alert(bar.foo());

... you would see true on the alert-dialog, because the object bar is the caller of foo().
Some also say that "this" is what is left of the dot.
When you have nothing on the left side of the dot, it will be the window object, as window it is the default-context for JS interpreters running in a browser.

Mind that "this" is JavaScript, not jQuery. So the expression

$(this)

is a call to jQuery with the JS this pointer as parameter!

jQuery each()

The each() loop requires a function as parameter. jQuery ensures that you can access the looped item by the this keyword within that function. That loop-body function can have no parameter, one parameter, or two parameters. If you decide to have no parameter, you still can access the looped item by this. Declaring one parameter will let you receive the index of the item within the loop-body. The second parameter will be the item itself. (Only God and the jQuery programmers know why the item is the second and not the first parameter.) 

$(".someCssClass").each(function(index, item) {
  var bareThis = this;
  var jQueryThis = $(this);
  
  isThisAllTheSame(bareThis, jQueryThis, item);
});

In this source code, jQuery would read all DOM-elements from current document that have an attribute class = "someCssClass". Then it would call the anonymous function that was passed to each() with each of those DOM-elements as second parameter.

Now the interesting question is what's the difference between the three opportunities to access the loop item:

  1. the bare JS this,
  2. the jQuery $(this),
  3. and the parameter item.

Test Page

Here is a HTML test page. 

 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
<!DOCTYPE html>
<html>
<head>
 <meta charset="UTF-8">
 <meta name="viewport" content="width=device-width, initial-scale=1">
 <title>jQuery each() test</title>
    
 <script
  src="https://code.jquery.com/jquery-2.2.4.js"
  integrity="sha256-iT6Q9iMJYuQiMWNd9lDyBUStIq/8PuOW33aOqmvFpqI="
  crossorigin="anonymous"></script>
</head>
     
<body>

 <div id="one" class="loop">One</div>
 <div id="two" class="loop">Two</div>
 <hr>
 <div id="output"></div>
 
 <script type="text/javascript">
  
  var outputElement = function(bareThis, jqueryThis, element) {
   // ...
  };
  
  var loopBody = function(index, element) {
    var bareThis = this;
    var jqueryThis = $(this);
    outputElement(bareThis, jqueryThis, element);
  };
  
  
  $(".loop").each(loopBody);

 </script>

</body>
</html>

Copy & paste this into some file and load that file into your browser. Currently nothing will happen, because the outputElement function is not yet implemented. 

  var outputElement = function(bareThis, jqueryThis, element) {
    var output = $("#output");
    var outputText = "<b>id = "+jqueryThis.attr("id")+"</b><br>";
    outputText += "bareThis = "+bareThis+", element = "+element+", jqueryThis = "+jqueryThis+"<br>";
    outputText += "typeof bareThis = "+(typeof bareThis)+", typeof element = "+(typeof element)+", typeof jqueryThis = "+(typeof jqueryThis)+"<br>";
    outputText += "bareThis == element: "+(bareThis == element)+", jqueryThis == element: "+(jqueryThis == element)+"<br>";
    outputText += "bareThis === element: "+(bareThis === element)+", jqueryThis === element: "+(jqueryThis === element);
    output.html(output.html()+"<br><br>"+outputText);
  };

Looping DOM Elements

Having the output-function added, this is what we see now for the first element with id "one": 

id = one
bareThis = [object HTMLDivElement], element = [object HTMLDivElement], jqueryThis = [object Object]
typeof bareThis = object, typeof element = object, typeof jqueryThis = object
bareThis == element: true, jqueryThis == element: false
bareThis === element: true, jqueryThis === element: false

It is never easy to get clearness about JS objects, so that output is somehow complicated. To summarize it, we see that the bareThis and element parameter are identical, it is the DOM-node, and the jQueryThis is the usual jQuery wrapper around such a DOM-node.

What we learn here is that this points to the item of the loop, not to the caller of the function.

How can this be?
There is a built-in JS function

Function.call(context, parameters)

that lets set the this pointer for the called function as first parameter. jQuery uses exactly that to call the loop-body function. So the this keyword does not really have a constant semantic, it is more a read-only variable that can point to any object the caller considered to be useful.

Looping a String Array

Here is JS code that loops over a String array. 

  var array = [
   "#one",
   "#two"
  ];
  $(array).each(loopBody);

Resulting output for the first element is: 

id = undefined
bareThis = #one, element = #one, jqueryThis = [object Object]
typeof bareThis = object, typeof element = string, typeof jqueryThis = object
bareThis == element: true, jqueryThis == element: false
bareThis === element: false, jqueryThis === element: false

The bareThis and element parameters are equal, but not identical. The bareThis is a String-object that was built by $(array), while element is the real String from the array.

Amazingly I could not trick jQuery to read the DOM-node with id "one": $("#one")!

What we learn is that the this pointer is not what we should use when we really want to work with the array item, because here it was built by jQuery that deep-cloned the array. But as long as identity === is not required, this will be sufficient.

jQuery Source Code

To understand parts of this Blog better, here is the original code of jQuery each(), which is executed by both variants of that function. 

  var each = function( obj, callback ) {
   var length, i = 0;
 
   if ( isArrayLike( obj ) ) {
    length = obj.length;
    for ( ; i < length; i++ ) {
     if ( callback.call( obj[i], i, obj[i] ) === false ) {
      break;
     }
    }
   } else {
    for ( i in obj ) {
     if ( callback.call( obj[i], i, obj[i] ) === false ) {
      break;
     }
    }
   }
 
   return obj;
  };



Keine Kommentare:
Abonnieren Posts (Atom)