HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Lunr backed client side search module

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
searchsidemoduleclientbackedlunr

Problem

I created a client side search module. Could anyone please provide review pointers on it?

```
var search = function(){

"use strict";

var _instanceMap = {};
var maxLimit = 50; //TODO: add a perf test to determine the scale.

/**
* @constructor
* @private
*/
function _IndexCreator(_searchConfig){
this.ref = _searchConfig.ref;
this.id = _searchConfig.id;
this.fieldsConfig = _searchConfig.fieldsConfig;
this.refToDocumentMap = {};
}

/**
* Creates an index with the document and the config
*/
_IndexCreator.prototype.createIndex = function(){
var me = this;
var _idx = lunr(function(){
this.ref(me.ref);
if(!me.fieldsConfig){
return;
}
var idx = this;
for(var key in me.fieldsConfig){
if(key && me.fieldsConfig[key]){
idx.field(key,me.fieldsConfig[key]);
} else {
idx.field(key);
}
}
});
this.idx = _idx;
return _idx;
};

/**
* Searches the index for the document
* @params {object} document - The document to be searched
*/
_IndexCreator.prototype.search = function(docToBeSearched){
var me = this;
var results = this.idx.search(docToBeSearched).map(function(result){
return me.refToDocumentMap[result.ref];
});
return results;
};

return {
/**
* Creates and index and pushes it to the instanceMap
* @params {object} documents - documents to be indexed
* @params {object} searchConfig - config object to create an index.
* @example
* {id:"",
ref:"",
fieldsConfig:{
'tags':{boost:1},
'widgetId'
},
limit:1
} - contains the searchable keys of a document, id of the document,id of the instance and the limit
of the documents which can be indexed
* @params {string} ref - refers to the id of the documents indexed
* @params {string} id - refers to the id of the index c

Solution

Not a full code review, and my comments might be off base if you based your naming on the API you are using. The naming confuses me. I cannot read this code and know what it does without really focusing, that's not how code should read.

For _IndexCreator:

  • ref



  • fieldsConfig



  • refToDocumentMap



Also in these comments:

* @params {string} ref - refers to the id of the documents indexed
   * @params {string} id - refers to the id of the index created


I would go for

* @params {string} doc(ument)sID  - refers to the id of the documents indexed
   * @params {string} indexId - refers to the id of the index created


For this:

_IndexCreator.prototype.createIndex = function(){
    var me = this;
    var _idx =  lunr(function(){
        this.ref(me.ref);
        if(!me.fieldsConfig){
            return;
        }
        var idx = this;
        for(var key in me.fieldsConfig){
            if(key && me.fieldsConfig[key]){
                idx.field(key,me.fieldsConfig[key]);
            } else {
                idx.field(key);
            }
        }
    });
    this.idx = _idx;
    return _idx;
};


  • You do not need _idx, you could assign straight to idx instead



  • The check for key in your loop is not needed.



  • I am not sure, to be tested, but you should be able to always call idx.field(key,me.fieldsConfig[key]); unless you really need that second parameter to be undefined.



  • I would create key as a var up front



  • You only need to assign this to something for closures, otherwise I would advise to stick to this, it helps the reader



  • I would assign me.fieldsConfig[key] to a shortcut variable for easier reading



Something like this:

_IndexCreator.prototype.createIndex = function(){
    var me = this;
    this.idx =  lunr(function(){
        var key, value;
        if(!me.fieldsConfig){
            return;
        }
        this.ref(me.ref);
        for(key in me.fieldsConfig){
            value = me.fieldsConfig[key];
            if( value  ){
                this.field(key, value);
            } else {
                this.field(key);
            }
        }
    });
    return this.idx;
};


For function update, there is a clear case of copy pasting of this :

idxInstance.idx.update(documents);
        var refId = documents.refId,
            refDocMap = idxInstance.idx.refToDocumentMap;
        refDocMap[refId] = documents;
        idxInstance.idx.update(documents[i]); <--- Hmmmm?
    }


Except that now you seem to have a bug potentially, since i is either undefined or some random global variable. Create a function with the proper 4 lines and call it either in a loop or not. Actually, my preferred approach for the how to deal with something that could be an array? is turning a not-array into an array and then just deal with arrays:

update: function(documents, id){
    if(_instanceMap[id] === undefined){
        console.log("No index found for the id");
        return;
    }
    var idxInstance = _instanceMap[id];
    //Turn that documents into an array!
    if(!Array.isArray(documents)){
        documents = [documents];
    }
    //Normal processing
    for(var i = 0; i < documents.length; i++){
        var refId = document[i].refId,
            refDocMap = idxInstance.idx.refToDocumentMap;
        refDocMap[refId] = documents[i];
        idxInstance.idx.update(documents[i]);
    }
}

Code Snippets

* @params {string} ref - refers to the id of the documents indexed
   * @params {string} id - refers to the id of the index created
* @params {string} doc(ument)sID  - refers to the id of the documents indexed
   * @params {string} indexId - refers to the id of the index created
_IndexCreator.prototype.createIndex = function(){
    var me = this;
    var _idx =  lunr(function(){
        this.ref(me.ref);
        if(!me.fieldsConfig){
            return;
        }
        var idx = this;
        for(var key in me.fieldsConfig){
            if(key && me.fieldsConfig[key]){
                idx.field(key,me.fieldsConfig[key]);
            } else {
                idx.field(key);
            }
        }
    });
    this.idx = _idx;
    return _idx;
};
_IndexCreator.prototype.createIndex = function(){
    var me = this;
    this.idx =  lunr(function(){
        var key, value;
        if(!me.fieldsConfig){
            return;
        }
        this.ref(me.ref);
        for(key in me.fieldsConfig){
            value = me.fieldsConfig[key];
            if( value  ){
                this.field(key, value);
            } else {
                this.field(key);
            }
        }
    });
    return this.idx;
};
idxInstance.idx.update(documents);
        var refId = documents.refId,
            refDocMap = idxInstance.idx.refToDocumentMap;
        refDocMap[refId] = documents;
        idxInstance.idx.update(documents[i]); <--- Hmmmm?
    }

Context

StackExchange Code Review Q#48575, answer score: 2

Revisions (0)

No revisions yet.