Why was index removed from Parameter?

[GoogleCodeExporter]

I just upgraded to as3commons-reflect-1.3.4 and found that the "index" property 
was removed from Parameter (which broke a bunch of my code).

This property was useful...  why was it removed?

Original issue reported on code.google.com by idontneedthisacct@gmail.com on 5 Oct 2011 at 7:58

[GoogleCodeExporter]

Okay, I see that r1085 has the comment:

"Removed index property from Parameter class since this can be easily derived
from the parent collection"

This comment is short-sighted.  While technically true, deriving the index is 
only practical in extremely limited situations (e.g., in the same scope where 
the parent collection is being enumerated).

In some designs, knowing a parameter's index (an an object-oriented manner) may 
be just as important as knowing its name.  We probably wouldn't think of 
removing the "name" property simply because Parameters were to be exposed in a 
Dictionary, because the name "can be easily derived from the parent 
collection", right?  Not, because it make sense to have the ability (in any 
context) to simply ask the Parameter for its name.  IMO, the same applies to 
index.

In my case, because of this short-sighted change to the API, I now have to pass 
an additional index value through many layers of code (adding it to numerous 
function signatures, making my code more verbose and fragile) where previously 
all I had to do was pass a nice, self-contained Parameter instance that could 
be asked for its index.

Would you please consider adding the index property back?  The change to remove 
it seems to have been trivial.

Original comment by idontneedthisacct@gmail.com on 5 Oct 2011 at 10:11

[GoogleCodeExporter]

Hi there,

The comment for the commit is incomplete, what it needed to read was this:
"Removed index property from Parameter class since this can be easily derived
from the parent collection. Parameter instances are now cached based on 
typeName and isOptional properties to prevent thousands of instances eating up 
memory. If the index was left in it would greatly increase the amount of 
Parameter instances."

We have been busy optimizing the memory consumption of as3-reflect since this 
was becoming a much heard complaint. We chose therefore to cache Metadata, 
MetadataArgument and Parameter instances that were equal in property values. 
This was the reason the index property was sacrificed, otherwise caching the 
Parameter instances would have become redundant.

So, I'm not sure if I agree with your repeated accusation of 
'short-sightedness'. I understand your frustration though, but please 
understand that we sometimes have to make choices. We prefer not to change an 
API, but in this case it seemed a valid one since it could greatly reduce the 
memory footprint of the library. Please consider the fact that in the old 
situation Parameter instance numbers could easily rise to thousands, if not 
tens of thousands. In many cases we profiled applications and saw that the Type 
information was consuming large amounts of memory. Especially now that mobile 
apps are getting more and more in the picture, memory consumption is becoming 
much more important because of the memory restrictions of those platforms.

We are now at version 1.5 and you are the first one to come in and complain 
about this. While I appreciate where you're coming from, I'm not sure if your 
particular case is a valid enough reason to revert.

I hope this explanation does make it clear though that this change was 
certainly not 'trivial'.

cheers,

Roland

Original comment by ihatelivelyids on 5 Oct 2011 at 11:03

[GoogleCodeExporter]

Sorry, I didn't mean to sound accusatory.  Just trying to make clear that this 
seemingly simple API change impacted a lot of code that now needs to be 
re-worked.   Also, I just meant the change to the Parameter class seemed 
trivial (meaning, "Hey, it looks easily revertible").  Lacking your complete 
explanation, it really did seem like someone decided to make a frivolous change 
("Ah, this property doesn't seem necessary... ").

Anyway, I understand now that the change was to make the Parameter instances 
"context-free" (in order to avoid having many copies, saving memory).  Indeed, 
I have been one of those asking for more memory- and CPU-efficiency from this 
library. :) I understand the change now.  

That said, you could have continued to support the existing API by having 
Method.parameters return a new array of temporary "proxies" that wrap the 
underlying shared "parameter" information.  These proxies could add the 
appropriate context (index, etc.) for the particular Method, and would simply 
be garbage-collected when the client was done with them.

Original comment by idontneedthisacct@gmail.com on 6 Oct 2011 at 12:46

[GoogleCodeExporter]

Hi there,

now you're talking! :) Your suggestion for returning the proxy objects is 
actually a really good one, I hadn't thought about that one. I'll definitely 
implement this when I find the time. If you feel inclined and have a bit of 
spare time, I would welcome the contribution too.
If that way we can make sure your, and other people's, code remains more 
stable, I'm all for it!

cheers!

Roland

Original comment by ihatelivelyids on 6 Oct 2011 at 7:45

[GoogleCodeExporter]

Ow, and sorry about my initial grumpy reply. I had a long day yesterday, I 
wasn't necessarily trying to sound pissed off, but my apologies if I did :)

cheers,

Roland

Original comment by rol...@stackandheap.com on 6 Oct 2011 at 7:46

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Hi there,

well, I've implemented the parameter decorator. Parameter has now become 
BaseParameter and Constructor and Method now both return an Array of Parameters 
that wrap the actual BaseParameters.
So, your code ought to be working again. The changes are available in the 
trunk, perhaps you can give it a testdrive?

cheers,

Roland

Original comment by ihatelivelyids on 6 Oct 2011 at 5:33

• Changed state: Accepted

[GoogleCodeExporter]

I will close the issue for now. Should any other problems surface, please add a 
new ticket.

cheers,

Roland

Original comment by ihatelivelyids on 15 Oct 2011 at 10:39

• Changed state: Fixed