patternjavascriptMinor
DOM-manipulation search-ordering algorithm
Viewed 0 times
searchorderingalgorithmmanipulationdom
Problem
(this is a crosspost from StackOverflow, it was suggested I asked here)
Goal
I've got a DOM with about 70 elements on it (divs with some content) . I need to move and toggle the display of those divs quite a lot and also quite fast. The speed is one of the most important things. The trigger for moving and toggling these divs is a search query, kind of like Google Instant, except that all the DOM elements I move around and toggle are loaded the first time (so no more calls to the server).
Implementation
I've implemented this in the following way: alongside the DOM I pass in a JavaScript array of objects representing the divs along with their attributes like position, contents etcetera. This array acts like a mirror to the DOM. When the user starts typing I start looping through the array and calculating, per div/object, what needs to be done to it. I actually loop over this array a couple of times: I first check if I need to look at a div/object, then I look at the object, then whether I need to look at the contents, then I look at the contents.
One of the things I do in these loops is the setting of flags for DOM-manipulation. As I understand it, reading and manipulating and the DOM is one of the slower operations in JavaScript, as compared to the other stuff I'm doing (looping, reading and writing object attributes etc.). I also did some profiling, confirming this assumption. So at every corner I've tried to prevent "touching" the DOM to increase performance. At the end of my algorithm I loop once more, execute all the necessary DOM actions and reset the flags to signal they've been read. For cross-browser compatibility I use jQuery to actually do the DOM actions (selecting, moving, toggling). I do not use jQuery to loop over my array.
Problem
My problem now is that I think my code and data structure is a bit ugly. I have this rather
large multidimensional array with lots of attributes and flags. I repeatedly loop over it with functions calling functions c
Goal
I've got a DOM with about 70 elements on it (divs with some content) . I need to move and toggle the display of those divs quite a lot and also quite fast. The speed is one of the most important things. The trigger for moving and toggling these divs is a search query, kind of like Google Instant, except that all the DOM elements I move around and toggle are loaded the first time (so no more calls to the server).
Implementation
I've implemented this in the following way: alongside the DOM I pass in a JavaScript array of objects representing the divs along with their attributes like position, contents etcetera. This array acts like a mirror to the DOM. When the user starts typing I start looping through the array and calculating, per div/object, what needs to be done to it. I actually loop over this array a couple of times: I first check if I need to look at a div/object, then I look at the object, then whether I need to look at the contents, then I look at the contents.
One of the things I do in these loops is the setting of flags for DOM-manipulation. As I understand it, reading and manipulating and the DOM is one of the slower operations in JavaScript, as compared to the other stuff I'm doing (looping, reading and writing object attributes etc.). I also did some profiling, confirming this assumption. So at every corner I've tried to prevent "touching" the DOM to increase performance. At the end of my algorithm I loop once more, execute all the necessary DOM actions and reset the flags to signal they've been read. For cross-browser compatibility I use jQuery to actually do the DOM actions (selecting, moving, toggling). I do not use jQuery to loop over my array.
Problem
My problem now is that I think my code and data structure is a bit ugly. I have this rather
large multidimensional array with lots of attributes and flags. I repeatedly loop over it with functions calling functions c
Solution
-
First off...all this
-
Secondly, you're over-granulating. The way you're doing things (doing all of one thing, then all of the next), it all but requires your object to wear all kinds of flags to carry information between functions. However, you could turn something like
to something like
which would make it possible to keep intermediate results local, and thus eliminate the need for a bunch of your flags.
-
-
Also, keep abstraction levels in mind. At the higher level of
With those things fixed, and some rearranging of parameters and such, this is what i have for everything up to and including
*/
function matchLink(link, query, previousQuery) {
if (isMatchingNeeded(link, query, previousQuery)) {
var properties = [ 'title', 'label', 'url', 'url', 'keywords', 'col', 'row' ];
link.matches = false;
for (var i = 0; i = 0. I
First off...all this
cats = and return cats confuses things, since you never actually replace cats -- the items inside are what you mess with. cats = doSomethingWith(cats) implies to me that doSomethingWith(cats) clones the array and doesn't touch the original.-
Secondly, you're over-granulating. The way you're doing things (doing all of one thing, then all of the next), it all but requires your object to wear all kinds of flags to carry information between functions. However, you could turn something like
x(cats)
y(cats)
z(cats)
function x()
for each (item) in cats:
do step 1 to item
function y()
for each (item) in cats:
if item.(step 1 result) says so, do step 2
function z()
for each (item) in cats:
...
to something like
for each (item) in cats:
do step 1 to item
if (step 1 result) says so, do step 2
...
which would make it possible to keep intermediate results local, and thus eliminate the need for a bunch of your flags.
-
isMatchingNeededForCat and isMatchingNeededForLink do the exact same thing (with the same properties, even!), and should be combined into one function.-
Also, keep abstraction levels in mind. At the higher level of
applyFilter, you don't particularly care if matching is required for an item. That's an optimization detail that can be relegated to the point where you're actually trying to match categories, particularly since (assuming previousQuery and currentQuery are similar enough, as the code seems to assume they are) it has little to no effect on the result. (isMatchingNeeded could always return true if it wanted to, and the code should still work, albeit slower.)With those things fixed, and some rearranging of parameters and such, this is what i have for everything up to and including
flagIfDisplayToggleNeeded. The code's been reduced by about 60% (even more if you don't count comments), and a number of "moving parts" have been eliminated./**
* Applies the filter (defined by query) to the cats array
*
* -finds and flags matching categories, and matching links within categories
* -checks whether DOM action is needed
* -if needed executes DOM action
*
* cats is an array of objects representing categories
* which themselves contain an array of objects representing links
* with some attributes
*
* cats = (array) array of categories through which to search
* currentQuery = (string) with which to find matches within the cats
* previousQuery = (string) with previously-typed-in query
*
* no return values, results in DOM action and manipulation of cats array
*/
function applyFilter(cats, query, previousQuery) {
// previousQuery must be a string (or at least quack like one)
if (previousQuery == undefined || !('length' in previousQuery)) {
previousQuery = '';
}
for (var i = 0; i 0 ) {
flagIfMoveNeeded(cats);
} else {
// move everything back to its original position
flagMoveToOriginalPosition(cats);
}
// take action on the items that need a DOM action
executeDomActions(cats);
}
/**
* Attempts to match the given link to the current query, if necessary.
* cat's matches and noMatchFoundAtNumChars properties will be set accordingly.
*
* cat (object) : the category to check. Will be modified as described below.
* query (string) : the current query.
* previousQuery (string) : the previous query, for which cat holds valid results.
*
* No return value. Modifies cat to reflect the results of the match attempt.
* The matches property will reflect whether the current query matches this item, and
* noMatchFoundAtNumChars reflects the current query's length if no match was found.
*/
function matchCategory(cat, query, previousQuery) {
if (isMatchingNeeded(cat, query, previousQuery)) {
var catName = cat.category_name.toLowerCase();
cat.matches = (catName.indexOf(currentQuery) !== -1 );
cat.noMatchFoundAtNumChars = (cat.matches ? false : query.length);
}
}
/**
* Attempts to match the given link to the current query, if necessary.
* link's matches and noMatchFoundAtNumChars properties will be set accordingly.
*
* link (object) : the link to check. Will be modified as described below.
* query (string) : the current query.
* previousQuery (string) : the previous query, for which link holds valid results.
*
* No return value. Modifies link to reflect the results of the match attempt.
* The matches property will reflect whether the current query matches this item, and
* noMatchFoundAtNumChars` reflects the current query's length if no match was found.*/
function matchLink(link, query, previousQuery) {
if (isMatchingNeeded(link, query, previousQuery)) {
var properties = [ 'title', 'label', 'url', 'url', 'keywords', 'col', 'row' ];
link.matches = false;
for (var i = 0; i = 0. I
Code Snippets
function showCategory(category_id) {
document.getElementById(category_id).style.display = '';
}
function hideCategory(category_id) {
document.getElementById(category_id).style.display = 'none';
}Context
StackExchange Code Review Q#6638, answer score: 5
Revisions (0)
No revisions yet.