Problem
The code does the following:
- Gets an array of objects and filters them according to some properties
- Returns the array of objects with the filtered data
I’m using the following code which is working and I wonder if there is a way to write it better.
By the way, I can use LodashJS if it’s helpful here.
filterNullUndefinedOrRejected: function(oData) {
if (oData && oData.length > 0) {
var aNewExtendedObjects = [];
for (var i = 0; i < oData.length; i++) {
if (oData[i].va === null || oData[i].val === undefined || oData[i].state === "rejected") {
if (oData[i].state === "rejected") {
console.log(oData[i].error);
}
oData.splice(i, 1);
i--;
}
else{
aNewExtendedObjects.push(oData[i].val);
}
}
return aNewExtendedObjects[0];
}
return oData;
},
Solution
splice
mutates the existing array. You wouldn’t want to do that as you might not be sure where else the array is being used. You might be expecting values in the array after calling this function, yet this function wiped them off.
Now if you’re really sure you want to use splice
to remove items in an array, loop backwards. The ending i--
isn’t very visible and missing it would mean skipped items.
Lodash isn’t necessary as you have the native array.filter
. This is unless you still plan to support browsers that don’t do ES5 (like IE8 and older).
Your function isn’t consistent. The ideal return is an array. But if I put in a falsy value, it returns that value, not an array.
The length check is meaningless. An empty array has the length of 0
which means the loop will never run at all.
Your function is essentially just including all values that aren’t null
, undefined
or have a state of rejected
. However, the name is misleading. You never explicitly mention that you’re only getting the val
properties as a result. You should name your function explicitly, or split off the operations separately.
As noted in the comments (and it wasn’t apparent in the code), you’re collecting the val
property of the filtered items. This can easily be done with an array.map
. So after filtering down to the items we do need, we transform the values to the ones we need.
filterNullUndefinedOrRejected: function(oData){
return !oData ? [] : oData.filter(function(oDatum){
if(oDatum.state === 'rejected') console.log(oDatum.error);
return oDatum.val !== null && oDatum.val !== undefined && oDatum.state !== "rejected";
}).map(function(oData){
return oData.val;
});
}
If you are interested, there is an ES6 version of Joseph’s response :
function filterNullUndefinedOrRejected (datas){
return (!datas) ? [] : datas.filter(data => {
if(data.state === 'rejected') console.log(data.error);
return hasRejectedStateWithAValue(data);
}).map(data => data.val);
}
function hasRejectedStateWithAValue(data) {
return data.val !== null && data.val !== undefined && data.state !== "rejected";
}
Also I have extracted the condition expression in its own function to make the code more readable (the function name allow us to explain the goal of the condition).
You can use the array filter method.
filterNullUndefinedOrRejected: function(oData) {
if (oData && oData.length > 0) {
return oData.filter(function(item){
// this function is used on on each value in the array
// if `false` is returned, that item is removed
if (item.val === null || item.val === undefined || item.state === "rejected") {
if (item.state === "rejected") {
console.log(item.error);
}
return false;
}
else return true;
}
}
}
You could also use the lodash _.filter
, the only slight difference is in syntax:
filterNullUndefinedOrRejected: function(oData) {
if (oData && oData.length > 0) {
return _.filter(oData, function(item){
if (item.val === null || item.val === undefined || item.state === "rejected") {
if (item.state === "rejected") {
console.log(item.error);
}
return false;
}
else return true;
});
}
}
Filter methods take an array and invokes a callback function on each item, expecting a boolean back. If false is returned, the item is removed.
This method returns a new array. So to update the data you would have to do something like:
dataSet = filterNullUndefinedOrRejected(dataSet);