Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.addEventListener(MouseEvent.CLICK,buildDeck);
            restart_btn.addEventListener(MouseEvent.CLICK,removeDeck);
        }

        // Methods
        public function buildArray(e:Event):Array
        {
            _cardArray = new Array();  
            _xml = new XML(_loader.data);
            for(var i=0;i<_xml.card.length();i++)
            {
                if(_xml.card.face[i]== "activity") {
                    _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]});
                } else if(_xml.card.face[i] == "event") {
                    _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]});
                } else {
                    trace("no card found");
                }
            }
            return _cardArray;
        }
        private function randomizeArray():Array
        {
            //trace(buildArray(null));
            _randomCardArray = new Array();
            do {
            var cardItem = int(Math.random()*_cardArray.length);
                _randomCardArray.push(_cardArray[cardItem]);
                _cardArray.splice(cardItem,1);
            } while (_cardArray.length > 0);
            return _randomCardArray;
        }
        public function buildDeck(e:Event):void
        {
            var _finalCardArray = randomizeArray();
            for(var i=0;i<_finalCardArray.length;i++) {
                if(_finalCardArray[i].face == "activity") {
                    _activity                = new ActivityCard();
                    _activity.x              = 200;
                    _activity.y              = 150;
                    _activity.buttonMode     = true;
                    _activity.mouseChildren  = false;
                    _activity.name           = "card"+i;
                    _activity.id_txt.text    = _finalCardArray[i].id;
                    _activity.value_txt.text = _finalCardArray[i].value;

                    _activity.addEventListener(MouseEvent.MOUSE_DOWN,dragStart);
                    _activity.addEventListener(MouseEvent.MOUSE_UP,dragStop);

                    addChild(_activity);
                } else if(_finalCardArray[i].face == "event") {
                    _event                = new EventCard();
                    _event.x              = stage.stageWidth/2;
                    _event.y              = 550;
                    _event.mouseChildren  = false;
                    _event.name           = "card"+i;
                    _event.id_txt.text    = _finalCardArray[i].id;
                    _event.value_txt.text = _finalCardArray[i].value;
                    addChild(_event);
                } else {
                    trace("no cards");
                }
            }
        }
        public function dragStart(e:Event):void
        {
            _activity = e.target as ActivityCard;     
            _activity.parent.setChildIndex(_activity, numChildren - 1);     
            _activity.startDrag(false);
        }
        public function dragStop(e:Event):void
        {
            this.stopDrag();
        }
        private function removeDeck(e:Event):void
        {
            if(_cardArray.length == 0) {
                for(var i=0;i<106;i++) {
                    removeChild(getChildByName("card"+i));
                }
                buildArray(null);
                buildDeck(null);
            }
        }
    }
share|improve this question
add comment

1 Answer 1

up vote 3 down vote accepted

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.

share|improve this answer
    
@ mbaker, this is GOLD! I really appreciate the feedback. I understand what you are saying about creating more classes and how that could improve many things. I am doing some reading and modifying of my code, but I will have questions. Thanks again .... stand by .... –  Mike Sep 16 '11 at 16:29
    
And to answer your question about buildArray() it does get called again in removeDeck() and that is the only time. What it's doing is it's called from the _loader and it builds the _cardArray and is held in memory to be used in randomizeArray(). buildDeck() will then use the new random array as _finalCardArray. My thought behind this was to have most my tasks as methods for easy modification and or readablilty. I never thought of making multiple classes, seems a better approach after I think of it. –  Mike Sep 16 '11 at 16:44
    
Now you have opened a can of worms, lol. I am trying to add more classes to my package and I receive an error: 5006: An ActionScript file can not have more than one externally visible definition: library.Card, library.ActivityVO. I have tried a few ways to instantiate the ActivityVO class and have no luck :-( –  Mike Sep 16 '11 at 18:41
    
I just learned something that I thought was something to tell anyone who is new to AS3 Classes, I thought you had to import all external class files into the document class in order to have access to any of the external classes methods, but it turns out that if the external class file resides in the same folder all you have to do is instantiate it. VERY USEFUL knowledge :-) –  Mike Sep 16 '11 at 20:50
    
Sorry I was MIA for a few days. Good to hear that my feedback was well received! With regards to import statements they can become a royal pain if you get beyond a few simple classes.If you're going to continue doing a good deal of AS3 work I highly recommend looking into a proper IDE. Something like FDT (fdt.powerflasher.com) or FlashDevelop (flashdevelop.org/wikidocs/index.php?title=Main_Page). They will handle all of the intellisense and import statements for you. –  mbaker3 Sep 23 '11 at 19:09
add comment

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.