tags:

views:

417

answers:

3

My javascript is pretty nominal, so when I saw this construction, I was kind of baffled:

var shareProxiesPref = document.getElementById("network.proxy.share_proxy_settings");
shareProxiesPref.disabled = proxyTypePref.value != 1;

Isn't it better to do an if on proxyTypePref.value, and then declare the var inside the result, only if you need it?

(Incidentally, I also found this form very hard to read in comparison to the normal usage. There were a set of two or three of these conditionals, instead of doing a single if with a block of statements in the result.)


UPDATE:

The responses were very helpful and asked for more context. The code fragment is from Firefox 3, so you can see the code here:

http://mxr.mozilla.org/firefox/source/browser/components/preferences/connection.js

Basically, when you look at the "Connect" preferences window in Firefox, clicking the proxy "modes" (radio buttons), causes various form elements to enable|disable.

+1  A: 

(Incidentally, I also found this form very hard to read in comparison to the normal usage.

Not necessarily, although that was my first thought, too. A code should always emphasize its function, especially if it has side effects. If the writer's intention was to emphasize the assignment to sharedProxiesPref.disabled then hey, roll with it. On the other hand, it could have been clearer that the action taking place here is to disable the object, in which case the conditional block would have been better.

Konrad Rudolph
+1  A: 

It depends on the context of this code. If it's running on page load, then it would be better to put this code in an if block.

But, if this is part of a validation function, and the field switches between enabled and disabled throughout the life of the page, then this code sort of makes sense.

It's important to remember that setting disabled to false also alters page state.

pottedmeat
A: 

It's hard to say what's better to do without more context.

If this code being executed every time that proxyTypePref changes, then you're always going to need set shareProxiesPref.disabled.

I would agree than an if statement would be a bit more readable than the current code.

Isn't it better to do an if on proxyTypePref.value, and then declare the var inside the result, only if you need it?

If you're talking strictly about variable declaration, then it doesn't matter whether or not you put it inside an if statement. Any Javascript variable declared inside a function is in scope for the entire function, regardless of where it is declared.

If you're talking about the execution of document.getElementById, then yes, it is much better to not make that call if you don't have to.

17 of 26