paulirish / break-on-access

break on access to a property
Other
1.16k stars 127 forks source link

optimize conditional statement #7

Closed tychio closed 11 years ago

tychio commented 11 years ago

I think it clearer.

paulirish commented 11 years ago

looks sharp here. @ashmind your move.

morganrallen commented 11 years ago

I think it looks fine and doesn't break tests

On Mon, Sep 16, 2013 at 7:59 PM, Paul Irish notifications@github.comwrote:

looks sharp here. @ashmind https://github.com/ashmind your move.

— Reply to this email directly or view it on GitHubhttps://github.com/paulirish/break-on-access/pull/7#issuecomment-24560995 .

http://ithoughtyouweretherobot.com Metal and Wire

http://nolonelyguineapigs.com/ Wandering and Rambling

http://morglog.org Old and Neglected.

-----BEGIN PGP PUBLIC KEY BLOCK----- Version: OpenPGP.js v.1.20121007 Comment: http://openpgpjs.org

xsBNBFG3btUBB/9+/WJNOSIuc/praKPaPOweqXV5s7PGRD+HAnNWF/19YAY3 AFtfeoelhhY4sMJoobTaJzcZojznZ1kl/7UuuYCnbJO3Z9kaSMVrVxEZMSLe YmW8Hc4ZsTnl9f05DQFy8ABpAvMLmXJQXh63BbfzjFqNbSWQfLhrubSM2Elq xmN6EsmRyAuEeYlSnal+Di2MlViohpCbbagE2D6AZTUECIamTib6+DuLG4XO b4GGKHR9TXu2qS1VH6hqcvdEz2MDE5OqZzAnBgcF2dvcOdTk9gAOoR1T+qKj IJtg2X0ETbxcPqEXQoXE8VrxN0MLT5JQKTg26+csU4P2w0rEWsUsJombABEB AAHNJU1vcmdhbiBBbGxlbiA8bW9yZ2FucmFsbGVuQGdtYWlsLmNvbT7CwFwE EAECABAFAlG3btwJEG9pFLKaVEfeAAD7hwf+K8LN/kfgi8GTRLiTcuM+hSbm uAeiBY+VaJIQFLzoFlt8A0hBlLNPFNNmUDxdPrMErdHgZ7HtJW/6RANuBTti 5hr3EBguH1GQyK7BbUqJGvaEX4UNRej3uyX3ufXDsIB/3NsKaTyyu+SO0+Hv Cn2DwmlfSXcPhoarx6Turizn/WwgtTN3tqrMWxxiCKWydLH5xYuz+23BLMdc oF/zkbF+7ddFhmyfiFh4Ej9//tsfwPj2cACU3uLTrcwCo7IxUtkrqMF/fhlm LEqjhzDDjq9v7GNWb/5xB2xgTaucJ7/ljuS8nPlZqwCI8NlTgSqC8nyVLyjS yVtOzkItcS0UqyW1Mg== =V/Ia -----END PGP PUBLIC KEY BLOCK-----

ashmind commented 11 years ago

I think it is great, but might be streamlined a bit more? (Sorry I have only time for a quick look, so I couldn't run tests on following proposal)

// ...
// remove verifyNotWritable

var enabled = true;
var originalProperty = getPropertyDescriptor(obj, propertyName);
var newProperty = { enumerable: originalProperty.enumerable };

// write
if (originalProperty.set || originalProperty.writable) {
    newProperty.set = function(val) {
        if (enabled)
            debugger;

        if (originalProperty.set) {
            originalProperty.set.call(this, val) 
        } else {
            originalProperty.value = val;
        }
    }
} else if (mode !== 'read') {
    throw "This property is not writable, so only possible mode is 'read'.";
}

// read
newProperty.get = function(val) {
    if (enabled && mode === 'read')
        debugger;

    return originalProperty.get ? originalProperty.get.call(this, val) : originalProperty.value;
}

// ...

I am OK with proposed version as well though.

morganrallen commented 11 years ago

If mode is not read should newProperty.get be defined at all?

ashmind commented 11 years ago

I think yes if original property was a value property (which is the most common case). Wait, nevermind, it should always be defined since we replace the property.

tychio commented 11 years ago

It's shorter and no problem, but I think conditions should be simplified rather than line.The originalProperty.set is judged two times.

morganrallen commented 11 years ago

Apparently it does.