1

I have this json object coming in for boxes:

Boxes: 
 [ { count: 5, Fragile: true },
 { count: 10, Fragile: false } ],

I have box records that need to be updated based on if they are fragile or not and that they haven’t been used. Basically when that json comes through, we want to update existing box records and send the boxes' information back to our 3rd party.

public static void createPack() {
    pack = new Package__c();

    if(packageWrapper.boxesNeeded != null && packageWrapper.boxesNeeded.size()>0){
        pack.Extra_Boxes__c = true;
    }
    insert pack;

 List<Decimal> countList = new List<Decimal>();
    List<Boolean> fragilelList = new List<Boolean>();
    for(BoxesNeeded boxesNeeded : packageWrapper.boxesNeeded){
        if( packageWrapper.boxesNeeded != null &&  packageWrapper.boxesNeeded.size()>0){
            numberOfBoxes = packageWrapper.boxesNeeded.size();
            countList.add(boxesNeeded.weight);
            fragileList.add(boxesNeeded.fragile);
    }
    List<box__c> boxesToUse = [SELECT ID, width__c, height__c, length__c FROM box__C WHERE Used__C = FALSE AND Fragile__c IN : fragileList LIMIT :numberOfBoxes]; 
    for(Integer i = 0; i < boxesToUse.size(); i++){
        box__C BoxToUse = BoxesToUse.get(i);
        BoxToUse.Package__c = Pack.id;
        BoxToUse.Date_Used__c = Date.today();
        for(Integer c = 0; c < countList.size(); c++){
            BoxToUse.count__c =countList.get(c);
        }
    }
    upsert BoxesToUse;
    for(Box__c BoxToUse : BoxesToUse){
       boxWrappers.add(new boxWrapper(boxToUse.width__c, boxToUse.height__c, boxToUse.length__c,,boxToUse.fragile__c)
    }
    }
}

My wrappers:

 public class BoxesNeeded{
    public Decimal count {get;set;}
    public Boolean fragile {get;set;}

    public BoxesNeeded(Decimal count, Boolean fragile){
        this.count = count;
        this.fragile = fragile;
    }
}

global class boxWrapper{
    Integer width {get;set;}
    Integer height {get;set;}
    Integer length {get;set;}
    Boolean fragile {get;set;}

    public BoxWrapper(Integer width, integer height, integer length, boolean fragile){
        this.width= width;
        this.height = height;
        this.length = length;
        this.fragile = fragile;
    }
}

After I retrieve the box records we send back a json to our third party of the records it should look like this(however it includes twice the amount. this snippet does not include it. just showing what it should look like):

 result.body { success: true,
  shipId: ’SH-0343452',
  Boxes: 
  [ { width: 12,
  height:8,
  length:12,
  fragile: true,
   Count: 5 },
   { width: 8,
   Height:10,
   length:12,
   fragile: false,
   Count: 10
   },
   ],

New Edit: After reviewing what we planned on doing with this JSON object:

Boxes:
    [ { count: 5, Fragile: true };
    {count: 10, Fragile: false};

I have gotten a clearer picture on what needs to be done.

With the JSON object above we will then want to update existing box records in our organization. Which is why we need to query for existing box records that are not used that are either fragile vs non fragile.

So this json object shows two box records, with a count of 5 and a count of 10, one is fragile and one is not.

SO we will be updating 2 box records that have not been used yet.

 So Box 1:
 Count__c = 5; <=field that needs to be updated, grab the first count 
 (5) from json
 Fragile__c = true<= we are not updating this, we need to find a un-used 
  box record that is fragile

 Box 2:
 Count__c = 10; <=field that needs to be updated, grab the second 
 count(10) from json
 Fragile__c = false<= we are not updating this, we need to find a un- 
 used box record that is not fragile

Once that is updated, we will then return back a json object to our third party with this information about the box: Width Height length Fragile Count

So to do so, I have created a wrapper class for Boxes Needed:

 public class BoxesNeeded{
   public Decimal count {get;set;}
   public Boolean fragile {get;set;}

   public BoxesNeeded(Decimal count, Boolean fragile){
      this.count = count;
      this.fragile = fragile;
   }
 }

Then to send the box information once updated to our 3rd party, the boxWrapper will look like this:

 global class boxWrapper{
    Integer width {get;set;}
    Integer height {get;set;}
    Integer length {get;set;}
    Boolean fragile {get;set;}
    Integer count {get; set;}

public BoxWrapper(Integer width, integer height, integer length, boolean 
    fragile, integer count){
    this.width= width;
    this.height = height;
    this.length = length;
    this.fragile = fragile;
    this.count = count;
   }
}

I have added new data types to capture non-fragile vs fragile counts of boxes needed:

 Public Integer fragileCount {get; set;}
 Public integer nonFragileCount {get;set;}

Then like mentioned below, my For Loop will count how many of each I need(changed var name to boxNeeded to differentiate from Object BoxesNeeded:

  for(BoxesNeeded boxNeeded : packageWrapper.boxesNeeded){
      if(boxNeeded.fragile == true){
            fragileCount ++;
        }
        else if(boxNeeded.fragile == false){
            nonFragileCount ++;
        }
    }

Then the two queries also mentioned below will bring back the box records I need to update:

   List<Box__c> fragileBoxesToUse = [SELECT ID, width__c, height__c, 
   length__c FROM box__C WHERE Used__C = FALSE AND Fragile__c =true LIMIT :fragileCount FOR UPDATE];
List<Box__c> nonFragileBoxesToUse = [SELECT ID, width__c, height__c, 
length__c FROM box__C WHERE Used__C = FALSE AND Fragile__c = false LIMIT :nonFragileCount FOR UPDATE];

Now to loop through the two lists:

 For(Box__c boxToUse : fragileBoxesToUse){
    BoxToUse.Package__c = Pack.id;
    BoxToUse.Date_Used__c = Date.today();
    BoxToUse.Count__c = ??? I need to get the count from the JSON 
    boxWrappers.add(new boxWrapper(boxToUse.width__c, 
    boxToUse.height__c, boxToUse.length__c,,boxToUse.fragile__c, 
    boxToUse.count__c);
}

upsert FragileBoxesToUse;

For(Box__c boxToUse : nonfragileBoxesToUse){
    BoxToUse.Package__c = Pack.id;
    BoxToUse.Date_Used__c = Date.today();
    BoxToUse.Count__c = ??? I need to get the count from the JSON 
    boxWrapper(boxToUse.width__c, boxToUse.height__c, 
    boxToUse.length__c,,boxToUse.fragile__c, boxToUse.count__c);

}
upsert nonFragileBoxesToUse;

Two more things, to combine my two List queries would I need to write a dynamic soql query? Also after clarifying what "Count" is, how would I grab the correct "count" to update my box records? Count does not represent how many box records that need to be updated but it represents a field on the box record to indicate how many items will go in the box. (Count__c)

So in all, the goal from this Json is to update existing box records and return the updated box records to our third party.

So in this json example we will be updating two box records and then return two box record's information.

Recent Edit: After changing it to List nonfragileCounts, when i grab nonFragileCount.get(i) in my loop to update the count field i receive:

List index out of bounds: 0

for(BoxesNeeded boxNeeded : packageWrapper.boxesNeeded){
        if(boxNeeded.fragile == true){
            fragileCount.add(boxNeeded.count);

        }
        else if(boxNeeded.fragile == false){
            nonFragileCount.add(boxNeeded.count);
        }
    }
    List<Box__c> nonFragileBoxesToUse = [SELECT ID, width__c, height__c, 
    length__c FROM box__C WHERE Used__C = FALSE AND Fragile__c = false 
    LIMIT :nonFragileCount.size() FOR UPDATE];
    for(Integer i = 0; i < nonFragileBoxesToUse.size(); i++){
        Box__c BoxToUse = nonFragileBoxesToUse.get(i);
        BoxToUse.Package__c = Pack.id;
        BoxToUse.Date_Used__c = Date.today();
        BoxToUse.Count__c =  nonFragileCount.get(i);
    }  

    upsert nonFragileBoxesToUse;
2
  • 2
    While there's a lot of information here, it appears we're still missing critical pieces. I don't see any reason why your boxes would be "duplicated" for fragile boxes (none of the code here actually suggests that's what's happening), and there's at least a few other logic flaws. I'd rather see too much code than too little. You've spent a lot of time hypothesizing, but from what I can tell, your ideas are actually incorrect. There's something else going on, and it's hidden in code you haven't shared with us. Please edit your question to include at least the entire method.
    – sfdcfox
    Commented Dec 28, 2018 at 21:05
  • I have edited my post to include the full method, however it was just to show the insertion of a pack record. Also include my wrapper methods
    – J.koh
    Commented Dec 28, 2018 at 21:26

1 Answer 1

1
+50

Architecture and Data Model Commentary

You may have very good reasons for adopting this data model, but looking in from the outside I think it's going to cause you more problems than it helps. If your Boxes don't have any individual attributes but simply represent inventory in a product, I would not create each Box in inventory as a separate record, but store an Inventory amount on a Product as a Roll-up Summary of Inbound and Outbound transactions as custom objects.

I also think the JSON design is strange. I'm assuming in my commentary below that the inbound request contains exactly one request for Fragile and one request for Non-Fragile Boxes. Right now, the inbound request is a list of objects that specify Fragility or Non-Fragility, but the targeted outbound response seems to include product details that aren't requested by the caller, and the actual response being generated in Apex is just a flat list of individual Boxes. More on this below.

Concrete Issue

That aside, let's look at the issue you have here.

List<Decimal> countList = new List<Decimal>();
List<Boolean> fragilelList = new List<Boolean>();
for(BoxesNeeded boxesNeeded : packageWrapper.boxesNeeded){
    if( packageWrapper.boxesNeeded != null &&  packageWrapper.boxesNeeded.size()>0){
        numberOfBoxes = packageWrapper.boxesNeeded.size();
        countList.add(boxesNeeded.weight);
        fragileList.add(boxesNeeded.fragile);
}

A few things here.

  1. Naming variables and classes the same thing can lead to confusing errors and confusing logic. Remember that Apex is case-insensitive, so BoxesNeeded and boxesNeeded are read by the compiler as equivalent. Here, the issues seem to primarily be with logic; I don't see any name shadowing.

  2. This if statement does nothing and can be removed, as it's confusing:

    if( packageWrapper.boxesNeeded != null &&  packageWrapper.boxesNeeded.size()>0){
    

    If the loop variable (packageWrapper.boxesNeeded) were null or empty, the loop would not execute and you'd never get here.

    If what you want is to check whether this BoxesNeeded wrapper is empty, you should write

    if (boxesNeeded.size() > 0) {
    
  3. numberOfBoxes = packageWrapper.boxesNeeded.size(); overwrites the value numberOfBoxes each time through the loop with the count of boxesNeeded wrappers. It looks like it is supposed to accumulate the total number of boxes required across all of the boxesNeeded wrappers, which would have been

     numberOfBoxes += boxesNeeded.count;
    
  4. countList accumulates a property boxesNeeded.weight, which is not included in your code. More generally, countList and fragileList don't make sense as data structures and cause problems with your query, of which more below.

It looks to me like your for loop needs to do exactly two things:

  1. Accumulate an Integer count of Fragile Boxes needed. Let's call that countFragile.
  2. Accumulate an Integer count of Non-Fragile Boxes needed. Let's call that countNonFragile.

Everything else in the loop appears to be superfluous or incorrect.

Now let's look at the query.

List<box__c> boxesToUse = [SELECT ID, width__c, height__c, length__c FROM box__C WHERE Used__C = FALSE AND Fragile__c IN : fragileList LIMIT :numberOfBoxes];

First of all, fragileList with the JSON given as an example will contain (true, false) - that is, both legal Boolean values, so this filter does nothing. As we've already see, numberOfBoxes is a problem. We're also going to need to guard against race conditions and validate that the number of boxes we got back is equal to the limit we specify. Let's rewrite this as two queries:

try {
    List<Box__c> fragileBoxesToUse = [SELECT ID, width__c, height__c, length__c 
                                      FROM box__C 
                                      WHERE Used__C = FALSE AND Fragile__c = true
                                      LIMIT :fragileCount
                                      FOR UPDATE];
    List<Box__c> nonFragileBoxesToUse = [SELECT ID, width__c, height__c, length__c 
                                         FROM box__C 
                                         WHERE Used__C = FALSE AND Fragile__c = false
                                         LIMIT :nonFragileCount
                                         FOR UPDATE];
} catch (QueryException q) {
    // Either try again or tell the caller of the service to retry their request
    // We didn't get exclusive access to the set of Boxes.
}

if (fragileBoxesToUse.size() != fragileCount || nonFragileBoxesToUse != nonFragileCount) {
    // Do something here to return an error
}

So we're adding a little complexity here, but we're also handling errors - something which the current code does not do - and ensuring that two callers aren't allocated the same Boxes by mistake. (Note: this is something that could be ameliorated by the architecture change I mention above. Less concerns about locking if each box isn't its own record).

Now let's turn to the box update.

for(Integer i = 0; i < boxesToUse.size(); i++){

This should be an sObject iteration loop because there's no reason to use a loop variable here:

for (Box__c boxToUse: boxesToUse) {

Since we've split our query in two, you'll need to either loop twice or, better, build a single List<Box__c> and add all of the fragile and non-fragile boxes to it to loop.

    box__C BoxToUse = BoxesToUse.get(i);
    BoxToUse.Package__c = Pack.id;
    BoxToUse.Date_Used__c = Date.today();
    for(Integer c = 0; c < countList.size(); c++){
        BoxToUse.count__c =countList.get(c);
    }

This for loop is incorrect, but does not make sense so I can't say what it was intended to do. countList as generated above doesn't include count values - it seems to include weights - and this for loop will simply add the last value in the countList array to every Box because you overwrite BoxToUse.count__c on every iteration. It's also not clear why you'd populate a count value on an individual Box record.

You may have meant to do BoxToUse.count__c =countList.get(i); without an inner for loop. In that case, you would need the loop variable i after all.

Lastly, the JSON generation.

for(Box__c BoxToUse : BoxesToUse){
   boxWrappers.add(new boxWrapper(boxToUse.width__c, boxToUse.height__c, boxToUse.length__c,,boxToUse.fragile__c)
}

This doesn't match your targeted JSON. The JSON you've shown in your question includes a Count, which you don't show here, of each sort of box, along with that sort of box's height, length, and fragility. But that's not how you're handling Box records - you query any Box that's unused and has the right fragility, and your inbound JSON doesn't specify the product details that you're returning here.

I'm not going to make a recommendation on how to fix this because I think the data model for the service doesn't really make sense, and repairing it digs enough into the architecture of your overall application and system integration that I think you need to work it out internally and/or ask more specific questions about the service structure.

Overall Notes

I want to reiterate that I think the problems with this code have to do partially with logic that is clearly incorrect, but ultimately with a data model that either does not make sense, is not well defined, or both. I would start addressing your issues not by writing more code but by getting clarity on what your data looks like, both in Salesforce and in the call and response for this service.

Are you tracking individual, physical boxes or are you tracking inventory, as the confusing count__c assignment at the end seems to suggest? Why are you tracking weight, or are you at all? Does the caller care what size box they receive? Most of these questions are things that SFSE cannot help you with. You need to do the business analysis on your own application, or work with other experts in your organization to understand what's happening.

When you answer questions like these effectively, you'll be in a position to come back and evaluate this code, incorporating commentary like the above, and likely do a significant rewrite to reflect your understanding of the overall data model and box lifecycle.

Response to Updates

Two more things, to combine my two List queries would I need to write a dynamic soql query?

You cannot combine these two queries to do what you're trying to do.

Also after clarifying what "Count" is, how would I grab the correct "count" to update my box records? Count does not represent how many box records that need to be updated but it represents a field on the box record to indicate how many items will go in the box. (Count__c)

You would need to go back to accumulating a List<Integer> fragileCounts and List<Integer> nonfragileCounts, and then querying with a LIMIT based on fragileCounts.size(), etc., rather than accumulating an integer.

Then, when you update your two lists of boxes, grab the count value at the corresponding index in the list and populate that data point.

Recent Edit: After changing it to List nonfragileCounts, when i grab nonFragileCount.get(i) in my loop to update the count field i receive:

List index out of bounds: 0

I think your query is returning no results. I would suggest you add the check that I proposed above to validate that your query returns the number of boxes you asked for.

11
  • Hi David thank you for the post, I have made edits to my post to clarify the business logic we are trying to achieve. I have taken your notes into consideration and applied it to some but feel as if my logic is still incorrect. I also added additional notes on what I would like some assistance on if possible.
    – J.koh
    Commented Jan 2, 2019 at 0:14
  • @J.koh Are you saying that each of the list items in your inbound JSON has a count parameter that represents the number of items in ONE box, not the number of boxes needed of this type? If yes, some of this logic is wrong.
    – David Reed
    Commented Jan 2, 2019 at 0:17
  • Yes after confirming, the count parameter is for one box. So above one fragile box has a count of 5, and the non fragile box has a count of 10.
    – J.koh
    Commented Jan 2, 2019 at 0:20
  • hi thank you for more updates. Another question i just edited, after including the if statement to check if it is fragile or not it is not failing the response in my test class.
    – J.koh
    Commented Jan 2, 2019 at 2:44
  • I have no idea why your test might be failing. I think that's really a separate question, but I would strongly encourage you to get to a point where your service works 100% to your specification before you start writing a unit test.
    – David Reed
    Commented Jan 2, 2019 at 2:48

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .