Archive for the ‘Clean Code’ Category

Document Classes… with Interfaces!

Often, a Flash developer will want to load an external swf and assign the contents to a strongly typed variable so as to take advantage of the compile-time and run-time safeguards associated with strong typing, as well as the intellisense capabilities of mature coding environments like FlashDevelop. Unfortunately, it can be less straightforward than we’d like to simply import the class definition for the external swf’s Document Class, particularly if our external content has any library items set to export with a Class name, or relies on Flash to automatically declare stage instances.

For an example of how this goes wrong, see the following zip file:

If you compile the LoadedContent.fla everything works just fine. The problem comes when you try to compile the ContentLoader, which mysteriously complains about problems in LoadedContent.as. You may wonder why there are suddenly problems in that class now when it compiles perfectly fine in the LoadedContent.fla. Let’s open up LoadedContent.as, as the answer is there:

package  {
	import flash.display.MovieClip;
	import flash.text.TextField;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class LoadedContent extends MovieClip{
		public function get asset1():LoadedContentAsset {
			return _asset1; 
		}
 
		private var _asset1:LoadedContentAsset;
		private var _asset2:LoadedContentAsset;		
		private var _someClip:MovieClip;
 
 
		public function LoadedContent() {			
 
		}
 
		public function showImages():void {
			_asset1 = new Asset1(); // Declared in the .fla library
			_asset2 = new Asset2(); // Declared in the .fla library
 
			_asset1.x = 100;
			_asset1.y = 100;
			_asset2.x = _asset1.x + _asset1.width;
			_asset2.y = 100;
 
			this.addChild(_asset1);
			this.addChild(_asset2);
 
			_someClip = new SomeClip(); // Declared in the .fla library
 
			_someClip.x = _asset2.x + _asset2.width;
			_someClip.y = 100;
 
			this.addChild(_someClip);
		}
 
		public function sayHello():String {
			this.text_txt.text = "Hello World"; // exists on the timeline.
			return this.text_txt.text;
		}
 
	}
 
}

Notice how, when we compile the ContentLoader.fla and read the error messages they’re all pointing to lines of code that I’ve marked with a comment. The reason LoadedContent.fla compiles correctly is because, in the case of Asset1, Asset2, and SomeClip, the classes it’s instantiating exist in its library. LoadedContent.fla knows where to look for them, and has a meaningful point of reference to them. ContentLoader does not. It just sees a Class definition trying to perform actions with other classes that don’t exist. The same problem exists with the reference to this.text_txt in LoadedContent.as. Because LoadedContent.fla is using the default compiler setting of “Automatically Declare Stage Instances”, it can simply reference the text field without actually calling this.getChildByName() or similar. All ContentLoader sees when it reads that line in the LoadedContent definition is “You’re trying to perform an action on an undeclared variable!”

What to do? We could simply cast the loaded content as MovieClip or Object, since those are Dynamic classes. That would silence the error, but it would leave us without the benefits of a strongly typed environment. Since The Horseman openly admits his bias in favor of strongly typed code, he feels compelled to illustrate how this can still be done. The key is interfaces.

An interface is nothing more than a “contract”. When a Class implements an Interface, it promises that the functions and properties of the interface will be available for use by other data, eg: declared as public. Absolutely no implementation details are present in the Interface. When put in context of the LoadedContent situation, it means that it doesn’t define, know about, or care about any such thing as Asset1, Asset2, SomeClip, or this.text_txt. Here’s an example:

package  {
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public interface ILoadableContent {
 
		function showImages():void;
		function sayHello():String;		
 
	}
 
}

By convention in ActionScript, Interfaces always start with the letter ‘I’. This differentiates them from Class definitions at a glance. What this Interface tells us is that any Class that implements it is guaranteed to have public functions with the same function signatures as shown above. Luckily for us, LoadedContent already has those functions defined! It just needs to officially implement the interface like so:

package  {
	import flash.display.MovieClip;
	import flash.text.TextField;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 * 
	 *    Implement the interface here in the LoadedContent.
	 * 
	 */
	public class LoadedContent extends MovieClip implements ILoadableContent{
 
	// and down further are the public functions expected by the Interface.

Now in our ContentLoader we just need to replace the references to LoadableContent with references to ILoadableContent.

package  {
	import flash.display.DisplayObject;
	import flash.display.Loader;
	import flash.display.MovieClip;
	import flash.events.Event;
	import flash.net.URLRequest;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class ContentLoader extends MovieClip{
 
		// Type the content as the Interface, not the concrete Class.
		private var _loadedContent:ILoadableContent;
		private var _loader:Loader;
 
 
		public function ContentLoader() {
			_loader = new Loader();
			_loader.contentLoaderInfo.addEventListener(Event.COMPLETE, onEventCompleteLoader);
			_loader.load(new URLRequest("LoadedContent.swf"));
		}
 
		private function onEventCompleteLoader(e:Event):void {
			e.currentTarget.removeEventListener(Event.COMPLETE, onEventCompleteLoader);
			_loadedContent = e.target.content as ILoadableContent;
			_loader.unload();
			_loader = null;
 
			// Casting because an interface cannot be a DisplayObject, sadly.
			this.addChild(_loadedContent as DisplayObject); 
 
			_loadedContent.showImages();
			var hello:String = _loadedContent.sayHello();
			trace(hello);
		}
 
	}
 
}

Now the ContentLoader.fla will compile correctly. Since it isn’t looking at the concrete LoadedContent, but instead only looking at the Interface ILoadedContent there are no more conflicts to resolve. The only thing about this that is slightly bothersome is the fact that if you have a reference to a DisplayObject but only know of it by its interface, you cannot use it as an argument in a call to addChild() without first casting it as a DisplayObject. This is however, a very small tradeoff for the compile-time, run-time, and author-time benefits of working in a strongly-typed manner with a modern coding environment.

Here is a zip file containing the new and improved code.

The implications of this reach further than loading, however. If you code your components so that they implement interfaces, you can switch them out with each other more easily than if they are simply Classes, and referenced as such.

EDIT: These days instead of coercing the ILoadableContent into being a DisplayObject, I would instead give the ILoadableContent interface a get view function like this

package  {
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public interface ILoadableContent {
 
		function get view():DisplayObject;
		function showImages():void;
		function sayHello():String;		
 
	}
 
}

That way, when you call addChild you no longer have to blindly assume that the ILoadableContent is a DisplayObject. This is also nice in that it allows the ILoadableContent to choose a displayObject to act as its own view. Maybe it should be the view and it returns ‘this’. Maybe it has some other object it wants to be the view instead? It doesn’t have to matter with the get view function.

Tags: , , , ,

1 Comment


Knock Down The If-ful Tower

Novice coders, as well as intermediate coders who are simply in a rush often end up with code structures like these:

public function importantLogic(player:Player):void {
 
	if (player.isMoving) {
		if (player.isJumping) {
			// do something with player...
			//...
			//...
		}else if (player.isAboutToJump) {
			// do something with player...
			//...
			//...
		}
	}else if (player.isCrouching) {
		// do something with player...
		//...
		//...
	}else if (player.isAboutToMove) {
		//...
		//...
	}else if (player.isAboutToJump) {
		//...
		//...
	}
 
 
 
}

I call these structures “If-ful Towers” (pronounced like “Eiffel Towers” without the leading “E”). Obviously there is nothing wrong with the humble if-else statement, or its cousin if-else-if. Our code would hardly work without branching logic, after all. One of my co-workers and I were talking about this sort of structure and how it’s highly indicative of the “Broken Windows” syndrome discussed in “The Pragmatic Programmer“. How do you maintain code that is structured this way? It already looks imposing, and there’s not even any real code written in the blocks.

If you look closely at this particular code snippet, you’ll notice that we’re testing the state of the Player instance. The Player clearly can exhibit a great number of states. Most of them are simple boolean properties. Many of those properties are mutually exclusive. Who knows what sort of havoc could happen if somehow the player thinks that it isMoving, and isCrouching, and isJumping, and isAboutToMove all at the same time? Couldn’t we knock down the If-ful Tower if we were to just be a little smarter about how we determine the state of the Player?

package {
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 * 
	 *   Here we have a base class for a certain set of actions to perform.  
	 *   We can create many different subclasses, such as....
	 * 
	 *       WalkingContext
	 *       CrouchingContext
	 *       JumpingContext
	 */
	public class Context {
 
		public function Context() {
 
		}
 
 
		// override in your subclass...
		public function execute(player:Player, foo:*):void {
			// player is the Player object.
			// foo is some data that player should be concerned with.
		}
 
	}
 
}
 
 
 
 
 
// And here's the very basics of Player...
package {
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 * 
	 *   The player has a context that is called upon to execute
	 *   on command.  Since there is only one context active at
	 *   any given time, we can be assured that as long as we 
	 *   supply the *one* correct context that the action taken
	 *   will be expected and desired.
	 * 
	 */
	public class Player extends MovieClip{
 
		private var _context:Context;
		public function get context():Context{
			return _context;
		}
 
		public function set context(value:Context):void{
			_context = value;
		}
 
 
 
		public function Player() {
 
		}
 
 
	}
 
}

Now it is completely unambiguous precisely which “state” the Player instance happens to be in at any given moment. There can be only one! As long as you have correctly assigned the context, simply calling upon it to execute should be all you need to do. Where once stood the If-ful Tower, we now have this:

public function importantLogic(player:Player):void {
 
	// That's it.  
	player.context.execute(player, 10);
 
 
 
 
}

Wow. Things just got a lot more readable in this code block. This is of course not the only way to knock down the If-ful Tower, and depending on your circumstances there may be better ways. Indeed, the player doesn’t necessarily need to have a reference to the Context object. That could perhaps be stored elsewhere, and simply accessed when needed.

Perhaps you need to implement collision handling between Player and an Enemy? It’s probably tempting to just build up yet another If-ful Tower replete with deeply nested logic and branches… but that’s a tower to topple another day!

Tags: , ,

2 Comments


At the Logic Gates

How many times have you found yourself with a tangle of code that looks something like this?

/*
While the code is in ActionScript 3, it could easily 
be something you've seen in the likes of Java, C#, 
or almost any other similar language.
*/
 
package  {
	import flash.display.MovieClip;
	import flash.events.Event;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class UtterMadness {
 
		private var bugsDance:Boolean = false;
		private var voiceOverCompleted:Boolean = false;
		private var fireworksExploded:Boolean = false
		private var bannerSign:MovieClip;
		private var youWinClip:MovieClip;
 
		public function UtterMadness() {
			this.playVoiceOver();
			this.makeBugsDance();
			this.shootFireworks();
			this.unfurlBannerSign();
		}
 
		private function onBugsDanceComplete(e:Event):void {
			bugsDance = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && bannerSign.currentFrame == bannerSign.totalFrames && youWinClip.currentFrame == 1) {
				youWinClip.gotoAndPlay("win");
				bugsDance = false;
				voiceOverCompleted = false;
				fireworksExploded = false;
				bannerSign.gotoAndPlay("goAway");				
			}
		}
 
		private function onVoiceOverComplete(e:Event):void {
			voiceOverCompleted = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && bannerSign.currentFrame == bannerSign.totalFrames && youWinClip.currentFrame == 1) {
				youWinClip.gotoAndPlay("win");
				bugsDance = false;
				voiceOverCompleted = false;
				fireworksExploded = false;
				bannerSign.gotoAndPlay("goAway");				
			}
		}
 
		private function onFireworksExploded(e:Event):void {
			fireworksExploded = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && bannerSign.currentFrame == bannerSign.totalFrames && youWinClip.currentFrame == 1) {
				youWinClip.gotoAndPlay("win");
				bugsDance = false;
				voiceOverCompleted = false;
				fireworksExploded = false;
				bannerSign.gotoAndPlay("goAway");				
			}
		}
 
		private function onBannerSignComplete(e:Event):void {
			if (bugsDance && fireworksExploded && voiceOverCompleted && bannerSign.currentFrame == bannerSign.totalFrames && youWinClip.currentFrame == 1) {
				youWinClip.gotoAndPlay("win");
				bugsDance = false;
				voiceOverCompleted = false;
				fireworksExploded = false;
				bannerSign.gotoAndPlay("goAway");				
			}
		}
 
	}
 
}

Sometimes, you need to make certain things happen as a result of some action or event in the program. In this hypothetical scenario, we have a MovieClip in the “youWinClip” variable that we’re clearly very interested in playing, but only if the following conditions are true:

  • Some Bugs on the screen have danced.
  • Some voice-over has completed playing.
  • Some fireworks have gone off.
  • The “bannerSign” MovieClip is on its final animation frame.
  • The “youWinClip” is not already playing (let’s assume that’s what was meant by currentFrame == 1

So our solution up above is fine and dandy for now, if a bit verbose and repetative. Let’s expand on this, though. What if you’re working for a client who decides that there need to be more conditionals? Like for example, somewhere in the middle of all this mess they want you to set up a listener for a boulder tumbling down the hillside, which may or may not happen before or after anything else. But before you can do any of this, they add a requirement that the “youWinClip” must only play every 3rd time that all the previous requirements have taken place.

So you add some more code to this class and it looks something like the following:

package  {
	import flash.display.MovieClip;
	import flash.events.Event;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class UtterMadness {
 
		private var bugsDance:Boolean = false;
		private var voiceOverCompleted:Boolean = false;
		private var fireworksExploded:Boolean = false
		private var bannerSign:MovieClip;
		private var youWinClip:MovieClip;
		private var boulderRolled:Boolean;
		private var numberOfTimeConditionMet:int = 0;
		private var maxTimesToMeetCondition:int = 3;
 
 
		public function UtterMadness() {
 
		}
 
		private function onBugsDanceComplete(e:Event):void {
			bugsDance = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						youWinClip.gotoAndPlay("win");
						bannerSign.gotoAndPlay("goAway");	
						numberOfTimeConditionMet = 0;
					}
					bugsDance = false;
					boulderRolled = false;
					voiceOverCompleted = false;
					fireworksExploded = false;
 
			}
		}
 
		private function onVoiceOverComplete(e:Event):void {
			voiceOverCompleted = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						youWinClip.gotoAndPlay("win");
						bannerSign.gotoAndPlay("goAway");	
						numberOfTimeConditionMet = 0;
					}	
					bugsDance = false;
					boulderRolled = false;
					voiceOverCompleted = false;
					fireworksExploded = false;
 
			}
		}
 
		private function onFireworksExploded(e:Event):void {
			fireworksExploded = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						youWinClip.gotoAndPlay("win");
						bannerSign.gotoAndPlay("goAway");	
						numberOfTimeConditionMet = 0;
					}	
					bugsDance = false;
					boulderRolled = false;
					voiceOverCompleted = false;
					fireworksExploded = false;
 
			}
		}
 
		private function onBannerSignComplete(e:Event):void {
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						youWinClip.gotoAndPlay("win");
						bannerSign.gotoAndPlay("goAway");	
						numberOfTimeConditionMet = 0;
					}		
 
					bugsDance = false;
					voiceOverCompleted = false;
					fireworksExploded = false;
 
			}
		}
 
		private function onBoulderRoll(e:Event):void {
			boulderRolled = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						youWinClip.gotoAndPlay("win");
						bannerSign.gotoAndPlay("goAway");	
						numberOfTimeConditionMet = 0;
					}	
					bugsDance = false;
					boulderRolled = false;
					voiceOverCompleted = false;
					fireworksExploded = false;
 
			}
		}		
	}	
}

Wouldn’t you say this is getting ridiculous? All this copy/pasting is becoming as tedious for me to execute as it is for you to read. Additionally, the more often you have to repeat yourself the more likely you are to make a mistake in your code. Just think of what might happen if you forgot to re-set boulderRolled in onBannerSignComplete. Oh wait, we just did! Have fun debugging that one.

Did I tell you there were more changes on the way? Yeah, the client has some more requests. You’ll have to fit them into that hornet’s nest somewhere. Hope you don’t mind poring over your wall of text for the next week… or is it month? Or year?

Maybe if you just move the on-true results of those checks to its own function, that will make things easier?

 
		/////////////////////////////////////////////////////
		// starting near the end for the sake of sanity...
		/////////////////////////////////////////////////////
 
		private function onBoulderRoll(e:Event):void {
			boulderRolled = true;
			if (bugsDance && fireworksExploded && voiceOverCompleted && 
				bannerSign.currentFrame == bannerSign.totalFrames && 
				youWinClip.currentFrame == 1 && boulderRolled) {
 
					numberOfTimeConditionMet++
					if(numberOfTimeConditionMet == maxTimesToMeetCondition){
						// move all this code to its own function.
						this.youWin();
					}	
			}
		}
 
		private function youWin():void {
			boulderRolled = false;
			youWinClip.gotoAndPlay("win");
			bugsDance = false;
			voiceOverCompleted = false;
			fireworksExploded = false;
			bannerSign.gotoAndPlay("goAway");	
			numberOfTimeConditionMet = 0;
		}

That does in fact help to solve the issue of having to re-initialize our booleans and play our “goAway” and “win” animations, but it’s still a great deal of copy-pasting. Also, you have to remember to account for each new boolean statement in every function as the client adds them.

I’m sure the electrical engineers in the audience, as well as honest-to-goodness CompSci majors are rolling their eyes right now. They see something blindingly obvious that we’ve missed so far. We’re in desperate need of a logic gate. What if we were to put some of this Object Oriented Programming to good use and make ourselves a nice little module that we can plug into our code any time we need to test the truth of a logic statement?

The last time you’ll see the boolean comparisons : In your logic gate
Edit: This approach is admittedly slightly ham-fisted.

package  {
	import flash.display.MovieClip;
	import flash.events.Event;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class WinCondition {
 
		private var bugsDanceEvent:Event;
		private var voiceOverCompletedEvent:Event;
		private var fireworksEvent:Event;
		private var boulderRolledEvent:Event;
		private var bannerEvent:Event;
 
		private var numberOfTimeConditionMet:int = 0;
		private var maxTimesToMeetCondition:int = 3;
 
		public function WinCondition() {
 
		}
 
 
		// call this to determine whether it's okay to proceed.
		public function permission(e:Event, winClip:MovieClip):Boolean {
			var allow:Boolean = false;
 
			// string references to our properties...
			var events:Array = ["bugsDanceEvent", "voiceOverCompletedEvent", "fireworksEvent", "boulderRolledEvent", "bannerEvent"];
			// the expected event types we will encounter, mapped to the property references...
			var types:Array = ["danceType", "VOType", "fireworksType", "boulderType", "bannerType"];
 
			// eg: e.type == "VOType" will return an index of 1...
			var index:int = types.indexOf(e.type);
 
			// events[1] == "voiceOverCompletedEvent"...
			// therefore we're assigning to this["voiceOverCompletedEvent"]
			if (index >= 0) this[events[index]] = e;
 
			if (bugsDanceEvent && voiceOverCompletedEvent && 
				fireworksEvent && boulderRolledEvent && 
				bannerEvent && 	winClip.currentFrame == 1) {
 
					numberOfTimeConditionMet++;
 
					bugsDanceEvent = null;
					voiceOverCompletedEvent = null;
					fireworksEvent = null;
					boulderRolledEvent = null;
					bannerEvent = null;
			}
 
			if (numberOfTimeConditionMet == maxTimesToMeetCondition) {
				allow = true;
				numberOfTimeConditionMet = 0;
			}
 
			return allow;
		}	
 
	}
 
}

Notice how this new class “WinCondition” has precisely one function that returns a boolean value. Instead of keeping all the permissions code in the UtterMadness class, we’re keeping it in one place that will be unobtrusive, and easy to maintain. We don’t have to look at, think about, or care about any of that noise unless we specifically need to. I don’t know about you, but if I were to have to look at, think about, and otherwise be distracted by all that mess every time I wander through the class file for UtterMadness, I’m pretty sure I’d need some horse tranquilizers to keep my blood pressure in check.

Here’s what UtterMadness looks like with about 80% less madness

package  {
	import flash.display.MovieClip;
	import flash.events.Event;
 
	/**
	 * ...
	 * @author The Horseman @ Scriptocalypse.com
	 */
	public class UtterMadness {
 
		private var bannerSign:MovieClip;
		private var youWinClip:MovieClip;
		private var _winCondition:WinCondition;
 
 
		public function UtterMadness() {
			_winCondition = new WinCondition();
		}
 
		// e.type == "danceType";
		private function onBugsDanceComplete(e:Event):void {
			if (_winCondition(e, youWinClip)) this.youWin();	
		}
 
		// e.type == "VOType"
		private function onVoiceOverComplete(e:Event):void {
			if (_winCondition(e, youWinClip)) this.youWin();	
		}
 
		// e.type == "fireworksType"
		private function onFireworksExploded(e:Event):void {
			if (_winCondition(e, youWinClip)) this.youWin();	
		}
 
		// e.type == "bannerType"
		private function onBannerSignComplete(e:Event):void {
			if (_winCondition(e, youWinClip)) this.youWin();	
		}
 
		// e.type == "boulderType"
		private function onBoulderRoll(e:Event):void {
			if (_winCondition(e, youWinClip)) this.youWin();	
		}
 
		private function youWin():void {
			youWinClip.gotoAndPlay("win");
			bannerSign.gotoAndPlay("goAway");
		}
 
	}
 
}

Notice that I kept the youWin function in the main UtterMadness class. While it’s possible to place it in the permissions class and not bother with the Boolean return value, I find that I’d rather give the logic gate no more responsibility than to return the truth of an input. This way, I never have to open the WinCondition class again unless I’m modifying the conditions. If I were to place the ‘youWin’ logic in the WinCondition class, I’d have a second reason to modify or edit the file. Given how capricious our theoretical client is, it’s likely we’d have to add or change the youWin behavior many more times.

Changing code logic without a good reason is to be avoided because every time you edit or change a class or add more responsibilities to it, you increase the likelihood of injecting an error. For my money, “because we need to change what happens when you win” is not a good reason to modify or change “how you determine when you win”.

Tags: , ,

4 Comments