-
Notifications
You must be signed in to change notification settings - Fork 732
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
Displayio: Make imp.bidfloor optional #3959
Conversation
Code coverage summaryNote:
displayioRefer here for heat map coverage report
|
adapters/displayio/displayio.go
Outdated
if impression.BidFloorCur != "USD" { | ||
convertedValue, err := requestInfo.ConvertCurrency(impression.BidFloor, impression.BidFloorCur, "USD") | ||
if impression.BidFloor != 0 { | ||
convertedValue, err := requestInfo.ConvertCurrency(impression.BidFloor, impression.BidFloorCur, "USD") | ||
|
||
if err != nil { | ||
errs = append(errs, err) | ||
continue | ||
if err != nil { | ||
errs = append(errs, err) | ||
continue | ||
} | ||
|
||
impression.BidFloor = convertedValue |
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.
convertedValue, err := requestInfo.ConvertCurrency(impression.BidFloor, impression.BidFloorCur, "USD")
if err != nil {
errs = append(errs, err)
continue
}
impression.BidFloor = convertedValue
impression.BidFloorCur = "USD"
}
Combining the two checks (BidFloor != 0 and BidFloorCur != "USD") into a single if block makes the code more concise and avoids excessive nesting. This improves readability and understanding of the logic because: It expresses in one condition that currency conversion only occurs when the currency is not USD and BidFloor is not zero.
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.
Hi @przemkaczmarek, thanks for your comment, I'm absolutely agree with your point but the one thing is disturbing me from doing that: https://github.com/prebid/prebid-server/pull/3959/files#diff-8602d2a822defde21ab7f6ced0600ed1c8f36a4faa1278aa67454d388eef17b5R56 . Even if bidfloor is 0 we have to convert (replace in that case) bidfloorcur to USD (as the only supported currency by our server) so I have to use nested if or write another one for this case
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.
Hi @xdevel, thanks for pointing that out – I understand now that BidFloorCur needs to be set to "USD" regardless of whether BidFloor is zero or not.
Based on this requirement, here’s the updated code I propose to ensure BidFloorCur is set to "USD" and currency conversion only occurs if BidFloor is greater than zero:
convertedValue, err := requestInfo.ConvertCurrency(impression.BidFloor, impression.BidFloorCur, "USD")
if err != nil {
errs = append(errs, err)
continue
}
impression.BidFloor = convertedValue
impression.BidFloorCur = "USD"
} else if impression.BidFloorCur != "USD" {
impression.BidFloorCur = "USD"
}
Its up to You witch solution You choose.
Code coverage summaryNote:
displayioRefer here for heat map coverage report
|
No description provided.