AreaFiftyLAN / toornament-client

A java api client for Toornament. Currently not under active development.
MIT License
1 stars 1 forks source link

Adding tournament with params #13

Closed BrentonPoke closed 7 years ago

BrentonPoke commented 7 years ago

Ok, checked and re-checked with multiple games. It works in my test program.

BrentonPoke commented 7 years ago

I just noticed I mis-named the branch. ¯_(ツ)_/¯

skamoen commented 7 years ago

I implemented something very similar for Matches already. That way you don't have to "guess" the parameters, but they'll be specified in the client already.

See here for the matches implementation: https://github.com/AreaFiftyLAN/toornament-client/blob/feature-matches/src/main/java/ch/wisv/toornament/concepts/Matches.java

The URIBuilder Matthijs mentioned is even cleaner, I'll change it for the Matches but you can combine it here :)

BrentonPoke commented 7 years ago

I can't find whichever package URIBuilder requires in Maven Central, so I can't use it. And javaFX's pair works just like C++'s std::pair, which is very simple.

Plus, I don't know if URIBuilder will know to use "?" to begin the parameters, then "&" for subsequent ones. This is why I didn't use okhttp3's HttpUrl.

BrentonPoke commented 7 years ago

Hold on; I tried something just now.

HttpUrl.Builder b = new HttpUrl.Builder();
       b.scheme("https")
.host("www.google.com")
.addQueryParameter("discipline","overwatch")
.addQueryParameter("country", "US");

This code resulted in this: "https://www.google.com/?discipline=overwatch&country=US" It's similar to URIBuilder for this purpose, so does everybody want to do this?

It should work in a simple for-loop.

skamoen commented 7 years ago

I just did this for the Matches branch as well, seems to work well and looks good.

Instead of a for loop, I'd prefer something like this: https://github.com/AreaFiftyLAN/toornament-client/blob/feature-matches/src/main/java/ch/wisv/toornament/concepts/Matches.java#L27

So you don't have to manually enter the parameter names.

BrentonPoke commented 7 years ago

You would have to have a much larger call defined and still must know the allowable string values for some of the parameters. Depending upon how much the library is supposed to do out of the box, you'd have to hard-code into the library a lot of possible values as options for something like the tournaments endpoint. You could do that, but if so, that would likely mean a parameter options builder object.

Sent from my Verizon, Samsung Galaxy smartphone null

skamoen commented 7 years ago

I covered a lot of it already in the Matches branch I think. If you add your for-loop implementation in this PR, I'll add a predefined method after I finish the Matches branch, ok?

BrentonPoke commented 7 years ago

Just so we're clear, does this mean throwing out the Pair<String,String> as an input? Because the for-loop basically does this:

HttpUrl.Builder b = new HttpUrl.Builder();
       b.scheme("https");

for (Pair<String, String> params : paramsList) {
      b.host("www.google.com")
.addQueryParameter("params.L",params.R)
        }

What would the new call look like?

skamoen commented 7 years ago

I'd prefer not to have a JavaFX package as a dependency. It might just be a single class, but it's not meant for this type of thing. How about a simple HashMap<String, String>? You can also iterate over that as well with

for (Map.Entry<String, String> entry : map.entrySet()) {
   b.addQueryParameter(entry.getKey() , entry.getValue())
}
BrentonPoke commented 7 years ago

That's fine; I just want to know what the function prototype would look like. Because you're still going to have a build a HashMap, and to do that, will need to know what the parameters for the endpoints are.

skamoen commented 7 years ago

You mean this?

public List<Tournament> getTournamentsWithParams(Map<String, String> parameterMap) {
for (Map.Entry<String, String> entry : parameterMap.entrySet()) {
   b.addQueryParameter(entry.getKey() , entry.getValue())
}

Sorry if I'm misunderstanding your question. It's been a long day of coding for me :sleeping:

BrentonPoke commented 7 years ago

Oh, well that's basically what I have now. I can make that change. I thought you wanted to do something that defined every parameter option in the API with a method.

skamoen commented 7 years ago

That's what I was trying to say in https://github.com/AreaFiftyLAN/toornament-client/pull/13#issuecomment-320470151. In the Matches branch, I already created a number of enums and date formatters to deal with the parameters. Once I'm done with that branch, I'll re-use those classes to create a similar method for Tournaments.

The signature would be something like this: getTournament(String discipline, TournamentStatus status, Boolean featured, Boolean online, String countryCode, Date startAfter, Date startBefore, Date endAfter, Date endBefore, Sort sort, String name)

They'd be all nullable so you can create a query using only the parameters you want.

BrentonPoke commented 7 years ago

I think you can do that, but I interpreted a path segment as a URL segment

skamoen commented 7 years ago

Doesn't really matter anyway. I'm currently working on the Matches parameter thing, and I think you're right that using a parameter builder object is better than a method containing all parameters. PR should be done today :)