Conversation
PavelLoparev
requested review from
anastasiia-kryshtop,
kdudina-smartling and
sl-pkryshtop
| <artifactId>guava</artifactId> | ||
| <version>18.0</version> | ||
| </dependency> | ||
| <!-- https://mvnrepository.com/artifact/org.projectlombok/lombok --> |
| @Getter | ||
| @Setter | ||
| @ToString | ||
| @AllArgsConstructor |
There was a problem hiding this comment.
Data lombok's annotation can be used instead of a combination of Getter,Setter,ToString
|
|
||
| private final HttpCommandExecutor httpCommandExecutor; | ||
|
|
||
| MarketoTriggerCampaignClient(HttpCommandExecutor httpCommandExecutor) { |
There was a problem hiding this comment.
why it isn't a public?
you can use @requiredargsconstructor lombok annotation
| public TriggerCampaignResult requestTriggerCampaign(int campaignId, | ||
| TriggerCampaignRequest.LeadId[] leadIds) | ||
| throws MarketoApiException { | ||
| return this.requestTriggerCampaign(campaignId, leadIds, |
| new TriggerCampaignRequest.Token[]{}); | ||
| } | ||
|
|
||
| public TriggerCampaignResult requestTriggerCampaign(int campaignId, |
There was a problem hiding this comment.
I'd have only one method:
TriggerCampaignResult requestTriggerCampaign(int campaignId, TriggerCampaignRequest triggerCampaignRequest)
and construct TriggerCampaignRequest outside client.
in your case, your split constructing TriggerCampaignRequest into 2 places, outside client your construct leadIds and tokens, inside client ( in RequestTriggerCampaign ) - wrap them in TriggerCampaignRequest
There was a problem hiding this comment.
well, maybe it is just the bad model for TriggerCampaignRequest.
and you need something like:
public TriggerCampaignResult requestTriggerCampaign(
int campaignId,
List<Integer> leadIds,
List<TriggerCampaignToken> tokens)
| throws MarketoApiException { | ||
| List<TriggerCampaignResult> triggerCampaignResults = httpCommandExecutor.execute( | ||
| new RequestTriggerCampaign(campaignId, leadIds, tokens)); | ||
| return triggerCampaignResults.get(0); |
There was a problem hiding this comment.
I don't know the context, but it is an unsafe call. check size isn't empty at least. the same in next method
| @Setter | ||
| @ToString | ||
| @AllArgsConstructor | ||
| public class TriggerCampaignRequest { |
There was a problem hiding this comment.
hm, it is confusing with RequestTriggerCampaign
There was a problem hiding this comment.
Request data (leads and tokens) should be moved to the appropriate command (RequestTriggerCampaignCommand)
|
|
||
| import java.util.List; | ||
|
|
||
| public class MarketoTriggerCampaignClient { |
There was a problem hiding this comment.
add some tests for the new client
|
|
||
| MarketoTokenClient getMarketoTokenClient(); | ||
|
|
||
| MarketoTriggerCampaignClient getMarketoTriggerCampaignClient(); |
There was a problem hiding this comment.
Naming convention of the Marketo and this sdk is not followed. I suggest to rename this client to SmartCampaignClient or CampaignClient. It will be able to be extended with other smart campaign endpoints.
| @Setter | ||
| @ToString | ||
| @AllArgsConstructor | ||
| public static class LeadId { |
There was a problem hiding this comment.
According Marketo https://developers.marketo.com/rest-api/endpoint-reference/lead-database-endpoint-reference/#/Campaigns/triggerCampaignUsingPOST
it should be separate domain class InputLead
| @Setter | ||
| @ToString | ||
| @AllArgsConstructor | ||
| public static class Token { |
There was a problem hiding this comment.
Like in the previous comment it should be separate domain class Token
| import lombok.ToString; | ||
|
|
||
| @Getter @Setter @ToString | ||
| public class TriggerCampaignResult { |
There was a problem hiding this comment.
Result of request trigger command should be Array of Campaign domain classes
kdudina-smartling
left a comment
kdudina-smartling
left a comment
There was a problem hiding this comment.
Looks like is deprecated request
5 participants
No description provided.