patternjavascriptMinor
Lunr backed client side search module
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
```
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
Also in these comments:
I would go for
For this:
Something like this:
For function
Except that now you seem to have a bug potentially, since
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 createdI 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 createdFor 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 toidxinstead
- The check for
keyin 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 beundefined.
- I would create
keyas a var up front
- You only need to assign
thisto something for closures, otherwise I would advise to stick tothis, 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.