-
Notifications
You must be signed in to change notification settings - Fork 181
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
Tappx changes - Backward compatible change of version #1222
Conversation
0d30cb3
to
a691967
Compare
String url = endpointUrl + host; | ||
if (!host.contains(endpoint)) { | ||
url += "/" + endpoint; | ||
} | ||
url += "?tappxkey=" + tappxkey; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have many concatenations in this method, can we use StringBuilder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snahornyi Sure, but that would look somewhere like this:
...
StringBuilder urlBuilder = new StringBuilder()
.append(endpointUrl)
.append(host);
if (!host.contains(endpoint)) {
urlBuilder.append("/").append(endpoint);
}
urlBuilder.append("?tappxkey=").append(tappxkey);
if (test != null && test == 0) {
int t = (int) System.nanoTime();
urlBuilder.append("&ts").append(t);
}
urlBuilder
.append("&v=").append(VERSION)
.append("&type_cnn=").append(TYPE_CNN);
String url = urlBuilder.toString();
try {
HttpUtil.validateUrl(url);
} catch (IllegalArgumentException e) {
throw new PreBidException("Not valid url: " + url, e);
}
return url;
...
Which I think is less cleaner than "old" approach with url += ...
I'm trying to say it is not required here - we're not concatenating strings in a loop or something.
Since it's a simple concatenation, Java compiler will do the trick for us. At least for now I don't see any problem with +=
approach.
If not - let me know :)
db94d78
to
e7da501
Compare
e7da501
to
545c3c0
Compare
uriBuilder.addParameter("type_cnn", TYPE_CNN); | ||
return uriBuilder.build().toString(); | ||
} catch (URISyntaxException e) { | ||
throw new PreBidException("Built invalid URL: ", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new PreBidException(String.format("Failed to build endpoint URL: %s", e.getMessage()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
private String buildUrl(String endpointUrl, String host, String endpoint, String tappxkey, Integer test) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And will be
uriBuilder.addParameter("v", VERSION); | ||
uriBuilder.addParameter("type_cnn", TYPE_CNN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets put all parameters creating together
uriBuilder.addParameter("tappxkey", tappxkey);
uriBuilder.addParameter("v", VERSION);
uriBuilder.addParameter("type_cnn", TYPE_CNN);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
545c3c0
to
c6584fa
Compare
uriBuilder.addParameter("type_cnn", TYPE_CNN); | ||
|
||
if (test != null && test == 0) { | ||
int t = (int) System.nanoTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls give reasonable names to variables.
go
repo PR: prebid/prebid-server#1777