I agree with Jeremy, but was already typing when he responded…
Going off your title and inferring what you mean: Is it concise enough, is it clear enough?
Well what does that mean? What is the goal?
Could it be more concise? Sure, this could be solved in one line:
const destroyer = (arr, ...keysToRemove) => arr.filter(el => !keysToRemove.includes(el))
Does that make it better? I would say that we have to be careful about being “too concise” at the expense of clarity. I could make it even smaller, like this:
const destroyer = (a, ...r) => a.filter(x => !r.includes(x))
Is that better? It’s more concise. But it’s also less clear. It’s all about a balance, trying to maximize both. And to me clarity is much more important than concision.
Your solution? For a learner, it’s great! You solved the problem. You found a solution. It works. You reinforced your coding knowledge. You understand what is happening on an algorithmic level. That is the purpose here - you nailed it.
If it came across my desk in a PR review, I might critique it for creating unnecessary variables, instead of leveraging some functional programming and ES6 features.
In “the real world” I would expect to see something like my first example. It is clear and concise. It is easy to understand (if you understand ES6 and FP, which a professional coder should) and is (imho) easier (or at least quicker) to absorb than the lengthier example you created. This is where concise is good - all things being equal, fewer lines are easier to digest and to maintain. There are fewer opportunities for bugs. The second example I provided? That is just being concise to try to show off. I would not like the second example.
So in summary, good job. You solved the problem. When I was at your stage, my solution was probably very similar. Is it the “best” solution? Maybe not. But it’s excellent for where you are.
I do have one complaint in your code:
let agr = [...arguments];
Why make a copy? You don’t need to mutate it in anyway so there is no need.
if(agr.includes(newArr[i]) === false){
Most people would write something like:
if(!agr.includes(newArr[i])){
Unless you are explicitly checking for false
, then a falsy check is sufficient.
agr.includes(newArr[i])
This bothers me a little, since agr contains as it’s first element a reference to newArr. It works for this test data. But I wonder if it would fail for some array with a circular reference.
const arr = ["tree", "hamburger", 53];
arr.push(arr);
console.log(arr);
// ["tree", "hamburger", 53, Array(4)]
console.log(destroyer(arr, "tree", 53));
// ["hamburger"]
In other words, the original array is something that we’re checking to remove from the original array. That’s kind of an edge case, and maybe it is desirable behavior - it just struck me as odd. If I saw that come across my desk I would ask questions.
But still, good job.