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

Review Ix bidder #1372

Merged
merged 6 commits into from
Jul 21, 2021
Merged

Review Ix bidder #1372

merged 6 commits into from
Jul 21, 2021

Conversation

SerhiiNahornyi
Copy link
Collaborator

No description provided.

@SerhiiNahornyi SerhiiNahornyi requested a review from And1sS July 15, 2021 16:30
.h(banner.getH())
.build();
if (bidType == BidType.video) {
updateWithVideoAttributes(bidBuilder, bid.getExt(), bid.getCat());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls avoid passing of builder as an arguments.
Especially in this case, we won't affect the performance since the condition.

btw, we can use ternary operator for return statement.


private void updateWithNativeAttributes(Bid.BidBuilder bidBuilder, String adm) {
final NativeV11Wrapper nativeV11 = parseBidAdm(adm, NativeV11Wrapper.class);
final Response v11Response = getIfNotNull(nativeV11, NativeV11Wrapper::getNativeResponse);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls rename v11Response to responseV11.

return target != null ? getter.apply(target) : null;
}

private String updateAdm(Response response, boolean isV11) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest to inline this method.

.build();
if (bidType == BidType.video) {
updateWithVideoAttributes(bidBuilder, bid.getExt(), bid.getCat());
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if (bidType == BidType.xNative)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it is already checked above.

Serhii Nahornyi added 2 commits July 19, 2021 22:02
# Conflicts:
#	src/test/resources/org/prebid/server/it/openrtb2/ix/test-auction-ix-response.json
@rpanchyk rpanchyk merged commit 1f323ca into master Jul 21, 2021
@rpanchyk rpanchyk deleted the ix/review branch July 21, 2021 10:41
nickluck9 pushed a commit that referenced this pull request Aug 9, 2021
nickluck9 pushed a commit that referenced this pull request Aug 10, 2021
nickluck9 pushed a commit that referenced this pull request Aug 10, 2021
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

Successfully merging this pull request may close these issues.

3 participants