New GenerateKeywordIdeas example for the KeywordPlanService#49
New GenerateKeywordIdeas example for the KeywordPlanService#49
Conversation
[keywordideas] Change-Id: Ide39a516a945f51d96f9786f18a4ebddd826d1ad
| StringValue.of(ResourceNames.geoTargetConstant(locationId))); | ||
| } | ||
|
|
||
| // Add each keyword seed to the request's keyword seed list. |
There was a problem hiding this comment.
Can we have some more commentary on the different seed types perhaps? Maybe call each of the different seed methods separately?
There was a problem hiding this comment.
I reworked this to handle the three supported seed types:
- UrlSeed for just a page URL
- KeywordSeed for just a list of keywords
- KeywordAndUrlSeed for both of the above
| } else { | ||
| // Convert the list of keywords into a list of StringValues. | ||
| List<StringValue> keywordStringValues = | ||
| keywords.stream().map(keyword -> StringValue.of(keyword)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Nit: can replace map(keyword -> StringValue.of(keyword) with map(StringValue::of)
| throw new IllegalArgumentException( | ||
| "At least one of keywords or page URL is required, but neither was specified."); | ||
| } | ||
|
|
There was a problem hiding this comment.
Perhaps add a comment here explaining the oneof semantics.
There was a problem hiding this comment.
I modified the existing comment on lines 162-163
| "At least one of keywords or page URL is required, but neither was specified."); | ||
| } | ||
|
|
||
| if (keywords.isEmpty()) { |
There was a problem hiding this comment.
Might be clearer to reduce the nesting here, as in
if (keywords.isEmpty() && pageUrl != null) {
} else if (pageUrl != null) {
} else {
}
There was a problem hiding this comment.
You could also add the above error branch to bring the entire oneof handling into a single block.
There was a problem hiding this comment.
Reworked to reduce nesting. I did not include the error case here because if that's true, the example will throw an exception. I personally prefer separating out the error case, but LMK if you feel strongly about combining them.
There was a problem hiding this comment.
Yeah that's a good point, thanks.
nwbirnie
left a comment
nwbirnie
left a comment
There was a problem hiding this comment.
One minor comment, otherwise LGTM.
| "At least one of keywords or page URL is required, but neither was specified."); | ||
| } | ||
|
|
||
| if (keywords.isEmpty()) { |
There was a problem hiding this comment.
Yeah that's a good point, thanks.
| // Only keywords were specified, so use a KeywordSeed. | ||
| requestBuilder | ||
| .getKeywordSeedBuilder() | ||
| .addAllKeywords(keywords.stream().map(StringValue::of).collect(Collectors.toList())); |
There was a problem hiding this comment.
Duplicated code for the stream().map(), maybe make a local variable?
There was a problem hiding this comment.
Unfortunately, if I do that I'd want to move back to the 2nd level of nesting. With the current nesting layout, creating the local variable is unnecessary for the if case. I'm concerned that it will just confuse readers.
I like the current nesting you suggested, so I'd prefer to keep it with the tradeoff of a bit of duplication. Also, the duplication means the else if and else blocks are a bit more amenable to copy/pasting.
@AnashOommen , what do you think?
There was a problem hiding this comment.
I agree with Josh, I prefer the ease of copy-pasting over a bit of duplication here.
nwbirnie
left a comment
nwbirnie
left a comment
There was a problem hiding this comment.
Github is really misleading when there's an open comment with LGTM. Also with two reviewers it looks like you can submit with one LGTM.
Removing my LGTM, but the comment on duplicated code is the only open issue from my side. Over to @AnashOommen for review and approvals.
| "At least one of keywords or page URL is required, but neither was specified."); | ||
| } | ||
|
|
||
| if (keywords.isEmpty()) { |
There was a problem hiding this comment.
If I read this correctly, it looks like the keyword texts will always be at least ['INSERT_KEYWORD_TEXT_1', 'INSERT_KEYWORD_TEXT_2']?
In that case, this condition would never be true?
There was a problem hiding this comment.
These values are only used when the flags fail to parse (e.g. missing required, invalid number formats etc.)
Also this flag is optional, so specifying --locationId 123 --languageId abc should be sufficient to clear the value here.
4 participants
No description provided.