Making a square move counter-clockwise around the edge

Question

This is so ugly code. There must be better way to write it, but I'm noob to AS3.

The goal is to make a square move at even speed counter-clockwise around the edge and on call reverseDirection();, changeColor();, or spin();.

I'm trying to understand Tweenlite vars onStart and onStartParams. I'm sure that it can be used to write this elegantly with only one TweenLite.to.

I use AS3, Starling and Tweenlite. This is the code I'm trying to simplify:

public class Square extends Sprite implements ISquare {

public const square:Quad = new Quad(100, 100);
private var tweeningDirection:String;

public function Square() {
}

public function draw():void{
    addChild(square);

    square.pivotX = square.width / 2;
    square.pivotY = square.height / 2;
    square.x = 50;
    square.y = 50;
    square.color = 0x4500FF;
    tweeningDirection = new String;

    down();
}

public function reverseDirection():void{
    TweenLite.killTweensOf(square);

    switch (tweeningDirection){
        case "down":
            stoppedDown();
            break;
        case "left":
            stoppedLeft();
            break;
        case "up":
            stoppedUp();
        case "right":
            stoppedRight();
            break;
        case "stoppedDown":
            down();
            break;
        case "stoppedLeft":
            left();
            break;
        case "stoppedUp":
            up();
            break;
        case "stoppedRight":
            right();
            break;
        case "rDown":
            stoppedRDown();
            break;
        case "rLeft":
            stoppedRLeft();
            break;
        case "rUp":
            stoppedRUp();
            break;
        case "rRight":
            stoppedRRight();
            break;
        case "stoppedRDown":
            rDown();
            break;
        case "stoppedRLeft":
            rLeft();
            break;
        case "stoppedRUp":
            rUp();
            break;
        case "stoppedRRight":
            rRight();
            break;
    }
}

private function down():void {
    tweeningDirection = "down";
    TweenLite.to(square, stage.stageHeight / 200, {y: stage.stageHeight - 50, ease: Linear.easeNone, onComplete: right});
}

private function stoppedDown():void {
    tweeningDirection = "stoppedDown";
    TweenLite.to(square, square.y / 200, {y: 50, ease: Linear.easeNone, onComplete: rRight});
}

private function right():void {
    tweeningDirection = "right";
    TweenLite.to(square, stage.stageWidth / 200, {x: stage.stageWidth - 50, ease: Linear.easeNone, onComplete: up});
}

private function stoppedRight():void {
    tweeningDirection = "stoppedRight";
    TweenLite.to(square, square.x / 200, {x: 50, ease: Linear.easeNone, onComplete: rUp});
}

private function up():void {
    tweeningDirection = "up";
    TweenLite.to(square, stage.stageHeight / 200, {y:50, ease: Linear.easeNone, onComplete: left});
}

private function stoppedUp():void {
    tweeningDirection = "stoppedUp";
    TweenLite.to(square, (stage.stageHeight - square.y) / 200, {y: stage.stageHeight - 50, ease: Linear.easeNone, onComplete: rLeft});
}

private function left():void {
    tweeningDirection = "left";
    TweenLite.to(square, stage.stageWidth / 200, {x: 50, ease: Linear.easeNone, onComplete: down});
}

private function stoppedLeft():void {
    tweeningDirection = "stoppedLeft";
    TweenLite.to(square, (stage.stageWidth - square.x) / 200, {x: stage.stageWidth - 50, ease: Linear.easeNone, onComplete: rDown});
}

private function rRight():void {
    tweeningDirection = "rRight";
    TweenLite.to(square, stage.stageWidth / 200, {x: stage.stageWidth - 50, ease: Linear.easeNone, onComplete: rDown});
}

private function stoppedRRight():void {
    tweeningDirection = "stoppedRRight";
    TweenLite.to(square, square.x / 200, {x: 50, ease: Linear.easeNone, onComplete: down});
}

private function rDown():void {
    tweeningDirection = "rDown";
    TweenLite.to(square, stage.stageHeight / 200, {y: stage.stageHeight - 50, ease: Linear.easeNone, onComplete: rLeft});
}

private function stoppedRDown():void {
    tweeningDirection = "stoppedRDown";
    TweenLite.to(square, square.y / 200, {y: 50, ease: Linear.easeNone, onComplete: left});
}

private function rLeft():void {
    tweeningDirection = "rLeft";
    TweenLite.to(square, stage.stageWidth / 200, {x: 50, ease: Linear.easeNone, onComplete: rUp});
}

private function stoppedRLeft():void {
    tweeningDirection = "stoppedRLeft";
    TweenLite.to(square, (stage.stageWidth - square.x) / 200, {x: stage.stageWidth - 50, ease: Linear.easeNone, onComplete: up});
}

private function rUp():void {
    tweeningDirection = "rUp";
    TweenLite.to(square, stage.stageHeight / 200, {y:50, ease: Linear.easeNone, onComplete: rRight});
}

private function stoppedRUp():void {
    tweeningDirection = "stoppedRUp";
    TweenLite.to(square, (stage.stageHeight - square.y) / 200, {y: stage.stageHeight - 50, ease: Linear.easeNone, onComplete: right});
}

}
}

Answer 1

After looking thoroughly at your code, I found a bug. Please be sure this is intended behavior, and if not, fix it.

In reverseDirection, you have this:

switch (tweeningDirection){
    case "down":
        stoppedDown();
        break;
    case "left":
        stoppedLeft();
        break;
    case "up":
        stoppedUp();//MISSED BREAK!
    case "right":
        stoppedRight();
        break; 

And, as I pointed out with a comment in this code snippet, you've forgotten a break. This will cause both stoppedUp and stoppedRight to be called when reversing during the up tween, potentially causing a crash again like you experienced before.

---

Magic numbers: replace all literal numerics that are not 0 or 1 with a constant so you know what it means. I think I can see a BORDER_SIZE in there, with all those 50's. Or do they mean something else? I'm also seeing / 200 as a speed multiplier of sorts.

Now, for the actual question.

For each tween you use, you need a speed, a target, and another tween that will follow it. Additionally, you'll need to have a reverse tween.

So I'd write a class named ReversableTween containing something like this:

getTweenSpeed(objectToTween:?):Number //Square is Quad, but maybe we want a superclass of that
createTweenVars(onComplete:Function):Object //creates the entire third argument for TweenLite.to, assuming the Linear.easeNone needs to be added always
getReverseTween():ReversableTween //gets the r version
var attribute:String;//holds x or y, but could hold wacky stuff like width or height
var targetValue:Number;//holds the target value for the attribute (attribute = x, targetValue = 50)
var reverseTween:ReversableTween;
var nextTween:ReversableTween;
//constructor, getters, setters, whatever

Then I'd set things up like so...

var LEFT_SIDE:Number = BORDER_SIZE;
var RIGHT_SIDE:Number = stage.stageWidth - BORDER_SIZE;
var TOP_SIDE:Number = BORDER_SIZE;
var BOTTOM_SIDE:Number = stage.stageHeight - BORDER_SIZE;

Creating the objects...

var downTween:ReversableTween = new ReversableTween("y", BOTTOM_SIDE);
var stoppedDownTween:ReversableTween = new ReversableTween("y", TOP_SIDE);
//many more

Apparently, it seems the constructor takes attribute:String, value:Number. Because that's the information that we can provide directly.

Linking them via functions...

setReverse(downTween, stoppedDownTween);//sets a's reverse tween to be b and vice versa
//more invocations, per pair
setNext([downTween, rightTween, upTween, leftTween]);//1.next = 2, 2.next is 3... 4.next is (5 doesn't exist, so first element) 1
//more invocations, per circle (normal, reverse)
stoppedDownTween.setNextTween(rRightTween);
//the stopped versions need to be manually linked, 
//since they switch between circle 1 (normal) and circle 2 (reverse)

And then I'd have 1 onComplete function, which looks at the current tween and grabs the next...

public function onComplete(e:Event):void {
    currentTween = currentTween.getNext();
    TweenLite.to(square, currentTween.getTweenSpeed(square), currentTween.createTweenVars(this.onComplete));
}

I'd also get rid of the switch, instead only having to do

public function reverseDirection():void {
    TweenLite.killTweensOf(square);
    currentTween = currentTween.getReverseTween();
    TweenLite.to(square, currentTween.getTweenSpeed(square), currentTween.createTweenVars(this.onComplete));
}

Now, I just had to copy paste something, which is an indication that it could be better. So lets alter onComplete and reverseDirection to use startTween instead... Also, see that I moved the killTweensOf - we only want 1 tween active at all times anyway.

public function onComplete(e:Event):void {
    currentTween = currentTween.getNext();
    startTween(currentTween);
}

public function reverseDirection():void {
    currentTween = currentTween.getReverseTween();
    startTween(currentTween);
}

public function startTween(tween:ReversableTween):void {
    TweenLite.killTweensOf(square);
    TweenLite.to(square, currentTween.getTweenSpeed(square), currentTween.createTweenVars(this.onComplete));
}

Lastly, lets fix the naming, it just doesn't feel right for me...

public function nextTween(e:Event = null):void {
    currentTween = currentTween.getNext();
    startTween(currentTween);
}

public function reverseTween():void {
    currentTween = currentTween.getReverseTween();
    startTween(currentTween);
}

public function startTween(tween:ReversableTween):void {
    TweenLite.killTweensOf(square);
    TweenLite.to(square, currentTween.getTweenSpeed(square), currentTween.createTweenVars(this.nextTween));
}

I added default null to onComplete so I can call it without having to pass an argument. I then renamed it to nextTween (since that's what it does!). I also renamed reverseDirection to reverseTween, so that I have 3 neatly named functions that look the same: nextTween, reverseTween and startTween.

And well, that's the changes I would make.

What I didn't provide for you is the implementation of the ReversableTween class. That's so that you can do the refactoring yourself, rather than just copying my answer and handing it in =). Since it's an assignment, be sure to understand WHY each change is made, and don't blindly apply it. Any changes you make now should be justifiable by you without having to look up my answer.

I don't think you can get rid of having to define the numbers. Save for nasty golfing, you can't abstract away data without hardcoding assumptions. For instance, I just turned that 50 into BORDER_SIZE... thus if you wanted it to go from 20 to stageWidth-10, you'd have to alter the code more than just a single constant.

That said, what you can do is prevent yourself from repeating yourself. I only have to define the tweens once. Control is done by three small functions.

Let's sum up what I did, and why I did so...

• I made constants to prevent duplication of numbers that can look cryptic. This benefits the readability of your code, as well as the maintainability.
• I removed part of the hardcoded tween values by wrapping them in a class. This allows you to define the tween values somewhere else. Like in some initialization function, or maybe from something that reads in a file (hint hint)... and yes, now that I look at it, it makes for a pretty weak reason, so lets expand;
• By using the data as objects rather than data, I could write generic functions for manipulating them (setReverse, which sets A's reverse tween to B and vice versa, and setNext, which goes through an array, setting each element's next tween to the next element, and in case of the last element, to the first element).

What I did wrong, now that I look back...

• I assumed there would only be one attribute that can change. This is wrong. If you wanted to move from 50,50 to 250,250 in one move, both x and y would have to change. I guess you'll have to solve that.
• setReverse is still used too often (once for each pair). Maybe there's a way to link two "chains" together?
• The stopped versions still need to be linked on their own. Could there be some improvement in this area?

I guess you'll have to fix that.

If you don't understand some of this or need more implementation details, let me know and I'll try to explain it a bit.