Skip to content

Adds new AddRemarketingAction code example#235

Merged
jradcliff merged 4 commits intomasterfrom
add-remarketing-sample
Feb 6, 2020
Merged

Adds new AddRemarketingAction code example#235
jradcliff merged 4 commits intomasterfrom
add-remarketing-sample

Conversation

@jradcliff
Copy link
Copy Markdown
Member

Change-Id: Id77a16e759dc0d159148fcce2841aeb4cda5c4c9

Change-Id: Id77a16e759dc0d159148fcce2841aeb4cda5c4c9
Change-Id: I2699b47fcf739e77b2312891701486251383e82d
Copy link
Copy Markdown
Contributor

@devchas devchas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one minor comment.

try (GoogleAdsServiceClient googleAdsServiceClient =
googleAdsClient.getLatestVersion().createGoogleAdsServiceClient()) {
// Issues a search request.
SearchPagedResponse searchPagedResponse =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not use a SearchGoogleAdsRequest with page size information for this search request, is it intended?

I am asking because the golden has it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're only interested in one row from the result set, I don't see much value in creating a request object or setting the page size, and I think it hinders readability. Would prefer to leave this as-is unless you have strong objections.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

RemarketingAction newRemarketingAction = googleAdsRow.getRemarketingAction();
System.out.printf(
"Remarketing action has ID %d and name '%s'.%n%n",
newRemarketingAction.getId().getValue(), newRemarketingAction.getName().getValue());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more readable with one argument per line as done in other code examples.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the format produced by the Google Java formatter, so would prefer to leave this as-is. Otherwise, any manual edits I make will probably be undone by the next editor of this file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

@jradcliff jradcliff merged commit 39af4cc into master Feb 6, 2020
@jradcliff jradcliff deleted the add-remarketing-sample branch February 6, 2020 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants