search

masecla22 / Reddit4J

A library which aims to attain full coverage of the Reddit API
MIT License
48 stars 21 forks source link

Implement automatic RedditData unpacking #6

Open Arnuh opened 2 years ago

Arnuh commented 2 years ago

Will handle deserializing a RedditData with none or minimal manual TypeToken creation and returning the specified generic type of the RedditData data field. PR also contains a ton of gson creation cleanup that is horrible for performance.

Everything changed should be tested but I may have missed some.

I also looked up eclipse import formatting and applied it to an intellij profile causing imports from other PRs being moved around

masecla22 commented 2 years ago

I'm aware the PR is taking a long time to review, don't worry, I'm still active.

It's just really huge!

masecla22 commented 2 years ago

Ok, I finally finished reviewing this.

The only thing I'm not quite sure about is the use of a class RedditUtils where we export something publicly.

Especially considering that we are a library rather than a project, it might be confusing / polluting to expose things like that publicly.

I'd love some more feedback on this, but I think sealing it inside the Reddit4J class, and exposing it through a getter would make far more sense.

Arnuh commented 2 years ago

Either option works, not sure what kind of design this library is going for.

One issue is then you have to share the Reddit4J object everywhere, wont be an issue in almost all cases but RedditObject#toString currently converts the object to json

masecla22 commented 2 years ago

Alright, so once we restructure this to sketch out RedditUtils, I'm more than happy to merge this.

Arnuh commented 2 years ago

Alright, so once we restructure this to sketch out RedditUtils, I'm more than happy to merge this.

Do I just remove the toString override that calls RedditUtils.gson or am I suppose to set a RedditClient into every RedditObject made

masecla22 commented 2 years ago

Although it sounds a little excessive, I think having a reference to the parent at all times might be useful.

I'm open for discussion though, since it feels like I might be missing something.

Arnuh commented 2 years ago

Only downside I can really is it can possibly end up making a mess in codebase. Might be able to make the json parser auto set the variable to prevent that.

The developer should always have the object and wouldn't need to grab it from a RedditThing object. And I see no real use of wanting toString to return a json result instead of information like the unique id.

Can still move Gson into the Reddit4J class if toString is not handled.