2
\$\begingroup\$

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;
\$\endgroup\$
2
  • \$\begingroup\$ Don't use hashtable[key] == undefined to test whether hashtable contains a key. Use key in hashtable - this is the purpose of this operator. As to the whole question: use appropriate data structures. This one is begging for a tree. Consolidated nodes would be the nodes that have child nodes and leafs would be the nodes which didn't require consolidation. You then can add a generic method for both leafs and intermediate nodes to give you the count or whatever other information you need. This would achieve O(n) parsing and O(log n) querying time. \$\endgroup\$
    – wvxvw
    Jan 2, 2015 at 14:17
  • \$\begingroup\$ Also, if inside if (dataJ.title == "ask") doesn't have an else branch. Is that intended? \$\endgroup\$
    – wvxvw
    Jan 2, 2015 at 14:21

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Any specific reasoning behind number 5? \$\endgroup\$ Dec 29, 2014 at 1:15
  • \$\begingroup\$ Usually it's a good idea to have lines of code which don't exceed 80 characters long otherwise it requires other developers to have to horizontally scroll in order to understand the code. By splitting apart your if-statement like I have shown, it's a little easier on the eye and easier to debug. Also, having many if-statements nested inside one another also increases your code complexity so this flattening of your code makes it simpler. See if there's a tool available for ActionScript for calculating McCabe's Cyclomatic Complexity metric. \$\endgroup\$ Dec 29, 2014 at 4:37
  • \$\begingroup\$ Regarding your first point, they're all on separate lines and your suggested code's first line is much longer than any line I have. I'll take a gander at the rest, but at this point I think we've moved past it. Still, I appreciate the help \$\endgroup\$ Dec 29, 2014 at 19:23

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.