Simon-says in AS3 - Prototyping functionality

Question

My non-serious entry for Simon Says.

Based on literal interpretation of the rules:

Create a UI with four colored buttons that light up in a random pattern. After displaying the pattern, the player must repeat the pattern by clicking the buttons in proper order. The pattern gets longer each time the player completes the pattern. If the player presses a wrong button, the game ends.

Basically:

• Must have UI
• 4 colored buttons
• that light up
• in a random pattern
• player repeats the pattern
• input: clicking
• when player completes pattern, pattern is made longer
• when player pushes wrong button, game ends.

This means that, as you'll see:

• I didn't add different colors
• There's no player feedback for clicking
• The game ends by having the buttons disappear

Perfect for prototyping.

I restrained myself from programming a version that did not re-display the pattern, since it's not stated in the requirements (But it would make a rather boring game if I did).

I wrote this quickly. I wrote this without an IDE.

My goal:

Learning how to prototype better. I'm interested in finding ways to test game mechanics faster. That means I don't want to write much code and I'm not interested in polishing it up nice. I just want it to work so I can test whether it's any fun. If it is, I can rebuild a clean version. For the answers, this means I'm looking for how I could alter the way I write code from the start, rather than refining it pass by pass.

The code:

package
{
 import flash.display.Sprite;
 import flash.events.*;
 public class Main extends Sprite {
 public var playerTurn:Boolean = false;
 private var pattern:Array = new Array();
 private var patternIndex:uint = 0;
 private var playerPattern:Array = new Array();
 private var blinkCounter:uint = 0;
 private var buttons:Array = new Array();
 private var graphicSprite:Sprite = new Sprite();
 public function Main(){
 if(stage)init();
 else addEventListener(Event.ADDED_TO_STAGE, init);
 }
 public function init(e:Event = null):void {
 removeEventListener(Event.ADDED_TO_STAGE, init);
 for(var i:uint = 0; i < 4; i++){
 buttons[i] = new SimonButton(this, i);
 }
 addChild(graphicSprite);
 addPattern();
 }
 public function addPattern():void {
 playerTurn = false;
 pattern.push(Math.floor(Math.random()*4));
 patternIndex = 0;
 addEventListener(Event.ENTER_FRAME, displayPattern);
 }
 public function displayPattern(e:Event = null):void {
 graphicSprite.graphics.clear();
 graphicSprite.graphics.beginFill(0xFFFFFF, 0.9);
 graphicSprite.graphics.drawRect(pattern[patternIndex]*110, 0, 100, 25);
 graphicSprite.graphics.endFill();
 blinkCounter++;
 if(blinkCounter >= 20){
 patternIndex++;
 if(patternIndex == pattern.length){
 removeEventListener(Event.ENTER_FRAME, displayPattern);
 playerTurn = true;
 graphicSprite.graphics.clear();
 patternIndex = 0;
 }
 blinkCounter = 0;
 }
 }
 public function addPlayerPattern(choice:uint):void {
 playerPattern.push(choice);
 for(var i:uint = 0; i < playerPattern.length; i++){
 if(playerPattern[i] != pattern[i]){
 for(var j:uint = 0; j < buttons.length; j++){
 removeChild(buttons[j]);
 }
 return;
 }
 }
 if(playerPattern.length == pattern.length){
 playerPattern = new Array();
 addPattern();
 }
 }
 }
}
import flash.display.Sprite;
import flash.events.*;
 class SimonButton extends Sprite {
 private var simon:Main = null;
 private var index:uint = 0;
 public function SimonButton(game:Main, buttonIndex:uint){
 simon = game;
 index = buttonIndex;
 simon.addChild(this);
 this.x = index*110;
 this.graphics.beginFill(0x007700);
 this.graphics.drawRect(0, 0, 100, 25);
 this.graphics.endFill();
 addEventListener(MouseEvent.CLICK, clickBtn);
 }
 public function clickBtn(e:Event = null):void{
 if(simon.playerTurn){
 simon.addPlayerPattern(index);
 }
 }
 }

Running version here:

http://wonderfl.net/c/rGp8

• wvxvw: The most widely accepted code style says that you should not use initializers, if they are the same as default values (so don't use var foo:Boolean = false, use var foo:Boolean. Same rules say that you shouldn't use new Array(), unless you give it length argument. Use [] instead. Binary operators require space on both sides. Single letter variable names are bad (except in few cases, like loop iterators). Put empty line between definitions. Separate keywords from the rest of the text with a space.
• Pimgd: Do not answer in comments, write an answer instead @wvxvw

Answer 1

Disclaimer: I'm not an Actionscript programmer, however, I think there are a few universal things/improvements that apply to your program:

• Aligning large blocks of variable initialization might benefit readability. Making similar things look similar is a good idea. The first block on class Main would probably benefit from this:

  public var playerTurn : Boolean = false;
  private var pattern : Array = new Array();
  private var patternIndex : uint = 0;
  private var playerPattern : Array = new Array();
  private var blinkCounter : uint = 0;
  private var buttons : Array = new Array();
  private var graphicSprite : Sprite = new Sprite();
  

  Now it is pretty clear to distinguish between the variable name, its type and default initialization.

• This type of if/else setup is harder to read. I would advise against:

   public function Main(){
   if(stage)init();
   else addEventListener(Event.ADDED_TO_STAGE, init);
   }
  

  Put each command in its own line. I also suggest always providing { } even for single line statements, to avoid problems like accidental highjacking of lines and errors introduced by bad code indenting.

• You have quite a few hardcoded constants, like colors (0x007700) and positioning of sprites (100, 25) and also blinkCounter, which is being compared against 20 (why 20?). Even if this is prototyping code, avoiding excessive hardcoding makes it easier to expand the prototype later. Hardcode the obvious stuff that won't change, like 4 for the number of squares in the game, but avoid hardcoding things that are very likely to change during the prototyping phase, such as the colors and sprite positions.

• Overall, your code seems nice, though I'm finding it a little too tightly packed. I think some blank lines between functions would make it more pleasant to read.

• Simon Forsberg: Aligning large blocks of variable initializations is something that is highly opinion based and not something I would recommend personally.
• glampert: @SimonAndréForsberg, well perhaps. My "school of thought" is that similar things should look similar. That plus the fact that the brain is pretty good at finding patterns, doesn't make this point seem that subjective to me.
• Pimgd: It's enough for me to not make the post warrant an upvote. Seriously? You recommend ALIGNING variables as first-write activity? That way you get hit with all the cons of aligning variables: You have to make changes each time you add more variables.
• Pimgd: the if/else there is the way FlashDevelop generates the code when you create a new project. I manually wrote it this time since my IDE was notepad, but I didn't think much of it. The constants and the blank lines are good ideas.
• glampert: @Pimgd, I made the suggestion because I believe the aligned text reads better, the same way that well formatted and spaced plain text is easier to read. This is open to discussion, I'm not stating a final point here, that's why I've used the words "might benefit readability". And BTW, you shouldn't be worried about changing code if the change improves readability.

Answer 2

public function addPlayerPattern(choice:uint):void {
 playerPattern.push(choice);
 for(var i:uint = 0; i < playerPattern.length; i++){
 if(playerPattern[i] != pattern[i]){
 for(var j:uint = 0; j < buttons.length; j++){
 removeChild(buttons[j]);
 }
 return;
 }
 }
 if(playerPattern.length == pattern.length){
 playerPattern = new Array();
 addPattern();
 }
}

This bit of code contains a weird structure.

It's for "adding a player choice to pattern", according to the function name.

And that's what the first line does. playerPattern.push(choice); adds the player's choice to the list.

Then it goes on to validate. Why is it validating here?

Instead, make a function called handlePlayerTurn(playerChoice:uint):void. Thinking in turns makes it easier to handle flow too.

Its body can be something like this:

public function handlePlayerTurn(playerChoice:uint):void {
 addPlayerChoice(playerChoice);
 if(!validatePlayerPattern()) {
 endGame();
 } else if (playerTurnDone()) {
 simonTurn();
 }
}

validatePlayerPattern would check if it matches the secret pattern so far. playerTurnDone would check if the player had input enough moves so far. simonTurn is what addPattern is now.

Your second pass

The code you posted was the first pass of the program. The first write, so to say. Well, once you're done with your first write, you should match if each function name does what it says it does and not much else. That's how you could have discovered the turn design too.

It's good that you extracted the button, but there's more to extract. Pattern would be the next thing to extract. It can have "matches" and "equals" and "extend" functions. That would help simplify some of the array functions you're using in addPattern, displayPattern and addPlayerPattern.

There might be a third class you can make responsible for handling the blinking animation so that you don't have this graphical logic stuck between your game logic.

Lastly, not having an IDE at hand is no excuse for not cleaning the code up later. Just because you're prototyping things doesn't mean you're not allowed to use your tools. Autoformat does wonders, and generating code can help even more.

Decluttering main

I know from experience that it's best to keep Main.as as clean as you can. That way you can abuse it later if you need to test something, and then delete it without regrets afterwards. Right now there is no such testing place.

To declutter main, start by taking a look at its member variables:

 public var playerTurn:Boolean = false;
 private var pattern:Array = new Array();
 private var patternIndex:uint = 0;
 private var playerPattern:Array = new Array();
 private var blinkCounter:uint = 0;
 private var buttons:Array = new Array();
 private var graphicSprite:Sprite = new Sprite();

And assign them a responsibility.

 public var playerTurn:Boolean = false; //turn keeping
 private var pattern:Array = new Array(); //player pattern
 private var patternIndex:uint = 0; //pattern
 private var playerPattern:Array = new Array(); //player pattern
 private var blinkCounter:uint = 0; //blink/graphical display
 private var buttons:Array = new Array(); //graphical display
 private var graphicSprite:Sprite = new Sprite(); //blink/graphical display

The turn keeping, you could perhaps keep in Main. But as I said before, pattern could be a separate class. Additionally, the graphical display is something that could be offloaded to some other class.

Presumably, you could even go so far as to make main just handle the obtaining of stage, and then creating a new SimonGame instance and passing it a stage instance. Then you can have a class like Game handle the model related part - the patterns, the turn keeping, etc. A class like GameInterface could be responsible for the graphical display and the blinking.

Bonus points: This would also make it easier to implement a proper reset, rather than just making all the buttons vanish.

• Pimgd: Accepted this answer because I still feel @glampert's answer is wrong for suggesting formatting variables as a first pass effort.