Efficiently consolidating an array of objects

Posted on

Problem

In my program, I’m getting Facebook objects and using temp to display on a separate screen how many unread messages they have. However, we have since decided to consolidate similar messages, such as getting XP from a Facebook friend more than once. We want those messages to be one message with the sum of the XP gifted from the friend. That part is fine and done. But, this makes temp an inaccurate representation, because the program will show how many Facebook objects there are instead of how many consolidated messages

I have written this code to loop through the objects and get a count of how many there are (if there is an ill-formatted object, we don’t want to count it so array.length is out of the question). Then I am looping through the array again TWICE to check items in the array against other items. If they are consolidated, I want the array to behave as if the objects weren’t there, because removing items from an array while you’re looping through it can cause problems and this seemed the easiest way.

Is there a way to do this more efficiently, perhaps in fewer loops?

var consolidated:Array = [];

for (var i:int = 0; i < requests.length; i++)
            {
                var dataJ:Object = JSON.parse(requests[i].data);

                if (dataJ.title == "ask")
                {
                    //check for duplicates
                    if (dupeCache[requests[i].from.id] == undefined)
                    {
                        dupeCache[requests[i].from.id] = true;
                        temp++;
                        consolidated.push(requests[i]);
                    }
                }
                else if (dataJ.title == "gift")
                {
                    temp++; 
                    consolidated.push(requests[i]);
                }


            }
            var popped:int = 0;
            for (var j:int = 0; j < consolidated.length; j++)
            {
                var dataObjectJ:Object = JSON.parse(consolidated[j].data);
                popped= 0;
                for (var k:int = 0; k < consolidated.length; k++)
                {
                    var dataObjectK:Object = JSON.parse(consolidated[j].data);

                    if (consolidated[j] == consolidated[k]) continue;

                    if (consolidated[j].from.id == consolidated[k].from.id)
                        if (dataObjectJ.title == dataObjectK.title)
                            if(dataObjectJ.type == dataObjectK.type)
                            {
                                //we dont want to consolidate hearts
                                if (dataObjectJ.type == "heart") continue;
                                temp--;
                                popped++;
                            }
                }
                j += popped;

            }

            globals.blob.unreadMessageCount = temp;

Solution

I’m afraid I’m not familiar with Actionscript but I can give you some general advice with regards to how your code is structured:

  1. Separate the requests into different arrays for each title instead of keeping them in a single array. Admittedly this is a violation of the DRY principle (don’t-repeat-yourself) but it will force you to apply specific handling rules for each rather than a general rule that you bend that you do at the moment. This is clear from the degree of nesting of if-statements in your code.

  2. You seem to JSON.parse(...) twice, can you not keep the result from the original call?

  3. You seem to check the uniqueness of the sender id twice requests[i].from.id and consilidated[j].from.id. I think you only need to do this once?

  4. Try to rely on array lengths rather than counters that you implement yourself. I know you’ve commented on it but consider building and modifying separate arrays.

  5. Don’t nest your if-statements but create separate boolean variables.

    var idIsEqual:Boolean = consolidated[j].from.id == consolidated[k].from.id;
    var titleIsEqual:Boolean = dataObjectJ.title == dataObjectK.title;
    var typeIsEqual:Boolean = dataObjectJ.type == dataObjectK.type;
    if (idIsEqual && titleIsEqual && typeIsEqual) {
        ...
    }
    
  6. It might be a little overkill but consider creating an associative array keyed by id_title_type (e.g. "12_myTitle_gift"). That would make searching a little easier.

  7. Don’t create variables named temp. This is a real bugbear of mine and I see it all the time. By the looks of it, it should probably be named unreadMessageCount. For someone who doesn’t know you code, the penny doesn’t really drop on what your code is trying to achieve until the last line is read, and suddenly everything becomes clear.

Leave a Reply

Your email address will not be published. Required fields are marked *