[Beginner Level] Code cleanup: Simple Quiz & Objects

[zephiraya]

For visualisation:

(http://imgur.com/GFRm4hQ)

 

Hi HTML5 Game Devs Forum. Please bear with my beginner-ish questions!

Project aim:

A simple 'game' where the user chooses between two options (which highlights their selection inside an ellipse) then locks in their answer. 

My issue(s) with my current code:

It works, but it's pretty bad. Basically everything is duplicated which is obviously poor coding, issue is, I don't know how to use JS objects in concert with Phaser, or how to check it if it were an object,  and what I've googled either doesn't look right or goes over my head massively.

 

Normally i think i'd want to use an object with some attributes, ala:

var button = { sprite = blah; thecorrectanswer = yes/no; currentlyselected = false} 

etc, and then i'd be able to have a bunch of them if i wanted.

Because currently I'm limited to two, and it's straight painful to look at. Similarly, the 'selection ellipses' are just shown/hidden based on clicks.

 

Ideally i think this'd be better if it was a sub-function(terminology?) of the question object that drew an ellipse if it was selected, however when i muddled with .graphics in Phaser to draw an ellipse, it wouldn't come on screen when i toggled it's visibility - do i need an update function (is it a loop that runs continuously?) and get it in there?

 

tl;dr: I can see my code is poorly styled, not very extendable/generalisable, but I don't know enough about JS/Phaser to fix: Can objects (and HTML5gamedev.com) be my saviour here?

var game = new Phaser.Game(800, 600, Phaser.AUTO, 'phaser-example', { preload: preload, create: create });var text;var counter = 0;var currentselection = "";var correctselection = "option1";function preload () {    game.load.image('option1','assets/option1.png');    game.load.image('option2','assets/option2.png');    game.load.image('selected','assets/circled.png');    game.load.image('checkbutton','assets/checkbutton.png');}function create() {		game.stage.backgroundColor = '#182d3b';    //  This creates a simple sprite that is using our loaded image and    //  displays it on-screen and assign it to a variable	var o1_xpos = game.world.centerX-200;	var o1_ypos = game.world.centerY;    var option1 = game.add.sprite(o1_xpos, o1_ypos, 'option1');    option1selected = game.add.sprite(o1_xpos, o1_ypos, 'selected');    option1selected.visible = false;    	var o2_xpos = game.world.centerX+200;	var o2_ypos = game.world.centerY;    var option2 = game.add.sprite(o2_xpos, o2_ypos, 'option2');    option2selected = game.add.sprite(o2_xpos, o2_ypos, 'selected');    option2selected.visible = false;    //  Moves the image anchor to the middle, so it centers inside the game properly    option1.anchor.set(0.5);    option2.anchor.set(0.5);    option1selected.anchor.set(0.5);    option2selected.anchor.set(0.5);    //  Enables all kind of input actions on this image (click, etc)    option1.inputEnabled = true;    option2.inputEnabled = true;    text = game.add.text(250, 16, '', { fill: '#ffffff' });    text.anchor.set(0.5);    //Assign callbacks onclick    option1.events.onInputDown.add(option1onclick, this);    option2.events.onInputDown.add(option2onclick, this);        //Check Button    button = game.add.button(game.world.centerX, 500, 'checkbutton', checkbuttonClick, this, 0, 0, 0);    //  Moves the image anchor to the middle, so it centers inside the game properly    button.anchor.set(0.5);    }function option1onclick () {	//text: debug purposes only    text.text = "You clicked option1";    option1selected.visible = true;    option2selected.visible = false;    currentselection = "option1";}function option2onclick () {	//text: debug purposes only    text.text = "You clicked option2";    option1selected.visible = false;    option2selected.visible = true;    currentselection = "option2";}function checkbuttonClick() {	if (currentselection != ""){		if (currentselection === correctselection){			text.text = "CORRECT";			//Do other 'on correct' actions here		} else {			text.text = "INCORRECT";		}	} else {		//do nothing because nothing has been selected	}}

[jfhs]

Hi zephiraya!

 

var button = { sprite = blah; thecorrectanswer = yes/no; currentlyselected = false} 

 

That's not how you create an object in JavaScript. You can use a literal to define an object like this:

var button = {    sprite: someSprite,    correctAnswer: true,    selected: false};

You can write a function that creates and initializes the objects for you:

function createOption (id, x, y) {        var option = {        id: id,        button: game.add.sprite(x, y, "option" + id),        selected: game.add.sprite(x, y, 'selected')    };        option.button.anchor.set(0.5);    option.selected.anchor.set(0.5);        option.button.inputEnabled = true;    option.selected.visible = false;        return option;}

Why is this better? Because you have less duplicated code: if you want to change something about all options, you now have only one place to look for.

 

You have more than one of the same kind of object, so it's probably a good idea to put those in an array instead of having variables for each:

var options = [    createOption(1, game.world.centerX - 200, game.world.centerY),    createOption(2, game.world.centerX + 200, game.world.centerY)];

Why is this better? Because when you want to do something with every button, you can now do it like this:

options.forEach(function (option, index) {    // option is the current item in the array,    // index is the offset of the current item    // inside the array (options[index] === option)});

There's a lot of other things you could do to make the code better, but listing them all here would take ages. E.g. you probably should have your questions and answers in a separate data structure as well. This way you can load them from a JSON file and if you want to add a new question, you can just add it to the JSON file without changing your code.

 

Also, your "option1selected" and "option2selected" are no variables because you forgot the "var". What you did there was create attributes of the global object instead. That's a no-go because it can lead to hard-to-find bugs. Please also do yourself and others a favor and indent your lines correctly.

 

I would advise you to use a JavaScript linter to check your code. Not only will it find potential bugs for you, but you can learn a lot about writing better code. You can use linters online, integrated into your editor or from the command line. One of the most popular linters is JSHint:

http://jshint.com/

 

 

 

 

[zephiraya]

Thankyou for your help.

I now have a problem that the selection circle doesn't appear. I've added a function (toggleSelection) to the options in the literal object definiton for an option, but i can't get the commands right to get it to register the Phaser .onInputDown to work such that when clicked, it shows the highlight sprite. Any guidance here? Is scope an issue here?

function create() {	game.stage.backgroundColor = '#182d3b';	text = game.add.text(250, 16, '', { fill: '#ffffff' });    text.anchor.set(0.5);    	var options = [	               createOption(1, game.world.centerX - 200, game.world.centerY),	               createOption(2, game.world.centerX + 200, game.world.centerY)	               ];	}function createOption (id, x, y) {        /*var option = new Object();        option.id = id;        option.button = game.add.sprite(x, y, "option" + id);        option.selected = game.add.sprite(x, y, 'selected');*/    var option = {            id: id,            button: game.add.sprite(x, y, "option" + id),            selected: game.add.sprite(x, y, 'selected'),            toggleSelection: function () {            	this.selected.visible = true;            }        };        option.button.anchor.set(0.5);    option.selected.anchor.set(0.5);        option.button.inputEnabled = true;    option.selected.visible = false;        //create onclick event    option.events.onInputDown.add(toggleSelection, this);            return option;}

[jfhs]

    //create onclick event    option.events.onInputDown.add(toggleSelection, this);

 

This should throw an error because toggleSelection isn't defined and/or because the 'option' object doesn't have an 'events' property. I think what you actually meant here is 'option.button.events' instead of 'option.events'. Also, you've put the 'toggleSelection' function inside the 'option' object, so you can't just reference its property name as a variable. Instead, you need to use the reference to the 'option' object like so:

option.button.events.onInputDown.add(option.toggleSelection.bind(option), this);

Please note: I'm not a Phaser user, and the documentation page for events doesn't really explain what kind of 'this' should be used as the second argument to .add(), but if it is the context of the callback, then you could also do it like this:

option.button.events.onInputDown.add(option.toggleSelection, option);

In JavaScript, when you pass a function around that uses 'this', you need to bind it to the object it belongs to because the 'this' context will be lost otherwise. It's a quirk in the language many people learning it stumble over. It means that when you call the method like this:

option.toggleSelection();

Then 'this' in 'toggleSelection' references the 'option' object. But if instead you pass the method to a function or assign it to a variable like so:

var toggle = option.toggleSelection;toggle();

Then 'this' in 'toggleSelection' doesn't point to the 'option' object anymore.

 

Every function in JavaScript is also an object that has a .bind() method. This method creates another function that does exactly what the original function does, but it makes sure that 'this' inside the new function points to the right object.

 

In your code, it's also possible to avoid using 'this' altogether if you write 'toggleSelection' not as a method but as a simple inner function of your 'createOption' function:

var option = {    // ...}function toggleSelection () {    option.selected.visible = !option.selected.visible;}option.button.events.onInputDown.add(toggleSelection);

You can reference 'option' inside the 'toggleSelection' function, because in JavaScript, inner functions (that is, functions defined inside of other functions) have access to the outer function's variables. This is called a 'closure' because the inner function 'closes over' the variables of the outer function.

 

If you write your 'toggleSelection' function like this, it doesn't matter what the 'this' context of the function is because you don't rely on it.

 

Please also note that I changed the 'toggleSelection' function so that it does what its name implies: toggle. The way you wrote the function in your example code it always sets the visibility of 'option.selected' to true.

 

You can read more about closures here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures

 

And here's a more in-depth explanation of 'this' in JavaScript: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/this

[zephiraya]

EDIT: DISREGARD THIS QUESTION, I FIGURED OUT A WORKING SOLUTION

 

Wow thanks again for the massive help.

 

Interesingly, i did try originally to use option instead of this, as well trying an inner function. It required both in concert to work though, which is where i never got to before.

 

Now I'm trying to fix the behaviour of the selection: Making it de-select every other clicked option and turning its own on. I tried using your forEach before but i thought it'd be easier to understand if i separated out the function

Note the onclick has been renamed to reflect what it (should do) now, and I made options a global array in order to access it from inside createOption

 

The forEach here isn't doing it's job and unselecting, is anything obviously wrong? 

    function onClick () {    	options.forEach(unselect);    	select();    }    function select () {    	option.selected.visible = true;    }    function unselect() {    	option.selected.visible = false;    }    option.button.events.onInputDown.add(onClick, option);

My instinct is telling me perhaps i haven't set the targets right, and while it may be iterating over options array, it's not doing anything to the individual option

so following https://msdn.microsoft.com/en-us/library/ff679980(v=vs.94).aspx I did the following

    function onClick () {    	options.forEach(unselect, opt);    	select();    }    function select () {    	//option.selected.visible = !option.selected.visible;    	this.selected.visible = true;    }    function unselect() {    	this.selected.visible = false;    }

where opt is meant to be the optional argument that sets the 'this' keyword to the right value inside the array.

 

This also doesn't work.