AS3 code feedback

Question

I have just started coding in AS3 and it would be really great to get some feedback from the experts; on my coding style, things I'm doing wrong, thing I can improve on, best practises, and so on... Also if you have some extra tips or tricks, that would be great.

Here's my first bit of AS3 code, took me 5 hours, puh:

package {

 import flash.display.Sprite;
 import flash.net.URLLoader;
 import flash.net.URLRequest;
 import flash.events.*;
 import flash.errors.*;
 import flash.display.MovieClip;
 import gs.*;
 import flash.display.Loader;
 import net.stevensacks.preloaders.CircleSlicePreloader;

 public class FlatSelector extends MovieClip {

  var preloader:CircleSlicePreloader = new CircleSlicePreloader();
  var imageLoader:Loader = new Loader();
  var globalXML:XML;

  public function FlatSelector() {
   stage.addEventListener(Event.ENTER_FRAME, init);
   building.alpha = 0;
  }

  public function init(event:Event):void {
   stage.removeEventListener(Event.ENTER_FRAME, init);
   var loader:URLLoader = new URLLoader(); 
   loader.addEventListener(Event.COMPLETE, handleXML);
   loader.load(new URLRequest('http://localhost/boligvelger/flats.xml'));
   TweenLite.to(building, 2, {alpha:1});
   TweenLite.to(building.flat, 2, {alpha:0.5, tint:0x00FF23});
   //var myTween:TweenLite = TweenLite.to(mc, 1, {x:200});
   //var myTween:TweenLite = new TweenLite(mc, 1, {x:200});
  }

  public function handleXML(e:Event):void {
   var xml:XML = new XML(e.target.data);
   globalXML = xml;
   for (var i:Number = 0; i < xml.leiligheter.leilighet.length(); i++) {
    var flatName = xml.leiligheter.leilighet[i].navn;
    if(movieClipExists(building[flatName])) {
     building[flatName].addEventListener(MouseEvent.MOUSE_UP, flatMouseClick);
     building[flatName].addEventListener(MouseEvent.MOUSE_OVER, flatMouseOver);
     building[flatName].addEventListener(MouseEvent.MOUSE_OUT, flatMouseOut);
     building[flatName].alpha = 0;
     TweenLite.to(building[flatName], 2, {alpha:0.5, tint:0x00FF23});
    }
   }
  }

  public function showInfoBox():void {

  }

  public function showFlat(flatName:String):void {
   trace('flatName: '+flatName);
   trace('flat shown');
   var imageURL;
   for (var i:Number = 0; i < globalXML.leiligheter.leilighet.length(); i++) {
    if(globalXML.leiligheter.leilighet[i].navn == flatName) {
     imageURL = globalXML.leiligheter.leilighet[i].plantegning;
    }
   }
   trace(imageURL);
   loadImage(imageURL);
  }

  public function showBuilding():void {
   TweenLite.to(imageLoader, 0.5, {alpha:0, onComplete:function(){
    removeChild(imageLoader);   
   }});
  }

  public function flatMouseClick(e:MouseEvent):void {
   trace('clicked');
   TweenLite.to(building, 0.7, {alpha:0, onComplete:showFlat(e.target.name)});
   TweenLite.to(building, 2, {y:stage.stageHeight, overwrite:0});
  }

  public function flatMouseOver(e:MouseEvent):void {
   TweenLite.to(building[e.target.name], 0.5, {tint:0x62ABFF});
   building[e.target.name].buttonMode = true;
  }

  public function flatMouseOut(e:MouseEvent):void {
   TweenLite.to(building[e.target.name], 0.5, {tint:0x00FF23});
  }

  public function showPreloader():void {
   preloader.x = (stage.stageWidth-preloader.width)/2;
   preloader.y = (stage.stageHeight-preloader.height)/2;
   preloader.alpha = 0;
   addChild(preloader);
   TweenLite.to(preloader, 0.5, {alpha:1});
  }

  public function hidePreloader():void {
   TweenLite.to(preloader, 0.5, {alpha:0, onComplete:function(){
    removeChild(preloader);   
   }});
  }

  public function loadImage(url):void {
   imageLoader.contentLoaderInfo.addEventListener(ProgressEvent.PROGRESS, loaderProgressStatus);
   imageLoader.contentLoaderInfo.addEventListener(Event.COMPLETE, loaderComplete);
   var imageURL:URLRequest = new URLRequest(url);
   imageLoader.load(imageURL);
   showPreloader(); 
   function loaderProgressStatus(e:ProgressEvent) {  
    //trace(e.bytesLoaded, e.bytesTotal); 
   }
   function loaderComplete(e:Event) {
    hidePreloader();
    imageLoader.alpha = 0;
    imageLoader.y = (stage.stageHeight-imageLoader.height)/2;
    addChild(imageLoader);
    TweenLite.to(imageLoader, 2, {alpha:1});
   }
  }

  public function movieClipExists(mc:MovieClip):Boolean {
   return mc != null && contains(mc);
  }




 }

}

Answer 1

This summer I wrote a graph class for creating line charts in AS3. Based on that experience, I can say that you code is quite good, but I prefer explicitly writing this keyword before class variables and functions. So if you declare some similar local variables, you won't get lost between them.

Answer 2

One thing to watch out for: to avoid memory leaks use weak references with event listeners. This is suggested, because even if all references are removed from an object, but it has a function which is registered to an other object's event, the object won't be garbage collected (because there is still some kind of reference), unless you used a weak reference.

More info: http://www.gskinner.com/blog/archives/2006/07/as3_weakly_refe.html

This article also points out the problem with anonymous functions and weak reference, which can cause a lof of headache for the unwary.

Also, I have to add that this is only an issue if you are excessively creating and "destroying" objects.

Answer 3

Full disclosure: I am an anal and pedantic reviewer so don't take it personally. In general your code is good.

• Why the ENTER_FRAME delay before init? Perhaps you have a good reason but I don't know of one. You should be able to call init directly from your constructor.
• I know Adobe recommends instantiation statically in the classes property definition, but I consider it bad style. I do it in the constructor or wherever I am certain it will first be used (it is most often the case I know where it is meant to be initialized).
• Good use of lib functions (TweenLite) for animations. +1
• Remove commented out code that is not in use. Use source control if you think you'll need to go back to an older revision one day. Never leave commented out code in live source - it is code rot.
• for (var i:Number <-- should use an int for a integer counter.
• xml.leiligheter.leilighet.length() <-- consider caching this in var len:int = ...
• var flatName = <-- don't be lazy and forget your types.
• xml.leiligheter.leilighet[i].navn <-- digging pretty deep there. You might rather do:

    var children:XMLList = xml.leiligheter.leilighet;
    for each(var node:XML in children) 
    {
        var flatName:String = String(node.navn);
        if(movieClipExists(building[flatName])) 
        {
            building[flatName].addEventListener(MouseEvent.MOUSE_UP, flatMouseClick);
            building[flatName].addEventListener(MouseEvent.MOUSE_OVER, flatMouseOver);
            building[flatName].addEventListener(MouseEvent.MOUSE_OUT, flatMouseOut);
            building[flatName].alpha = 0;
            TweenLite.to(building[flatName], 2, {alpha:0.5, tint:0x00FF23});
        }
    }

• public function showInfoBox <-- empty function? Code rot, get rid of it.
• trace('flatName: '+flatName); <-- I make a habit of removing traces. There are several in your code. Keep them in there only as long as you absolutely need them then get rid of them.
• in function showFlat more traces and poor XML handling.
• TweenLite.to(imageLoader, 0.5, {alpha:0, onComplete:function(){ <-- I know it is easy to write an anonymous function here but it is better practise IMHO to make a member function. I tend only to use anonymous functions when I want a closure.
• TweenLite.to(building, 0.7, {alpha:0, onComplete:showFlat(e.target.name)}); <-- this calls show flat immediately ... it doesn't wait for the onComplete like you may think.
• building[e.target.name].buttonMode = true; you should set this only once when you are constructing the buildings (during handleXML) and not everytime it is moused over
• In loadImage you have two named functions defined inside named loaderProgressStatus and loaderComplete. These should be defined at the class level and since loaderProgressStatus is empty -- don't define it and don't assign it as a listener (code-cruft)
• Avoid the wildcard symbol (*) in your import statements. It may be a little more work to explicitly declare each import but it minimizes the chances of a possible name-clash.

Like I said - I am very picky. Your code style is consistent which shows good potential. There are only a few major types of error:

1. Unused code should never remain in an active file.
2. traces should only exist during debugging and should disappear ASAP.
3. Avoid anonymous functions and inner functions unless you necessitate closures.
4. Become better familiar with E4X style handling of XML so you don't have to dig unnecessarily into XML structures.
5. Make sure you always use types and the most appropriate type (int over Number)
6. Watch out assigning callbacks - you are executing a function at the wrong time.

Answer 4

When you import a class, try not to use wildcard "*". e.g.

import flash.events.*;

Please specify exactly which class you are going to use.

reasons:

1. When your project grows larger, you might need to import other class, it may conflict with one of the classes which is imported using wild card, and the class which causing conflict is not being used at all.
2. Easier for debugging. You know which package need to look for when something goes wrong.