Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: static initialisers to avoid side effects in the JSON constructors #990

Merged
merged 2 commits into from
Apr 13, 2023
Merged

fix: static initialisers to avoid side effects in the JSON constructors #990

merged 2 commits into from
Apr 13, 2023

Conversation

sp00m
Copy link
Contributor

@sp00m sp00m commented Mar 29, 2023

As the title says :)

@sp00m sp00m requested a review from a team as a code owner March 29, 2023 13:44
@wboereboom
Copy link
Contributor

Hey @sp00m ,
Thanks a lot for contributing to our repository.
Could you please elaborate on the side effects you are trying to avoid?

Kind Regards,
Wouter
Adyen

@sp00m
Copy link
Contributor Author

sp00m commented Mar 29, 2023

Hey @wboereboom,

The previous code relied on an instance initializer block to set a static variable, which is a side-effect: as the block executes each time a new instance is created, each new instance of JSON will overwrite the static gson instance. That's also the reason why you had to have new JSON(); statements scattered a bit everywhere I suppose.

Using a static block instead will get rid of this oddity and ensure gson is set only once: when the class first loads (unless JSON.setGson(...) is explicitly called again obviously).

Hope that makes sense. It's rather minor for sure, but I just came across it and the fix was rather quick, so I thought why not contributing :)

@wboereboom wboereboom merged commit f2bf042 into Adyen:develop Apr 13, 2023
@wboereboom
Copy link
Contributor

Great contribution @sp00m!

Thank you very much for taking the time to provide this quality-of-life change for the Library 🎉

Kind Regards,
Wouter
Adyen

@sp00m sp00m deleted the json-static-initialiser branch April 13, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants