There has been some discussion on whether to have the JS helper functions in a separate file and what functions to have.

Comments

zzolo’s picture

zzolo commented

See http://drupal.org/node/589642#comment-2089116 by tmcw

One thing I'd note about the patch: I'm in favor of removing the isSet function and replacing it with just

variable === undefined

Which is about the same length, character-wise as the function and is much clearer about how it works. I really think that the openlayers.helper.js functions should be completely eliminated, because they either eliminate more idiomatic javascript (like isSet), do things that jQuery does better (like triggerCustom), or do things you could easily do more quickly in the few calls that they have (like getObject).

zzolo’s picture

zzolo commented

I think some of this hinges on the agreed namespace we go with: #589548: 2.x: JS Functions Namespace and Syntax

As far s OL.isSet, this is much easier than writing typeof variable == 'undefined' (see Coding Standards for typeof, which is used very often considering how JS handles working with undefined variables. "OL.isSet" is even shorter than the word "undefined".

I do agree about using Jquery properly for events. Started issue: #591252: Custom Event System?

As for getObject(). Again, if you look at the code for this function, it would be silly to replicate this every time we need to actually call a function that is named by a string (specifically one that is nested in an object). I just don't see the logic of removing this.

I don't care too much about having this functions in a separate file, but to me it just makes things easier and seems logical.

rsoden’s picture

rsoden commented

I have sort of an irrational opposition to convenience functions that don't actually end up saving very much code. Everything discussed here fits into that category. I just find they muddle things and any gains from writing less characters are outweighed by increasing the amount of time it takes for people new to the module to figure out what's going on. I don't feel super strongly about this, it's more of an "icky" thing. Just my .02.

tmcw’s picture

tmcw commented

As far s OL.isSet, this is much easier than writing typeof variable == 'undefined' (see Coding Standards for typeof, which is used very often considering how JS handles working with undefined variables.

This is part of the problem: I'm not suggesting the use of typeof, because that would be a waste when dealing with undefined variables. Equality with undefined (no quotes, it's a reserved word) is the way to check whether a variable is defined in javascript - not checking the type of the object to see if it's undefined. isSet is probably an attempt to port isset() to javascript instead of learning javascript.

And looking at the usage of getObject, it looks like the only thing it can do is stuff we don't want to do (evading scope and functions like data()), and it only does them in one behavior.

zzolo’s picture

zzolo commented
Title: 2.x: JS Helper Functions » 2.x: Remove JS Helper File and Migrate Code

Well it seems like it was a misunderstanding for us on how to write correct JS code. A link provided by tmcw in IRC:
http://eloquentjavascript.net/chapter2.html#key80

Yes, it definitely makes sense to use variable == defined, not OL.isSet if we are checking to see if variables exists.

As for getObject, it does look like the adding features control uses, but we can just stick that function in there if we need to.

The only one not discusses is parseRel which I don't think we need anymore, but someone should check. If it is in there, we should definitely look at removing it. I believe I moved the CCK stuff off of it a while ago.

So, I am changing the name of this ticket and putting these action items:

* Remove any use of OL.isSet; this will be addressed a lot as code is migrated over.
* Address OL.getObject when we get to updating the other behaviors
* Remove helper file

phayes’s picture

phayes commented

the parseRel should be removed in favour of the jQuery data method - http://docs.jquery.com/Internals/jQuery.data

We need to go through the code to make sure we still aren't storing variables in the rel tag.


isSet is probably an attempt to port isset() to javascript instead of learning javascript.

I have to accept blame isSet and parseRel - they are my creations. I take a bit of offense at the suggestion that were implemented "instead of learning javascript", but I do admit they were armature implementations from someone who was still in the process of *learning* javascript.

tmcw’s picture

tmcw commented

er, yeah, that was a little harsh regarding the approach to javascript there. I've massacred the javascript scope system elsewhere, and was projecting my shame.

tmcw’s picture

tmcw commented
Status: Active » Closed (fixed)

openlayers.helper.js removed in 285944.