Refactor HTTP `Query`'s

neopixeldev / hypixel-api-java

Asynchronous Java wrapper for the Hypixel API

MIT License

6 stars 2 forks source link

Refactor HTTP `Query`'s #101

Closed unpluggedsam closed 2 years ago

unpluggedsam commented 2 years ago

This PR will refactor the way Query's are used to communicate with the API.

⚫ Query

Made Query not abstract.
Made constructor take a Endpoint and a array of QueryParameter's.
createRequest() uses the array of QueryParameter's to add parameters to the request.

⚫ QueryParameter

Record used to keep track of the query parameters.
Has a key (HypixeQueryParameterType), and a value (String).

⚫ HypixelQueryParameterType

Used to enforce the correct parameter key.

⚫ RequestController

Added method to instantiate a Query and send it to the RequestFactory.
Add methods to get specific data.
Initialize caching in RequestController instead of RequestFactory.

Notes:

These commits will not practically change anything, it just makes it much easier to create new requests to the API which will be nice when we need to implement Skyblock.

Nopock commented 2 years ago

Hello Sam, I think it looks good with the queries but just wondering why we have Optionals since in a previous PR I believe we switched away from them.

unpluggedsam commented 2 years ago

Hello Sam, I think it looks good with the queries but just wondering why we have Optionals since in a previous PR I believe we switched away from them.

So I was actually meaning to bring this up. The point of the optional is to handle the possibility of the player not being in a guild. The other option is to just throw an exception. I was going to look for more input from you guys.

Nopock commented 2 years ago

Hello Sam, I think it looks good with the queries but just wondering why we have Optionals since in a previous PR I believe we switched away from them.

So I was actually meaning to bring this up. The point of the optional is to handle the possibility of the player not being in a guild. The other option is to just throw an exception. I was going to look for more input from you guys.

I feel like being consistent with either Optionals or nulls is important but I agree that we should discuss this as a group.

Mqlvin commented 2 years ago

Hello Sam, I think it looks good with the queries but just wondering why we have Optionals since in a previous PR I believe we switched away from them.

So I was actually meaning to bring this up. The point of the optional is to handle the possibility of the player not being in a guild. The other option is to just throw an exception. I was going to look for more input from you guys.

Providing users with errors to handle as well as Optional's may be complicated, however it would be the most explainable way to deal with a problem like that. I agree we should have a conversation on best implementation.