current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I find the default Javascript extremely poor on useful functions. There are some nice libraries, but somehow I always need something I can't find. Currently, I'd need a method removing all matching properties from an object. And I really mean removing, not creating a filtered copy. I've found similar methods in both jquery and underscore, but no exact match.

That's a long introduction, why I want to give it a try. As both $ and _ are taken, I'm extending the Object.prototype which should be fine. So this is the code:

function removeMatching(predicate) {
    for (var key in this) {
        if (!Object.prototype.hasOwnProperty.call(this, key)) continue;
        if (predicate(key, this[key])) delete this[key];
    };
};

Object.defineProperty(Object.prototype, 'removeMatching', {
    value: removeMatching,
    writable: false,
    configurable: false,
    enumerable: false
});

It's all gets enclosed in a closure to prevent namespace pollution. I don't care much about speed as this can be improved later (but tell me). I don't care about style (it's fixed) not about compatibility with old crap. I do care about all problems possible in modern browsers.

It's to be used like

var o = {a: 1, b: 2, c: 3};
o.removeMatching(function(k, v) {return k === "a" || v === 2});
// now o is {c: 3}
share|improve this question
    
So, you're not asking about coding style or speed. What exactly do you want feedback on? There are no problems iterating or deleting properties in any browser. Object.defineProperty() only exists in modern browsers, but I assume you already know that. –  jfriend00 Jun 24 '14 at 21:29
    
@jfriend00 I could imagine e.g. a Java-like problem deleting a property during an iteration. Or whatever (and the things I said I don't care about might be good to hear about anyway). –  maaartinus Jun 24 '14 at 22:53
    
To be forward compatible wrap the declaration in an if: if (Object.prototype.removeMatching === undefined) { ... } –  pgraham Jun 25 '14 at 1:24

3 Answers 3

up vote 6 down vote accepted

There are extraneous semicolons (null statements) in your code. Semicolons do not belong after function declarations and for loops.

I'm extending the Object.prototype which should be fine.

I would argue that it's not fine. While it's unlikely to cause undesired side effects, breaking encapsulation and polluting all objects just for the sake of being able to write foo.removeMatching(bar) instead of removeMatching(foo, bar) seems like bad form. 

if (!Object.prototype.hasOwnProperty.call(this, key)) continue;

Since this is an Object by definition, because you are attaching this to Object.prototype, there is no need to write it like that. This would achieve the same thing (unless you are shadowing hasOwnProperty in which case you have bigger problems):

if (!this.hasOwnProperty(key)) continue;

However there's no need to write this line in the first place. If the object doesn't have a property as its own property, attempting to delete it will do nothing.

It's all gets enclosed in a closure to prevent namespace pollution.

This also seems unnecessary, why not just write it like this:

Object.defineProperty(Object.prototype, 'removeMatching', {
    value: function (predicate) {
        for (var key in this) {
            if (predicate(key, this[key])) {
                delete this[key];
            }
        }
    },
    writable: false,
    configurable: false,
    enumerable: false
});
share|improve this answer
    
Nice, thanks! Just a comment: "instead of removeMatching(foo, bar)" - no, I'd have to write someGoddamnPrefix.removeMatching(foo, bar) and make sure that it's in scope. And it's not writing, what takes time, it's reading. –  maaartinus Jun 24 '14 at 22:45
2  
@maaartinus If this is intended to be generic library code for anyone to use, I would strongly urge you not to modify Object.prototype. It may be excusable in application code, but not in library code. You don't necessarily need to pollute the global scope with someGoddamnPrefix; check for the presence of AMD-style define and CJS-stlye exports and use one of them if available, and fall back to your "namespace" as a last resort. I'd also suggest simply using the name of your library as that namespace; it's much more meaningful and straightforward than something like $ or _. –  Dagg Jun 25 '14 at 6:12
up vote 1 down vote

You could define it on Object itself instead of adding it to the prototype and exposing it to every object. This would parallel how Object.keys works:

Object.defineProperty(Object, 'removeMatching', { ... });

Since you're already requiring ECMAScript 5 you may as well use Object.keys and Array.prototype.forEach to perform the iteration. This removes the need for hasOwnProperty (which @Dagg points out isn't necessary anyway), but it avoids the problem of modifying the underlying object's shape while iterating over it. I do not know which browsers, if any, would be affected by it.

Combining this with the above yields

Object.defineProperty(Object, 'removeMatching', {
    value: function(obj, predicate) {
        Object.keys(obj).forEach(function (key) {
            if (predicate(key, obj[key])) {
                delete obj[key];
            }
        };
    },
    writable: false,
    configurable: false,
    enumerable: false
});

var o = {a: 1, b: 2, c: 3};
Object.removeMatching(o, function(k, v) {return k === "a" || v === 2});
share|improve this answer
    
+2 if I could.. –  konijn Jun 26 '14 at 18:57
    
@konijn, what in particular do you like about this answer? As is, your comment doesn't add much. IMO, adding properties to Object still breaks encapsulation (why not just store stuff in our own namespace?) and suggesting that there could be problems "modifying the underlying object's shape while iterating over it" in this case is misleading. It's not clear to me why you think this answer is worth double upvotes. –  Dagg Jun 26 '14 at 21:57
    
@Dagg Thx for the link. As I said, I wasn't sure how browsers treat this and didn't know it was in the spec. Luckily, every browser precisely adheres to the spec. ;) –  David Harkness Jun 27 '14 at 0:10
    
@DavidHarkness this is the sort of thing browsers have to adhere to if they don't want to break thousands of web sites. Places where browsers don't adhere to the spec these days are usually very obscure corner cases. Anyway, I wasn't trying to bash your answer, just trying to understand konjin's comment. –  Dagg Jun 27 '14 at 0:18
1  
@Dagg, I am not sure what you mean with breaks encapsulation. +2 because I dont like modifying Object.prototype, I also dont like just stuffing in my own namespace, adding it to Object itself is an elegant lesser of evils for me. I will actually use this approach for my own code, which is kind of rare for answers on CR. –  konijn Jun 27 '14 at 12:59
up vote 0 down vote

Put it on Function.prototype, makes more sense that way anyhow, and enhance it a bit more by offering two more lists -- (nature does this double-negative type of mechanism all the time). You could also get fancy with the return value.

        filter.removePropertiesFrom(obj);// simple form
        filter.removePropertiesFrom(obj,options);
        // where
        var options={
           except:[names]||filterFn||RegEx
           ,always:[names]||filterFn||RegEx
        };
        // chaining, etc /////////////////////
         // meh, pass forward options via a temp attached to a dynamic and()?
        filter.removePropertiesFrom(obj).and(obj2);
        filter.removePropertiesFrom(obj).fn;// hand back filter()
        filter.removePropertiesFrom(obj).removed;// [names]
        filter.removePropertiesFrom(obj).items;// new object with the removed

        //inline call style
        (function(k, v) {return k === "a" || v === 2}).removePropertiesFrom(obj);
share|improve this answer

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.