current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

The code does the following:

  1. Gets an array of objects and filters them according to some properties
  2. 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;   
},
share|improve this question
    
I don't understand the line "return aNewExtendedObjects[0];" That returns just the first element of the results-array. Shouldn't it be "return aNewExtendedObjects;" ? So that the whole array is returned. – st88 Apr 15 at 7:21
up vote 3 down vote accepted

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;
  });
}
share|improve this answer
    
Thanks you :-) , it seems that the code is not working i've two questions 1. how can I change the code that I will be able to debug it easily 2. in the input array of object I've many objcts that I need to filter and in some case need to return array of filterd object, does your code is handle it? – Nina S Apr 15 at 12:15
    
I think I found the issue, the "wrong object" doesnt have the oDatum.val and the "right" object have oDatum[0].val which I should return , so if I call to the function like this var aa = filtevar rNullUndefinedOrRejected(oData) and return to the next function aa[0].val this is working but there is a way to avoid it? – Nina S Apr 15 at 12:31
    
@NinaS 1) Consider using dev tools, adding breakpoints, trace through code and watch scope variables. 2) filter returns an array. – Joseph the Dreamer Apr 15 at 12:32
    
Yep do it already please see my last comment I found the issue , see if you can help how to avoid the return aa[0].val , thank you very much! – Nina S Apr 15 at 12:33
1  
@cFreed When JS passes primitives, the local variable gets a copy of the value. When JS passes non-primitives, the local variable will point to the same exact value. This means passing foo array to the function, oData points to the same array. JS is always passing by value. It's just a matter of are you getting a copy or pointing to the same thing. – Joseph the Dreamer 2 days ago
up vote 1 down vote

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).

share|improve this answer
up vote 1 down vote

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);

repl demo

share|improve this answer
    
Thanks but it seems that your code is returning true or false, I need to take array of object filter it according the condition I put and return new array of multiple filtered objects... – Nina S Apr 15 at 12:16
    
The true and false you're referring to is in the callback function used by the filter method. That callback is used on each item in the array and removes the item if false is returned. Check it out here (repl) – Nick Apr 15 at 19:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.