SempaiGames / haxe-ga

GoogleAnalytics Client API port to Haxe
Other
60 stars 23 forks source link

Stats.destroy #7

Closed delahee closed 10 years ago

delahee commented 10 years ago

Hi !

There is no ga.Stats destructor and we feel it should available since many usage patterns with android activities can mess with this.

We believe Variables should be null set, connection closed ( flushed ) and threads stopped.

Can you implement it ?

Thanks !

fbricker commented 10 years ago

Hi, why do you believe that? Stats class has static methods and variables. It does not create multiple instances of objects, and since you can call stats at any point during the program execution, the class should remain alive while the app is still alive. At least this is the idea of the Stats class, if you find any bug or piece of code that you consider is leaving trash behind it, please let me know and I'll try to fix it.

Thanks!

delahee commented 10 years ago

I don't like very much to leave potential things/thread behind me ( maybe its dued to my embed dev background ) ...

Here are some examples :

1- an android activity can entre the class from concurrent thread leaving the first in background, calling init another time with different data ( at least in haxe ).

2- I don't know how the thread will behave when the app will close too.

3- If we want to reuse the stat class the main thread reference will just be lost init(...) init(...) <- first thread has no longer reference and will stay stuck

Feel free to close this issue if you think it's pointless but a destroy func would not hurt :)

fbricker commented 10 years ago

Hi, I understand your concerns. But in this case the Stats class is an static class with static vars. So, there's no reference to any Stats object anywhere and stats class does not reference any external object also.

Also, if you call Init twice, the Stats ignores the second call, and keeps using the objects and threads generated at the first Init call.

About the thread, it'll be deleted when the OS closes the app (together with every variable from the Stats class).

I understand that a destroy function should not hurt, but if it's pointless, it just shouldn't be there (any extra line of code and function is a potential cause of bugs... so I don't want to add things that are not justified).

But I don't want to close this issue since I may be ignoring a real problem. If you can find a case failing case, I'll for sure add the necessary changes to avoid that case :)

Thanks!

delahee commented 10 years ago

Ok I didn't get that the class would not accept reentrency...

Let's keep that as is for simplicity... if I were you though I would throw an error still because concurrent activity could try to init them from multiple end points (android activity can do that). In this case there will be an unexpected data sent to potentially not the good ga account.

As for static class, they are still generating gc refs, static calls are just executed in different context of allocation but they still work with gc.

Bye :) Le 30 mai 2014 18:28, "fbricker" notifications@github.com a écrit :

Hi, I understand your concerns. But in this case the Stats class is an static class with static vars. So, there's no reference to any Stats object anywhere and stats class does not reference any external object also.

Also, if you call Init twice, the Stats ignores the second call, and keeps using the objects and threads generated at the first Init call.

About the thread, it'll be deleted when the OS closes the app (together with every variable from the Stats class).

I understand that a destroy function should not hurt, but if it's pointless, it just shouldn't be there (any extra line of code and function is a potential cause of bugs... so I don't want to add things that are not justified).

But I don't want to close this issue since I may be ignoring a real problem. If you can find a case failing case, I'll for sure add the necessary changes to avoid that case :)

Thanks!

— Reply to this email directly or view it on GitHub https://github.com/fbricker/haxe-ga/issues/7#issuecomment-44670654.