Find bid duration in linear time #2805
Conversation
| assert.Equal(t, tc.expected.dur, newDur, tc.desc) | ||
| assert.Equal(t, tc.expected.err, err, tc.desc) | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks good and this change makes sense!
May you please modify existing applyCategoryMapping unit test to test this change in the main flow, for instance:
In TestCategoryMappingBidderNameNoCategories change next lines:
L3185: requestExt.Prebid.Targeting.DurationRangeSec = []int{30, 10, 25, 5, 20, 50}
then input bids (L3194 and 3195):
bid1_1 := ... &openrtb_ext.ExtBidPrebidVideo{Duration: 17} ...
bid1_2 := ... &openrtb_ext.ExtBidPrebidVideo{Duration: 8}, ...And then result asserts should be (L3217 and 3218):
assert.Equal(t, "10.00_20s_bidder1", bidCategory["bid_id1"], "Category mapping doesn't match")
assert.Equal(t, "12.00_10s_bidder2", bidCategory["bid_id2"], "Category mapping doesn't match")
exchange/exchange.go
Outdated
| } | ||
| } | ||
| if !madeSelection && len(a) > 0 { | ||
| err = errors.New("Bid duration exceeds maximum allowed") |
There was a problem hiding this comment.
Very minor nitpick: I think error messages are supposed to be entirely lowercase? I could be wrong on this
exchange/exchange.go
Outdated
| // findDurationRange returns the element in the array 'a' that is both greater than 'dur' and closest | ||
| // in value to 'dur' unless a value equal to 'dur' is found. Returns an error if all elements in 'a' | ||
| // are less than 'dur'. | ||
| func findDurationRange(dur int, a []int) (int, error) { |
There was a problem hiding this comment.
Could the array variable a have a more descriptive name? Or are you keeping it more general in case this function is re-used in another place? If that's the case, could this function be moved to utils.go?
There was a problem hiding this comment.
Renamed to durationRanges
We currently sort the
targeting.DurationRangearray in order to pick a duration that better resembles thebid.BidVideo.Duration. This results in O(nlogn) time and O(n) space because we copy thetargeting.DurationRangeslice before sorting. This pull requests modifies to get the same result in O(n) time and O(1) space.