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

Use a single Gson instance #28

Closed
kilink opened this issue May 28, 2016 · 3 comments
Closed

Use a single Gson instance #28

kilink opened this issue May 28, 2016 · 3 comments

Comments

@kilink
Copy link
Contributor

kilink commented May 28, 2016

It looks like all of the objects in com.facebook.ads.sdk.* have the same static, synchronized method for constructing the Gson object:

synchronized /*package*/ static Gson getGson() {
    if (gson != null) {
      return gson;
    } else {
      gson = new GsonBuilder()
        .excludeFieldsWithModifiers(Modifier.STATIC)
        .excludeFieldsWithModifiers(Modifier.PROTECTED)
        .disableHtmlEscaping()
        .create();
    }
    return gson;
}

Gson instances are threadsafe, and therefore a single instance could be shared throughout the codebase. Maybe the APIContext can grow a method for getting the Gson instance? It would also make it easier for consumers to override the defaults.

@JiamingFB
Copy link
Contributor

Hi Kilink,

We use different Gson instance for each class because the Gson parsing settings might be different for different classes. Although it is not happening right now, we want to keep the flexibility for the future.

If you've got performance hit because of this, please let us know so that we can try to find a solution.

@kilink
Copy link
Contributor Author

kilink commented Jul 6, 2016

It confers other benefits apart from performance. Currently I can't customize the Gson instance whatsoever (e.g., to turn on pretty printing).

The Gson instance could still be moved to APIContext, and when the need for a customized instance for a specific type arises, then you could handle it either using the current approach in that specific place, or via a factory method on the context (maybe by exposing the GsonBuilder?).

As I mentioned in another issue, I will open a PR for this if you guys agree that it's a good idea.

@kilink
Copy link
Contributor Author

kilink commented Jul 6, 2016

Additionally, I think any class/type-specific behavior can be achieved with type adapters or custom serializers/deserializers, while still maintaining the single Gson instance.

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

No branches or pull requests

2 participants