HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

More efficient way to write this Actionscript 3 class?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thismorewayefficientwriteactionscriptclass

Problem

I wrote one of my first classes in Actionscript 3 and I want someone versed in AS3 to help me fine tune my class. I am sure there are things that could really help with efficiency and I love learning about "Object Oriented Programming" so please take a look and let me know what you think.

Project Background: This project is a card game that reads all the cards information from an XML file so the client can easily add, edit, or delete card information. The user will drag an Activity card from one of the six stacks and place it on top of the Event card. The six stacks will have a card face up so the user can see the information and match the best card to the Event card. i.e. If the Event Card is: "Burning auto wreck with a person inside"; the Activity Cards may read "Pull Victim", "Put out Flames" etc.

When the user makes drops the Activity card on the Event Card it goes through a point check to determine whether it was a good drop, and assigns points accordingly.

After the drop, the next user clicks "Next >>", the deck re-shuffles, and the process starts all over again until a certain number of points have been gained in total.

Hopefully that made sense and you can get some kind of idea on how it works. I can elaborate more if needed.

```
/ Document Class (CardGame.as) /
package library
{
import flash.display.*;
import flash.events.*;
import flash.net.*;

public class CardGame extends Sprite
{
// Variables
private var _xml:XML;
private var _loader:URLLoader;
private var _cardArray:Array;
private var _activity:ActivityCard;
private var _event:EventCard;
private var _randomCardArray:Array;

// Contructor
public function CardGame():void
{
_loader = new URLLoader();
_loader.addEventListener(Event.COMPLETE,buildArray);
_loader.load(new URLRequest("library/card.xml"));
start_btn.addEventListene

Solution

There are a few areas that can be improved but I guess we need some more context as to how this is laid out. My comments and questions below:

1 - In ActionScript, since you can modify the contents of a collection inside a loop querying the length of the collection at each iteration is a small performance hit. So:

for(var i=0;i<_xml.card.length();i++)


should really be

var xmlLength:int = _xml.card.length();
for(var i=0;i<xmlLength;i++)


2 - To help with clarity you want to use strongly typed objects wherever possible. For example when you are storing each event or activity in the _cardArray consider having a class which just holds the data for those objects. This helps with runtime performance as well as other developers understanding not only the contents of the array but the data type of each property in the object. For example:

_cardArray.push({id:_xml.card.id[i],face:_xml.card.face[i],category:_xml.card.category[i],point:_xml.card.point[i],value:_xml.card.value[i]});


could instead be

_cardArray.push(new ActivityVO(_xml.card.id[i], _xml.card.face[i], _xml.card.category[i], _xml.card.point[i], _xml.card.value[i]));


where ActivityVO is(I'm just assuming the type of these properties):

public class ActivityVO {

    public var id:int;
    public var face:String;
    public var category:String;
    public var point:int;
    public var value:int;

    public function ActivityVO(_id:int, _face:String, _category:String, _point:int, _value:int){
        id = _id;
        face = _face;
        category = _category;
        point = _point;
        value = _value;
    }
}


3 - How does buildArray work right now? I see it is executed onComplete of the loader but it returns the array. If buildArray is called by something else(I see its public) then all of this logic will be executed twice.
Where does this get returned to? If this is the document class what is calling this public method?
randomizeArray seems to suffer from the same problem. I'm not sure where you are returning this array to.

4 - Think about having a new class which manages the deck of cards. Methods like randomizeArray and buildDeck could be within this class and reduce the clutter of your current class.

5 - The removeDeck method doesn't feel right. You should always try to avoid for loops with hard coded breaking values (106). Arguably a better way to go about the whole loop would be to maintain an array of all the cards on the table, then when it comes time to remove them they can all be cleaned up by looping through the array.

These comments aren't really in any particular order, I put them together as I was reading through your code. Let me know if you have any questions.

Code Snippets

for(var i=0;i<_xml.card.length();i++)
var xmlLength:int = _xml.card.length();
for(var i=0;i<xmlLength;i++)
_cardArray.push({id:_xml.card.id[i],face:_xml.card.face[i],category:_xml.card.category[i],point:_xml.card.point[i],value:_xml.card.value[i]});
_cardArray.push(new ActivityVO(_xml.card.id[i], _xml.card.face[i], _xml.card.category[i], _xml.card.point[i], _xml.card.value[i]));
public class ActivityVO {

    public var id:int;
    public var face:String;
    public var category:String;
    public var point:int;
    public var value:int;

    public function ActivityVO(_id:int, _face:String, _category:String, _point:int, _value:int){
        id = _id;
        face = _face;
        category = _category;
        point = _point;
        value = _value;
    }
}

Context

StackExchange Code Review Q#4859, answer score: 3

Revisions (0)

No revisions yet.