2
\$\begingroup\$

Algorithm that I had to write is a perfect case for while, or do..while loop, however I found out that if I will implement it with a for loop I will save few lines of code and also scope of variables will be more appropriate. Take a look at the following code:

        for ( var i = 1, offset = -1; currentPosition.top === fakePosition.top && offset !== 0; i++ ) {
            fakePosition.top -= i * 10;
            offset = this.documentView.getOffsetFromPosition( fakePosition );
            fakePosition = this.documentView.getRenderedPosition( offset );
        }

As you can see second parameter is very atypical as for for loops. My mine reason to switch to this loop was fact that I need iterator (i) and offset variables inside loop (and only there).

What do you think about this approach?

\$\endgroup\$
  • 1
    \$\begingroup\$ Too much magic in the loop condition. \$\endgroup\$ – Raynos Oct 21 '11 at 19:16
  • \$\begingroup\$ @Raynos What exactly do you mean? I just see two simple conditional statement (for variables, not even method/function calls). \$\endgroup\$ – Inez Oct 21 '11 at 19:36
2
\$\begingroup\$

As you can see second parameter is very atypical as for for loops

Which is why it shouldn't be a for loop, because as you write yourself(!) "[it] is a perfect case for while, or do..while loop". Saving a few lines of code and variable scope is not a good reason to obscure the code. You will (or should) be losing any perceived time/line benefit in writing the comments necessary to explain your trickery. If you want to limit variable scope, declare a new scope or function.

As @Raynos alludes to in the comments, you should avoid "magic", or rather, complicated expressions in your for loops (and other conditional statements). It might not look bad to you now (though it probably will in X months), but consider what happens when your boss/client asks you to add support for a special case. Very quickly it'll look like this:

    for ( var i = 1, offset = -1; currentPosition.top === fakePosition.top && offset !== 0 && currentPosition.top != -1 /* browser XYZ gives bogus answer */ && offset < fakePosition.bottom /* handle condition explained in ticket #123 */; i++ ) {
        fakePosition.top -= i * 10;
        offset = this.documentView.getOffsetFromPosition( fakePosition );
        fakePosition = this.documentView.getRenderedPosition( offset );
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks! I refactored it to look like this: pastebin.ca/2092328 But I'm still not happy with it - especially that it has 'return' inside loop body. \$\endgroup\$ – Inez Oct 21 '11 at 20:34
  • \$\begingroup\$ Sometimes code just has to stay "ugly", but clear, until you figure out the right way of being clever where you actually reduce complexity rather than increase it. In this case I have to say good luck :) \$\endgroup\$ – user786653 Oct 21 '11 at 20:49
1
\$\begingroup\$

The for command is great for simple loops, as it makes the code easier to read when the loop follows a common pattern. For more complex loops you lose that advantage, so you should use a construct that better describes what the loop does.

scope of variables will be more appropriate

No, it won't. It doesn't matter where in a function you declare a variable, the scope is the entire function.

You can even declare variables below the code where you use it, they will actually be created when you enter the function. Example:

function test() {
  alert(x); // shows undefined
  x = 42;
  alert(x); // shows 42
  var x;
  alert(x); // shows 42
}

// global scope:
var x = 1337;
alert(x); // shows 1337
test();
alert(x); // shows 1337
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.