Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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

I have written this code according to my requirements. But after finishing the code, it looks like there are too many loops. I have reduced it as much as I can. Can you find any improvements that I can make to this method?

The relationship is that one theater activity has many bookable resources. Note: Bookable resources are common to all activities, but the count of those bookable resources are unique to each activity.

  private TheatrePlanResourceData[] buildTheatrePlanResourceData(Map<Long, List<TheatreResourceData>> theatreActivities)
  throws SpiderException{
     TheatrePlanResourceData[] result = new TheatrePlanResourceData[theatreActivities.size()];
     Map<Long, Map<Long, Integer>> reverseBookableResMap = new HashMap<Long, Map<Long, Integer>>();
     Map<Long, Integer> bookableResourceKeyToCountMapOutter = new LinkedHashMap<Long, Integer>();

     int count = 0;

     for (Map.Entry<Long, List<TheatreResourceData>> activityEntry : theatreActivities.entrySet())
     {

       List<TheatreResourceData> assistants = new ArrayList<TheatreResourceData>();
       List<TheatreResourceData> surgeons = new ArrayList<TheatreResourceData>();
       Map<Long, Integer> bookableResourceKeyToCountMap = new LinkedHashMap<Long, Integer>();

       TheatrePlanResourceData planResourceData = new TheatrePlanResourceData();
       planResourceData.theatreActivityKey = activityEntry.getKey();
       List<TheatreResourceData> resourceData = (activityEntry.getValue() != null) ? activityEntry.getValue() : new ArrayList<TheatreResourceData>();


       if (resourceData.size() > 0){
         planResourceData.stateKey = ((TheatreResourceData) resourceData.get(0)).stateKey;
       }



       for (TheatreResourceData resData : resourceData){
         switch (resData.type){
          case TheatreResourceType.SURGEON1:
               planResourceData.staff.surgeon1 = resData;
               break;
          case TheatreResourceType.ASSISTANT:
               assistants.add(resData);
               break;
          case TheatreResourceType.OTHER_SURGEONS:
               surgeons.add(resData);
               break;
          case TheatreResourceType.BOOKABLE_RESOURCE:
               bookableResourceKeyToCountMap.put(Long.parseLong(resData.resourceID), resData.count);
               bookableResourceKeyToCountMapOutter.put(Long.parseLong(resData.resourceID), resData.count);
               break;
          }
        }

        reverseBookableResMap.put(activityEntry.getKey(), bookableResourceKeyToCountMap);

        planResourceData.staff.assistants = assistants.toArray(new TheatreResourceData[assistants.size()]);
        planResourceData.staff.otherSurgeons = surgeons.toArray(new TheatreResourceData[surgeons.size()]);


        result[count++] = planResourceData;

      }

      Long[] bookableResourceKeyList = bookableResourceKeyToCountMapOutter.keySet()
    .toArray(new Long[bookableResourceKeyToCountMapOutter.keySet().size()]);
      TheatreBookableResourceTypeData[] allBookableResourceTypeList = InternalTheatreBookableResourceTypeToolkit
    .getInstance().getByKeys(CollectionUtils.toPrimitive(bookableResourceKeyList));


      for (TheatrePlanResourceData resourceDataObj : result){
        Map<Long, Integer> bookableResourceCountMap = reverseBookableResMap.get(resourceDataObj.theatreActivityKey);
        TheatreBookableResourceTypeData[] relevantResourceList = new TheatreBookableResourceTypeData[bookableResourceCountMap
      .size()];
        int index = 0;

        for (Map.Entry<Long, Integer> countEntry : bookableResourceCountMap.entrySet()){

          for (TheatreBookableResourceTypeData resTypeObj : allBookableResourceTypeList){

            if (countEntry.getKey() == resTypeObj.key){
              try{
                TheatreBookableResourceTypeData clonedResourceType = (TheatreBookableResourceTypeData) resTypeObj.clone();
                clonedResourceType.count = countEntry.getValue();
                relevantResourceList[index++] = clonedResourceType;
              }catch (CloneNotSupportedException e){
                TheatreModuleException.convert(e);
                continue;
              }

            }
          }

        }

        resourceDataObj.bookableRes = relevantResourceList;

      }

      return result;
  }
share|improve this question
    
Are you on Java 8? – h.j.k. Feb 24 at 6:40
1  
@h.j.k. Java 1.7.51 – Jude Feb 24 at 7:54
    
Can you fix the indentation if that's related to pasting it here? – ferada Feb 24 at 11:35
up vote 3 down vote accepted

Transform Bookable into an actual object.

I tried writing 3 different answers, and it just didn't work. You always end up with a long, long complicated mess.

If you transform Bookable into an actual object, you can add it to a list. You can then pass a list of TheatrePlanResourceData to some TypeData mapper.

This way you can split up your code into four parts:

