Refactor sortColumn

[edelooff]

This PR attempts to clean up the sortColumn method by compressing the similar blocks of code used for sorting, using jQuery provided facilities and re-using variables declared at plugin initialization on the table.

Specifically it does the following:

• Removes the $headers variable used to find table head cells and uses the already defined data.$headerRow for this
• Replaces the inline sort function with a defined sort function:
  • Sort func adds an order argument for either ascending or descending operation
  • Performs a single .find() for the relevant cell (previous code did a second if data-value was undefined)
  • When used to sort, the rowComparator has its first argument bound reducing its signature to a,b with a locked sort order
• Sort result is applied to current table rather than rows removed and then re-added
• Simplifies class setting for sorted column header

I noticed the change of the (long) boolean short-circuit in the other PR and there's some more stylistic bits here that I'm not sure follow your style. There's some newline-happy method chaining, use of (short and simple) ternaries and another use of boolean short-circuiting. I think they add to readability of the code, but of course preferences differ

If this or anything else needs a change though, I'd be happy to update the PR. (Also, if they don't follow style, do you have a reference document somewhere for that?)

[edelooff]

As a bonus: the shorter code-path in the sort to retrieve the .text() value makes the sorting a good bit faster as well. Results obtained from a table of some 250 rows on a timbles-enabled table.

Original sortColumn
Sorting a column by data-value: 114ms, 108ms, 108ms, 101ms [106ms]
Sorting a column by .text(): 315ms, 249ms, 248ms, 237ms [244ms]

Refactored sortColumn
Sorting a column by data-value: 108ms, 93ms, 92ms, 91ms [92ms (~15% faster)]
Sorting a column by .text(): 187ms, 154ms, 148ms, 154ms [152ms (~60% faster)]

Timing results are for asc-desc-asc-desc sortings respectively. The timing in brackets is the average of the set after discarding the first measurement. Why the first measurements is so different from the others I'm not sure about. I suspect it's due to browser optimizations, as the code certainly does cache anything between runs.

Measurements obtained by changing the click event function to the following:

click: function(e) {
  var t0 = performance.now();
  methods.sortColumn.call($this, $(this).attr('id'), false);
  var t1 = performance.now();
  console.log((t1 - t0) + "ms");
}

[jennschiffer]

This all looks great - I should have some time later this week to test and merge!

[edelooff]

I realized something earlier today about sorting and comparisons. There are many more comparisons than items in the array to be sorted (I know, shocking stuff). However, if one of the slow things is retrieving the correct value to compare against, it makes sense to retrieve all of those once, and sort based on those values. This means only (n) value retrievals rather than approximately (2n log n) value retrievals (because each comparison needs to get both values).

The last commit does this and the gains seem to be quite substantial. On another test table that's around 2300 rows, a sort with the orignal PR code takes about 2100ms. With the new one, it takes all of 350ms. Smaller tables gain performance as well, but a bit less pronounced.

Unfortunately, the code grows a bit in size again but the extract, sort and apply steps are separate enough to not get too complex. The final row placement takes a native DOM approach to avoid jQuery overhead (using jQuery there would bump the time taken up to ~500ms)

[edelooff]

Another small change to the previous code. This detaches the <tbody> before sorting the rows on it. This doesn't affect small tables much, but does really good things for the rather large and slow to sort 2300 row table I'm also testing against:

• Chrome 46 is goes down from 350ms for a sort to 200ms*;
• Firefox 40 drops from 340ms to roughly 260ms.

Medium size tables seem largely unaffected, a 250 row test table sorts in a similar 22-28ms across both browsers, with a slight edge toward the detached-DOM variant. Even smaller tables may well sort a little slower, but the difference would be hardly noticeable (6 vs 8ms for a sort).

From a technical/history point of view, should I squash this to a single, or reduced set of commits to better show the changes from the master branch?

* It has to be noted that Chrome has a rather optimistic view of how long things take. After the sorting is completed (the point at which the time is measured) it takes another half second (stopwatch estimate) before the document is actually repainted. Firefox has a similar discrepancy between reported time and actual time, but it's a bit harder to tell as the JS console is not immediately flushed upon writing to it. The delay seems to be the same regardless of detached or attached DOM, it just makes results less immediately obvious.

[jennschiffer]

yeah a squash would be great! i got thrown into a new project so i've lost out on some "open source time" but i plan to set aside thursday to catch up on github stuff 🎅

[edelooff]

Squashed it down to three commits that each do their own part of the change (though not all parts are created equal). If you want it down to just a single commit that works, but this way the steps taken should be easier to follow.

Thanks for the schedule update, Thursday would be excellent :-)

[jennschiffer]

Hey this looks great speed-wise. I was testing it out with the examples, though, and it doesn't sort paginated tables - check it out in this example: https://github.com/edelooff/timbles.js/blob/master/examples/timbles-pagination.html

I'm looking into it right now but I could also use your eyes on this. Then we will be good 2 merge!

[jennschiffer]

🎅

[edelooff]

That is odd... Some quick console logging seems to indicate that data.$records doesn't get sorted, which is what throws off enablePagination().

Changing the assignment of $recordsToPaginate to $this.find('tbody tr') (from data.$records), and subsequent references to data.$records with $recordsToPaginate causes the sort to do the right thing, but what remains is problematic: increasing the page size doesn't work. (Bonus: I now understand why data.$records exists.)

Another (suboptimal) solution seems to be to re-assign the data.$records list with a fresh jQuery extract: data.$records = $this.find('tbody tr');. This has the bonus of not touching with the pagination code which I'm not sure I understand well enough to be messing with.

The data.$records update solution is easy to understand and makes sense, but it feels like a hack. I'll try some more solutions over the weekend, see if I can come up with something better.

[jennschiffer]

never mind - figured it out, just needed to save the rows and records to data so it knows what to paginate. all merged and fantastic, thanks!

I added you as a collaborator to this project. I say all future updates, mine included, come through feature branches and PRs. I'm going to update the version number and publish to npm right now though 🎅

[jennschiffer]

Also if you don't wanna be a collaborator, that's fine. Literally no obligation either way. But I can also add you on NPM if you let me know your username - https://www.npmjs.com/package/timbles

[edelooff]

Thanks for that. Just made a brand new NPM account for this, because hey, apparently I do JavaScript as well now :-)

https://www.npmjs.com/~edelooff is where that's at.

[jennschiffer]

awesome. welcome to hell