3
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Is your usage of StatementBuilder the recommended approach by the developers of that class? It looks ripe for SQL injection attacks... \$\endgroup\$
    – h.j.k.
    Mar 10, 2016 at 8:22
  • \$\begingroup\$ @h.j.k. seems yes, here is the reference. \$\endgroup\$
    – vivek
    Mar 10, 2016 at 8:55
  • \$\begingroup\$ But it looks like bindings are supported too, so you may want to try that out... \$\endgroup\$
    – h.j.k.
    Mar 10, 2016 at 9:11
  • \$\begingroup\$ You mean to use replace string concatenation with the bindings, right? \$\endgroup\$
    – vivek
    Mar 10, 2016 at 9:12
  • \$\begingroup\$ Yes, see answer below. \$\endgroup\$
    – h.j.k.
    Mar 10, 2016 at 9:21

1 Answer 1

4
+50
\$\begingroup\$

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;
\$\endgroup\$
5
  • \$\begingroup\$ Out of curiosity, what are the pros of this? \$\endgroup\$
    – vivek
    Mar 10, 2016 at 9:38
  • \$\begingroup\$ In the unlikely event you don't sanitize lineItemId, and that gets passed in as " 1; TRUNCATE TABLE ... ; --". \$\endgroup\$
    – h.j.k.
    Mar 10, 2016 at 10:08
  • \$\begingroup\$ I was curious about your second suggestion about lineItem. \$\endgroup\$
    – vivek
    Mar 10, 2016 at 11:32
  • 2
    \$\begingroup\$ @vivek it's recommended to reduce the live time of a variable: the lines between declaration and the end of their scope. A variable that's visible may be accidentally misused. It may also be noise, distraction. You don't need this variable, it's not useful, so it has no reason to exist. \$\endgroup\$
    – janos
    Mar 19, 2016 at 8:07
  • \$\begingroup\$ @janos Excellent explanation. \$\endgroup\$
    – vivek
    Mar 19, 2016 at 10:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.