Part 1:

   List<TheatreResourceData> assistants = new ArrayList<TheatreResourceData>();
   List<TheatreResourceData> surgeons = new ArrayList<TheatreResourceData>();
   Map<Long, Integer> bookableResourceKeyToCountMap = new LinkedHashMap<Long, Integer>();

   TheatrePlanResourceData planResourceData = new TheatrePlanResourceData();
   planResourceData.theatreActivityKey = activityEntry.getKey();
   List<TheatreResourceData> resourceData = (activityEntry.getValue() != null) ? activityEntry.getValue() : new ArrayList<TheatreResourceData>();


   if (resourceData.size() > 0){
     planResourceData.stateKey = ((TheatreResourceData) resourceData.get(0)).stateKey;
   }



   for (TheatreResourceData resData : resourceData){
     switch (resData.type){
      case TheatreResourceType.SURGEON1:
           planResourceData.staff.surgeon1 = resData;
           break;
      case TheatreResourceType.ASSISTANT:
           assistants.add(resData);
           break;
      case TheatreResourceType.OTHER_SURGEONS:
           surgeons.add(resData);
           break;
      case TheatreResourceType.BOOKABLE_RESOURCE:
           bookableResourceKeyToCountMap.put(Long.parseLong(resData.resourceID), resData.count);
           bookableResourceKeyToCountMapOutter.put(Long.parseLong(resData.resourceID), resData.count);
           break;
      }
    }

    reverseBookableResMap.put(activityEntry.getKey(), bookableResourceKeyToCountMap);

    planResourceData.staff.assistants = assistants.toArray(new TheatreResourceData[assistants.size()]);
    planResourceData.staff.otherSurgeons = surgeons.toArray(new TheatreResourceData[surgeons.size()]);


    result[count++] = planResourceData;

the inner loop of your first for loop, where you make a TheatrePlanResourceData out of a theatreActivity.

Part 2:

The part where you make all your objects.

 TheatrePlanResourceData[] result = new TheatrePlanResourceData[theatreActivities.size()];

 int count = 0;

 for (Map.Entry<Long, List<TheatreResourceData>> activityEntry : theatreActivities.entrySet())
 {
     result[count++] = part1(activityEntry);
 }

Part 3:

The part where you take all the objects you've made, and use .bookables to get a list of ids, so you can call InternalTheatreBookableResourceTypeToolkit .getInstance().getByKeys(CollectionUtils.toPrimitive(bookableResourceKeyList)); with the right values. ... Try using a Set<Long>.

Part 4:

The part where you take all the objects you've made, and use the retrieved database objects to clone TheatreBookableResourceTypeData and set the objects into the TheatrePlanResourceDatas.

It's up to you to do the actual cutting and pasting. Ask for a follow up review with a new question once you're done, maybe then we can take a look at writing some "parser class" that handles all the crap for you.

Result:

  private TheatrePlanResourceData[] buildTheatrePlanResourceData(Map<Long, List<TheatreResourceData>> theatreActivities)
  throws SpiderException{
    //method one
    TheatrePlanResourceData[] result = new TheatrePlanResourceData[theatreActivities.size()];

     int count = 0;

     for (Map.Entry<Long, List<TheatreResourceData>> activityEntry : theatreActivities.entrySet())
     {
         //method 3, for you to write
         result[count++] = part1(activityEntry);
     }

     //method 2
     Set<Long> keys = new HashSet<Long>();
     for(TheatrePlanResourceData data : result){
         List<Bookable> bookables = data.getBookables();
         for(Bookable b : bookables){
             keys.add(b.getResourceID());
         }
     }

     TheatreBookableResourceTypeData[] allBookableResourceTypeList = InternalTheatreBookableResourceTypeToolkit
.getInstance().getByKeys(CollectionUtils.toPrimitive(bookableResourceKeyList));

     //method 4
           for (TheatrePlanResourceData resourceDataObj : result){
    List<TheatreBookableResourceTypeData> relevantResourceList = new ArrayList<TheatreBookableResourceTypeData>();

    for (Bookable b : resourceDataObj.getBookables()){

      for (TheatreBookableResourceTypeData resTypeObj : allBookableResourceTypeList){

        if (b.getResourceID() == resTypeObj.key){
          try{
            TheatreBookableResourceTypeData clonedResourceType = (TheatreBookableResourceTypeData) resTypeObj.clone();
            clonedResourceType.count = b.getValue();
            relevantResourceList.add(clonedResourceType);
          }catch (CloneNotSupportedException e){
            TheatreModuleException.convert(e);
            continue;
          }

        }
      }

    }

    resourceDataObj.bookableRes = relevantResourceList.toArray(new TheatreBookableResourceTypeData[relevantResourceList.size()]);

  }
}

You probably can't reduce the amount of for loops, but what you can do it is migrate it to separate methods and make it understandable. Complexity isn't that bad when you know what's going on.

share|improve this answer
    
What I learned: take out the content from a lengthy loop and put it to a function. Thank you! :) – Jude Feb 26 at 6:09

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.