4
\$\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\$
  • \$\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 '16 at 8:22
  • \$\begingroup\$ @h.j.k. seems yes, here is the reference. \$\endgroup\$ – vivek Mar 10 '16 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 '16 at 9:11
  • \$\begingroup\$ You mean to use replace string concatenation with the bindings, right? \$\endgroup\$ – vivek Mar 10 '16 at 9:12
  • \$\begingroup\$ Yes, see answer below. \$\endgroup\$ – h.j.k. Mar 10 '16 at 9:21
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\$
  • \$\begingroup\$ Out of curiosity, what are the pros of this? \$\endgroup\$ – vivek Mar 10 '16 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 '16 at 10:08
  • \$\begingroup\$ I was curious about your second suggestion about lineItem. \$\endgroup\$ – vivek Mar 10 '16 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 '16 at 8:07
  • \$\begingroup\$ @janos Excellent explanation. \$\endgroup\$ – vivek Mar 19 '16 at 10:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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