current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

The method below converts an array of TheatreListData objects to an array of TheatreListDataWrapper. It also loads additional attributes such as patientIdentifierNumber and requestText to enrich the converted data.

This method is called by many EJBs, and so I'm especially looking for ways to improve performance.

/** 
* Converts TheatreListDatas into TheatreListDataWrappers. This also 
* loads the attributes that has to be generated from client such as 
* patientIdentifierNumber, requestText 
* 
* @param theatreListData 
* @return TheatreListDataWrapper[] 
*/
public TheatreListDataWrapper[] wrapData(TheatreListData[] theatreListData){

    TheatreListDataWrapper[] wrapperList = wrapDataWithoutRequestData(theatreListData);

    if (wrapperList != null && wrapperList.length > 0)
    {
      Set<String> requestIds = new HashSet<String>();

      for (TheatreListData listData : theatreListData)
      {

        // Collect request id for each List Data into a set
        String requestId = listData.getCarePlan().requestId;
        //Should check for '-1' here to avoid calls to fetch invalid requests.
        //'-1' could have been saved with registrations with 'None' requests.
        if (requestId != null && requestId.trim().length() > 0 && !requestId.equals("-1"))
        {
          //Since adding to a set no duplicates are possible.
          requestIds.add(requestId);
        }
      }

      //Load request Id, request Text map using the collected request ids.
      Map<String, String> requestMap = Collections.emptyMap();
      if (!requestIds.isEmpty())
      {
        requestMap = getRequestDelegator().getRequestPresentationStringsByIds(new ArrayList<String>(requestIds));
      }

      //Create wrappers & fill with loaded data.
      for (int i = 0; i < theatreListData.length; i++)
      {
        TheatreListDataWrapper wrapper = wrapperList[i];

        TheatreListData listData = theatreListData[i];
        String requestId = listData.getCarePlan().requestId;
        if (requestId != null && requestId.trim().length() > 0)
        {
          wrapper.requestText = requestMap.get(requestId) != null ? requestMap.get(requestId) : "";
        }

        wrapperList[i] = wrapper;
      }

    }
    return wrapperList;

  }
share|improve this question
up vote 2 down vote

Depending on the constant overhead of retrieving the request presentation string for a request ID (i.e. is it considerably more efficient to get all the strings for a bunch of IDs at once than for each ID separately?), a better design might be to iterate over the theatre list only once, getting the representation strings as you go and caching the results in a Map.

Map<String, String> cache = new HashMap<>();
for (int i = 0; i < theatreListData.length; i++) {
    TheatreListDataWrapper wrapper = wrapperList[i];
    TheatreListData listData = theatreListData[i];
    String requestId = listData.getCarePlan().requestId;
    if (validRequestId(requestId)) {
        String text = cache.get(requestId);
        if (text == null) {
            text = getRequestDelegator().getRequestPresentationStringsById(requestId);
            cache.put(requestId, text);
        }
        wrapper.requestText = text;
    }
}

This is probably not a good idea of getting the request text requires, say, a database query.

Other than that, I can think of no algorithmic improvements. You should store the result of requestMap.get(requestId) in a local variable so you don't do the call twice; that's inefficient. Perhaps you can modify getRequestPresentationStringsByIds to take a Collection instead of a List so that you don't have to wrap your set in an ArrayList. Can't think of more.

share|improve this answer
    
Appreciate your feedback. Thank you – Crazy Ninja yesterday
up vote 1 down vote

If you're under Java 8 then I'd prefer using the Stream API:

theatreListData
    .stream()
    .map(listData -> listData.getCarePlan().requestId)
    .filter(id -> id != null)
    .filter(id -> id.trim().length() > 0)
    .filter(id -> !id.equals("-1"))
    .collect(Collectors.toSet())

And this comparsion never throws NPE.

"Constant".equals(variable);
share|improve this answer
    
Sorry mate. I'm in Java 7 – Crazy Ninja yesterday
    
Then I would define each "for" as new function – llama yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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