Friday, May 1, 2015

Double Trouble: Two risky features come together for a conflict in JavaScript

On the value of being careful with modifying global scope. There is a code conflict I encountered recently when combining somebody else's JavaScript code with a 3rd party library. It didn't go well, and took time code-digging / stepping-through / debugging / tracing, until finally the cause became apparent.

JavaScript in general is very flexible (beyond loose), which is reason to both love it (“With great power…”) and hate it (“…comes great responsibility”). But blaming one's tools isn't helpful when you need to get a job done.

These are the two things that caused the conflict. Both are discouraged, but you won't understand why until you see them together.

Array.prototype.[method]

There is a very powerful ability in JavaScript to modify a prototype (a sample Object template similar to class in OOP languages) by adding new methods:
MyClass.prototype.newMethod = function() {
   // New method for "class" MyClass
}
Then you can call this method on any object of MyClass. Sweet.

Yet what then seems extra-powerful, is the ability to do the same with system classes. And, since (almost) everything is an object in JavaScript, you can add your own methods to String, Date, RegExp. This could really help locally improve readability of your code, since instead of getPresidentNames(getAllNames()) you could call getAllNames().getPresidentNames(), potentially chaining operations instead of matrushka-dolling them.

But modifying system objects like this is discouraged, because the change is global. Your added method will be visible to all of your code (which, in our day and age, 90% is made of code not written by you).

In my case, Array object was expanded with .max() and .min() methods. This sounds useful, but…

Array property iterator

Here's another convenient feature—you can iterate over all of the object's properties:
for(var prop in myObject) {
   console.log("myObject has property " + prop);
}
Very powerful, though potentially dangerous. If you run this in a controlled environment, where every single line of code was written by you, you'll get the list of properties that you've yourself defined. It is a very nice and expected result.

But what if someone redefined the object elsewhere?

Custom objects—not a problem, especially if you use closures.

System objects—not a problem… until somebody decides to add a method. Then you get list of properties and methods, including the newly added one.

For arrays, iterating with for(… in …) goes through the list of existing array indices. If you add methods—it will include them too. E.g. the list of values will be "1", "2", "newMethod".

The painful part (and that makes it hard to detect these kinds of issues) is that even if you make the change inside a closure, you're still modifying a global object, so you'll affect all of your code. Here's a simple Fiddle demonstrating this:

What should we do?

Two options.

None of the above features are really damaging by itself. Your code will work fine if you do either. So, when a conflict happens, you can play the blame game (Don't you expand prototypes! No, don't you iterate over arrays as objects!).

But that's not very productive.

On the other hand—you can understand that both practices are risky, since you don't always have control over all the code. So just avoid these.

For methods on global objects it is probably safer to use functions, if you really need to. They don't look as nice, but they are local to your closure, so—safe.

For iterating over arrays, there are other methods (for(i = 0; i < Array.length; i++), or Array.forEach()), whatever suits you. Or, if you are so adamant on using for(… in Array), then make sure to type-check to exclude functions (at least!).

You'll figure it out :)

4 comments:

  1. >>JavaScript in general is very flexible (beyond loose), which is reason to both love it (“With great power…”) and hate it (“…comes great responsibility”).

    I would not agree with your allusion to "power" here. JavaScript is not powerful. It is, as you said, loose - almost to the point of machine code. As to the two features - I would say the "for(...in...)" usage for enumerating an array is actually a bug. Prototype modification is actually good (if coder really understands what prototype inheritance is and how to use it).

    ReplyDelete
    Replies
    1. I like JS a lot, though :)

      But mainly for what it allows me to do, not for how it does it. I love to see pages come alive under my fingertips. If there was another as widespread language with the same power—I would probably love it too, and would forgive it a lot of missteps.

      (yes, it is extremely loose, and often annoyingly so :))

      As to your judgement—sorry, but for(…in…) is correct if you understand what it does. It should go over all properties of an object, and Array is an object. I would actually say that using for(…in…) on an Array generally comes from misunderstanding how it works.

      With prototype modification—yes, it is a very neat feature, if you're aware of affecting global scope. Which falls under your broad statement of “know how to use it”, which could be said about anything in life :)

      Delete
    2. What I meant is that for(...in...) is a bug if user meant iteration over all "elements" of the array. Am I wrong?

      About prototype modification - I agree that modifying prototypes of base library objects is not a good practice. But in general it's one of proper legal ways to implement what you described, although very brittle and potentially conflicting with code of others. But it's proper. While iterating over array "elements" using for(..in..) is just a bug - wrong implementation for given task. So one is a bug, another is just bad practice - I do not understand why you place them on the same level and saying it's just a "conflict of two risky features"?

      Delete
    3. Oh, you mean bug of someone who uses for(…in…), not bug of the language! Gotcha :)

      Agreed, if I were to choose which is worse, I'd say iterating over array's properties isn't a good idea. But I've seen it used fairly often, and it is hard to prove to someone that it is not a good idea, since “it works”. If it were me, I'd just add it to a code style guide—“don't do it”.

      Modifying global prototypes “works” too, but I started disliking it as well (I did my share of String.prototype.this-and-that in early days when I discovered this feature :)).

      You're correct that they are not equally “bad”, but I don't intend to measure them. For me it is an interesting case that I encountered where two seemingly “working” features clash. Makes one pay attention to squashing “bugs” and avoiding bad practices.

      Delete