HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Querying the Google ads API

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thegooglequeryingapiads

Problem

I have a method which relies heavily on object supplied by third party APIs. Below is my working code, is there any scope of improvement?

public LineItem getLineItem(
      String networkId, String lineItemId) throws ApiException_Exception {
    LineItem lineItem = null;
    session.setCode(networkId); 
    LineItemServiceInterface lineItemService = servicesInterface.lineItemService(dfpSession);
    StatementBuilder statementBuilder =
        new StatementBuilder()
            .where("id = " + lineItemId.trim())
            .orderBy("id ASC")
            .limit(StatementBuilder.SUGGESTED_PAGE_LIMIT);
    LineItemPage lineItemPage =
        lineItemService.getLineItemsByStatement(statementBuilder.toStatement());
    if (lineItemPage != null && lineItemPage.getResults() != null) {
      lineItem = lineItemPage.getResults().get(0);
    }
    return lineItem;
  }


Every type used above belongs to third party code. I also need to know the good strategy to handle exceptions?

Update 1

One frequent issue I encounter and I am sure many if us will is that there is usually references to the third party type LineItem is sprinkled everywhere in the code base. Is this a good practice to leave it like that or should I wrap that type into my own type and use that instead?

Reference code here.

Solution

SQL statement construction

Your third-party library do support bindings, so instead of .where("id = " + lineItemId.trim()), you should consider:

//  .where("id = " + lineItemId.trim())
    .where("id = :lineItem")
    .withBindVariableValue("lineItem", lineItemId.trim());


Early variable assignment

LineItem lineItem = null;
// ...
if (/* ... */) {
    lineItem = /* ... */;
}
return lineItem;


You usually should not need to declare the return result right at the start, perform the optional assignment, and then return from the method. You should just return below, without needing the lineItem variable:

// ...
if (/* ... */ ) {
    return /* ... */;
}
return null;

Code Snippets

//  .where("id = " + lineItemId.trim())
    .where("id = :lineItem")
    .withBindVariableValue("lineItem", lineItemId.trim());
LineItem lineItem = null;
// ...
if (/* ... */) {
    lineItem = /* ... */;
}
return lineItem;
// ...
if (/* ... */ ) {
    return /* ... */;
}
return null;

Context

StackExchange Code Review Q#122451, answer score: 4

Revisions (0)

No revisions yet.