Skip to content

added remove method (issue #1856) #2906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

added remove method (issue #1856) #2906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions modules/remove.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import findIndex from './findIndex.js'

// removes first element having the condition
export default function remove(collection, predicate){
collection.splice(findIndex(collection, predicate), 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a start, but the intent of #1856 is to remove all elements that match the predicate. I think this will be easiest to do correctly if you go through collection backwards, i.e., remove the rightmost element first and the leftmost element last.

return collection;
}
7 changes: 7 additions & 0 deletions test/arrays.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,4 +583,11 @@
assert.deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 2), [[10, 20], [30, 40], [50, 60], [70]], 'chunk into parts of less then current array length elements');
assert.deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 3), [[10, 20, 30], [40, 50, 60], [70]], 'chunk into parts of less then current array length elements');
});

QUnit.test('remove', function(assert) {
assert.deepEqual(_.remove([1, 2, 3, 4], [1, 3, 4]), function(obj) {return(obj == 2)});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems to have been written with the following intuition: if you have the elements 1, 2, 3, 4 and you remove the elements 1, 3, 4, you'll be left with only element 2. While this intuition is right, this is not what you should be testing, and the way you're trying to test it also doesn't work.

You shouldn't be testing this, because the intent of _.remove is to take as its second argument a criterion (predicate) by which to decide whether an element should be removed, not a list of values to be removed. A function that accepts a list instead of a criterion as its second argument would be the modifying version of _.difference instead of the modifying version of _.reject. As-is (with your current implementation) the argument [1, 3, 4] will in fact be interpreted as a predicate, i.e., it will be passed to _.iteratee and be converted to a function equivalent to function(x) { return x[1][3][4]; }. I'm sure this is not at all what you meant; I think you meant function(x) { return x == 1 || x == 3 || x == 4; }.

The test also doesn't work, because assert.deepEqual() takes two arguments and then recursively compares them by value. You're passing it two arguments: the result of _.remove([1, 2, 3, 4], [1, 3, 4]) and a function. Unless you expect _.remove to return a function, this assertion is always going to fail. You should pass the expected value as the second argument to assert.deepEqual, not a function that performs a comparison.

There is also something missing. You should not only compare the result of _.remove to an expected value, but also verify that this result is the same array as the one that you passed as the first argument. After all, the intent of _.remove is that it modifies the array in place.

assert.deepEqual(_.remove([{a: 1}, {a:3, b: 2}, 3], [{a: 1}, 3], function(obj) {return(obj.a == 3)}));
assert.deepEqual(_.remove([{a: {a: 1}}, 2, 3], [2, 3], function(obj) {return(obj.a.a == 1)}));
assert.deepEqual(_.remove([{a: 1, b: 2, c: 3}, {a:3, b: 2}, {c: 3}], [{a:3, b: 2}, {c: 3}], function(obj) {return(obj.b == 2 && obj.c == 3)}));
Comment on lines +589 to +591
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next three tests have the same problems, but in addition, you mixed up the parentheses. You're now passing three arguments to _.remove and only one to assert.deepEqual.

Copy link
Author

@matinFT matinFT Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry my bad. don't know how i came up with this! i messed up deepEqual and _. remove second arguments. i'll fix it and try with a new pr.

});
}());
9 changes: 9 additions & 0 deletions underscore.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.