-
Notifications
You must be signed in to change notification settings - Fork 760
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
GumGum: adds slot param #1949
GumGum: adds slot param #1949
Conversation
PR for updates to prebid docs: prebid/prebid.github.io#3185 |
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.
Left a nitpick. I'm totally ok if you don't agree, I'm ready to approve either way.
@@ -146,6 +146,34 @@ func preprocess(imp *openrtb2.Imp) (*openrtb_ext.ExtImpGumGum, error) { | |||
format := bannerCopy.Format[0] | |||
bannerCopy.W = &(format.W) | |||
bannerCopy.H = &(format.H) | |||
|
|||
if gumgumExt.Slot != 0 { |
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.
In order to keep the preprocess(imp *openrtb2.Imp)
function short, can the newly added functionality be grouped into a function of its own? Maybe a getBiggerFormat(formatList []openrtb2.Format, slot float64) openrtb_ext.ExtImpGumGumBanner
could help.
diff --git a/adapters/gumgum/gumgum.go b/adapters/gumgum/gumgum.go
index 4d375af3..9688854a 100644
--- a/adapters/gumgum/gumgum.go
+++ b/adapters/gumgum/gumgum.go
@@ -148,26 +148,8 @@ func preprocess(imp *openrtb2.Imp) (*openrtb_ext.ExtImpGumGum, error) {
bannerCopy.H = &(format.H)
if gumgumExt.Slot != 0 {
- maxw := int64(0)
- maxh := int64(0)
- greatestVal := int64(0)
- for _, size := range bannerCopy.Format {
- var biggerSide int64
- if size.W > size.H {
- biggerSide = size.W
- } else {
- biggerSide = size.H
- }
-
- if biggerSide > greatestVal || (biggerSide == greatestVal && size.W >= maxw && size.H >= maxh) {
- greatestVal = biggerSide
- maxh = size.H
- maxw = size.W
- }
- }
-
+ bannerExt := getBiggerFormat(bannerCopy.Format, gumgumExt.Slot)
var err error
- bannerExt := openrtb_ext.ExtImpGumGumBanner{Si: gumgumExt.Slot, MaxW: float64(maxw), MaxH: float64(maxh)}
bannerCopy.Ext, err = json.Marshal(&bannerExt)
if err != nil {
return nil, err
@@ -197,6 +179,28 @@ func preprocess(imp *openrtb2.Imp) (*openrtb_ext.ExtImpGumGum, error) {
return &gumgumExt, nil
}
+func getBiggerFormat(formatList []openrtb2.Format, slot float64) openrtb_ext.ExtImpGumGumBanner {
+ maxw := int64(0)
+ maxh := int64(0)
+ greatestVal := int64(0)
+ for _, size := range formatList {
+ var biggerSide int64
+ if size.W > size.H {
+ biggerSide = size.W
+ } else {
+ biggerSide = size.H
+ }
+
+ if biggerSide > greatestVal || (biggerSide == greatestVal && size.W >= maxw && size.H >= maxh) {
+ greatestVal = biggerSide
+ maxh = size.H
+ maxw = size.W
+ }
+ }
+
+ return openrtb_ext.ExtImpGumGumBanner{Si: slot, MaxW: float64(maxw), MaxH: float64(maxh)}
+}
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.
thanks @guscarreon ! I've updated the PR to move it into a getBiggerFormat
func - please let me know if there are any other changes/suggestions you might have
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.
Thank you @susyt
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.
LGTM
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.
I agreed with Gus' comment about creating that getBiggerFormat
function, but i think that looks great! Apart from that, LGTM!
allows publishers to optionally send in a slot param
update to prebid doc to come