Advanced interstitial support #127
Conversation
…lear a json from empty values
| try { | ||
| for (int i = 0; i < jsonArray.length(); i++) { | ||
| if (i != pos) { | ||
| jsonArray.put(jsonArray.get(i)); |
There was a problem hiding this comment.
This should be result.put(jsonArray.get(i));
| HashMap<String, Object> additionalMap = new HashMap<>(1); | ||
| additionalMap.put(RequestParams.INSTL_MIN_SIZE_PERC_KEY, minSizePerc); | ||
|
|
||
| RequestParams requestParams = new RequestParams(configId, adType, sizes, keywords, additionalMap); |
There was a problem hiding this comment.
Why do we need an additionalMap for passing the minSizePerc when currently we have only one object to pass to the RequestParams? This HashMap is certainly introducing overheads as well.
There was a problem hiding this comment.
Extensibility. I would like to eliminate passing a lot of optional arguments into constructor because it creates unnecessary state and makes initializing process overhead. I am sure that the RequestParams class will be extended in a future by adding new features
By the way, I have a plan to improve this logic soon and provide you a PR
There was a problem hiding this comment.
Hi @yoalex5,
I am not in support of having HashMap for just one parameter. But yes, you can create a PR for refactoring the RequestParams as a whole and maybe have all the parameters attached to the HashMap. Or you can come up with a better approach.
There was a problem hiding this comment.
@avohraa, are you going to approve this PR or you are waiting for changes from my side ?
There was a problem hiding this comment.
I am still waiting for your changes.
| Integer minSizePercWidth = null; | ||
| Integer minSizePercHeight = null; | ||
|
|
||
| Map<String, Object> additionalMap = requestParams.getAdditionalMap(); |
There was a problem hiding this comment.
Passing an instance of AdSize eliminates this processing overhead.
2. small refactoring 3. Unit tests were updated
|
@avohraa, could you please review the latest changes |
…ial_advanced2 # Conflicts: # PrebidMobile/API1.0/src/test/java/org/prebid/mobile/PrebidServerAdapterTest.java # PrebidMobile/API1.0/src/test/java/org/prebid/mobile/UtilTest.java
|
@avohraa, I have merged the master into current branch. Could you please review it? |
GitHub issue
InterstitialAdUnit(String, int, int)was addedAPI:
Example:
OpenRTB request:
request.device.ext.prebid.interstitial: {"minwidthperc": 50, "minheightperc": 70}TODO: