15
\$\begingroup\$

In an effort to complete this month's code challenge, I've started off with the basics. Below I have for review, a class that I can call to create the square/button a user would click to mark a spot on a Tic Tac Toe board. I haven't done any game logic, so I could very easily be forgetting something my buttons need to have.

  • Would this package stand up to any future additions?
  • How is my objected orientation here?
  • Is there anything I can do to compact it?
  • Am I following common practices?

package 
{

    import flash.display.MovieClip;
    import flash.events.MouseEvent;

    public class SquareSpace extends MovieClip 
    {

        public static const SQUARE_LENGTH:int = 30;
        public static const PLAYER_A_OWNED_SQUARE:int = 0;
        public static const PLAYER_B_OWNED_SQUARE:int = 1;
        public static const NEUTRAL_SQUARE:int = 2;
        public static const AVAILABLE_SQUARE:int = 3;
        public static const STATE_COLORS:Array = [0xFF3333,0x3333FF,0xDDDDDD,0x33FF33];

        private var _state:int = AVAILABLE_SQUARE;
        private var _square:MovieClip;

        public function SquareSpace(boxX:int, boxY:int) 
        {
            _square = formSquare();
            _square.x = boxX;
            _square.y = boxY;
            _square.buttonMode = true;
        }

        public function addSquare():MovieClip 
        {
            _square.addEventListener(MouseEvent.CLICK, sendClick);
            return _square;
        }

        private function sendClick(event:MouseEvent):void 
        {
            dispatchEvent(new MouseEvent(MouseEvent.CLICK));
        }

        public function changeSquareState(sqrState:int):void 
        {
            if (sqrState >= PLAYER_A_OWNED_SQUARE && sqrState <= AVAILABLE_SQUARE) {
                _state = sqrState;
                _square.graphics.beginFill(STATE_COLORS[sqrState]);
                _square.graphics.drawRect(0,0,SQUARE_LENGTH,SQUARE_LENGTH);
                _square.graphics.endFill();
            } else {
                throw new ArgumentError('State not acceptable.');
            }
        }

        public function getState():int 
        {
            return _state;
        }

        private function formSquare():MovieClip 
        {
            var mvclp:MovieClip = new MovieClip();
            mvclp.graphics.beginFill(STATE_COLORS[NEUTRAL_SQUARE]);
            mvclp.graphics.drawRect(0,0,SQUARE_LENGTH,SQUARE_LENGTH);
            mvclp.graphics.endFill();
            return mvclp;
        }
    }
}

Here is some calling code. I dispatch that click event (sendClick()) so my actual SquareSpace variable can handle clicks, rather than having the event listeners for the movieclip square outside of the class.

private function setUpBlankSubBoard (xOffset:int, yOffset:int, topBoard:int):void
        {
            for (var boxIndX:int = 1; boxIndX <= BOARD_LENGTH; boxIndX++) {
                for (var boxIndY:int = 1; boxIndY <= BOARD_LENGTH; boxIndY++) {
                    var newSqr:SquareSpace = new SquareSpace(
                     xOffset + (SquareSpace.SQUARE_LENGTH * 1.2) * boxIndX,
                     yOffset + (SquareSpace.SQUARE_LENGTH * 1.2) * boxIndY);
                    newSqr.addEventListener (MouseEvent.CLICK, squareChosen); // Called an unrelated function
                    addChild (newSqr.addSquare());
                }
            }
        }
\$\endgroup\$
8
\$\begingroup\$

Overall this is looking pretty. I do have a few questions and suggestions based on what I see.

  1. Constructor
    Your class extends MovieClip therefore it IS a Movieclip. What is the purpose of the inner _square class variable? Is SquareSpace meant to be purely a logic controller class with _square being the visual representation of the square? If so there is no need for SquareSpace to extend MovieClip. I would look at EventDispatcher or just plain Object.

  2. addSquare/sendClick
    I'm not sure I'm understanding the purpose of these methods. Can you explain your thought behind them?

  3. changeSquareState
    For state control/detection you generally want to use a switch statement. That way adding more states in the future is much easier.

  4. The State Constants
    This is more of a cosmetic comment but I usually prepend any constants with the word STATE_ so that you/other developers can easily tell that those are values intended to be used for state control and passed into changeSquareState.

  5. Argument Error
    A really minor comment but I like to include the state which is not acceptable in the error statement. Makes debugging in the future easier. Not a big deal though.

  6. Future Design
    As you build out this game do you plan on using the Flash timeline to position and configure your assets or are you going pure AS3?

  7. Package
    Generally you want to organize each of your classes into a package. If you're not using a proper IDE like FlashBuilder, FlashDevelop, or FDT this will be a bit of a pain but it's good practice none the less. Packages generally look like reversed domain names so for example your SquareSpace class might be in the following package com.alex.ultimatetictactoe.square

\$\endgroup\$
  • \$\begingroup\$ Thanks for #1: I switched to EventDispatcher #2: I've edited my question to include some calling code. Hopefully that might help! #3: Changed! #4: Good idea, renamed my consts! #5: Got it #6 It's all in AS3, nothing on the timeline! #7: I didn't think this project would last, so I didn't bother structure my directories when I started! But now that it looks fairly good, I've added some packages. \$\endgroup\$ – Alex L Feb 25 '14 at 4:58
  • 1
    \$\begingroup\$ #2 That makes sense. I usually name my event handlers using the convention on{item}{eventType} so I would rename sendClick to onSquareClick. That way I know it's an event handler at first glance. \$\endgroup\$ – mbaker3 Feb 25 '14 at 16:24
8
\$\begingroup\$

Besides what mbaker3 wrote, I just have one thing to add (for now at least), I consider it quite important though:

    public function addSquare():MovieClip 
    {
        _square.addEventListener(MouseEvent.CLICK, sendClick);
        return _square;
    }
  1. This method doesn't add the actual square to anything and therefore it's name is misleading
  2. This method adds an event listener each time it is called, so calling it twice would add two event listeners (If I remember my ActionScript correctly)

I strongly suggest adding the event listener in the constructor instead, and only returning _square in the method, and renaming the method to getSquare().

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.