My first game (and a code review?)

[buttonsrtoys]

I just finished my first JS project and my first Phaser project -- http://richardcoutts.com/game.html. It's mostly just a knockoff of the classic asteroids game with some different graphics. Had a lot of fun making it. Thanks to all who answered my newbie questions and to the Phaser developer(s) for putting together such an awesome package!

 

This is a lot to ask, so I won't be offended if no one takes me up on it, but I'd appreciate some feedback on my code. I'm an old C++ programmer, so comfortable with OO concepts, but not sure I implemented them correctly, or whether I broke JS conventions, missed opportunities for better patterns are maybe tighter code or just plain did something silly?

 

Edit: even though my app's short (~1000 lines), I'd like to hold it to the standards of a larger app.

[MichaelD]

This might come as a surprise (I know it took me a long time to accept this) but with javascript 90% of the times if it is working then its fine, I came from Java and the real hardship to learn javascript was the realization that you can make something with countless ways and the result has almost the same performance or effect, sure there are best practices that you can look up and have in mind but it really is so ridiculously easy and free (until you hit some bug that is totally nightmare to debug).

 

Javascript for C++, Java and other strict-type languages is like a catholic schoolgirl going to a rave-party in Mykonos at night!

[Skeptron]

Could you link a URL to your repo? Might be easier for code review

[buttonsrtoys]

@Skeptron, I don't have a repository but will try to get something up tonight. If it's not too much trouble, you can "view source" on the url to the game (it's not minified).

 

@MichaelID, thanks for the thoughts. I added an edit to my post. Even though my app's short, I tried holding it to higher standards.

[Skeptron]

Ok first comment : don't put everything into a single file. Make files for classes, with a nice package hierarchy. (or is it your build system that puts everything into game.html?).

 

It's gonna be wayyyyyy easier for us to review

 

Then I think you also should have a look at GameStates. This could be great to better split your game into lifecycle phases : loading screen (loading assets), game, scoreboard etc. And it adds practical advantages (like being sure that your assets are loaded before starting the game, for example).

[buttonsrtoys]

Skeptron, thanks for the great feedback. My build environment (Cloud9) didn't put everything into a single html file. I'll break it down as you suggest.

 

I was aware of GameStates when I was coding and will certainly use it on my next Phaser app. For this one I was more focused on learning JavaScript, so wanted to code the states myself with a singleton.

[drhayes]

MichaelD is right. Getting it working and finished and polished is a big achievement. And, yeah, putting things in different files will help you a lot. Especially once you get your source out of the <script> tag and into its own file, I bet your editor will help you navigate.

 

I noticed you had an Actor <- Shipt <- Mothership class hierarchy going. Inheritance hierarchies in JS don't earn you much because of the weak typing and, if too deep, can cause performance issues because of how JavaScript has to walk the prototype chain looking for properties and methods. You might be better off using mixins that modify the prototype of the class you're making instead; for example, add common methods directly to the prototypes of the objects that share them instead of inheriting from a parent.

 

Similarly, don't bother with getters and setters as you have them now. JS (and, by extension, Ruby and Python) don't really *do* encapsulation like that. In later versions of JS you can use a getter syntax; handy for computed values and whatnot. But don't worry about private variables. 

 

Looks like for Ship you added methods in the constructor using "this.thing = function() {}" vs. using the prototype. For the single Ship, not a big deal. For classes where you're going to make lots of objects (e.g. the buggeroids or custom Phaser particles) it eats up memory for each copy of the object because each one carries its own copies of the methods.

 

For all the Beetle* classes, I wonder if you could get away with not doing classes at all. The way you did it is not "wrong", just offering a different perspective. Since there's not a lot of state/behavior beyond the bugs being sprites you might be able to more easily manage all that stuff in your game state or in the bug manager.

 

Another thing to try for that big case statement at the end (you didn't do it wrong!) is to make each one of those case statements its own function, then assign that function to GameStateMgr.currentState. Then, on update, just call GameStateMgr.currentState(). Each state can re-assign GameStateMgr.currentState as needed for transitions.

[buttonsrtoys]

Wow. Thanks Dr.! Lots of great feedback to chew on. Regarding the "this.thing = function() {}" approach, I needed this to accessed private vars, but it sounds like it's too high a price to pay for private vars? Looks like I over did it with private vars in general, which is also why I had so many get/set functions. 

 

My inclination is to bail on privates (except in special cases) and go with the prefix approach:

this.publicVar; // I'm a public varthis._privateVar; // I'm a public var, too, but my underscore says 'keep out'

The approach is described here -- https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Contributor_s_Guide/Private_Properties. Is this one of the more popular approaches? I'm not a slave to popular opinion. Just trying to pick up some good habits.

[drhayes]

It is, yeah. It's a solid practice and lots of people and libraries do that. You'll be in good company.

 

The "this.thing = function() {}" approach is totally valid, by the way, and a great solution to maintaining encapsulation. Good tool for the toolbox.

 

FWIW, I don't use private variables in JS. There are mechanisms available in JS to create getters/setters later that look exactly like naked property accesses if you need to backtrack it for, e.g., a computed value or a notification upon set or something. I think at the application level (us) vs. the library level (Phaser manages its private vars like this) there's less of a need for encapsulation and it makes for more keyboard typing than necessary. But that's just me. ( ;

[buttonsrtoys]

I incorporated the coding suggestions, included splitting the code into separate files, moving functions to the prototype, and losing most of the private vars. The updated code is at http://richardcoutts.com/game.html. If anyone's up for reviewing the updated code, I'd be surprised and appreciative! 

[drhayes]

You're back! Do you do all your work with Cloud9? How is it? Does it do version control? Does it allow SSH? Have you done stuff besides JS in it? Have you seen nitrous.io?

 

Okay, enough with my questions. ( =

 

'use strict', awesome. Phaser.ArrayUtils.getRandomItem and, I believe, Phaser.Math.wrap do what your two functions do in functions.js. You should check out Phaser.Point#angle as well, to get the angle between two things. Actually, all those utility functions in Math, Point, ArrayUtils... lots of good stuff there.

 

Why is the code around playing a sound in the DeviceMgr? The "sound = new Object();" parts? (also, you can say "sound = {};" to get a new object, same thing but slightly faster and less to type) I've only recently started using sounds in my games and I've been okay with loading one sound and playing it over and over.

 

You store the game as a "static" in Actor.Game, but all of your actors are sprites and thus have "this.game". Almost everything in phaser has a "this.game" property.

 

You have a typo in "setHighSCore", but it looks like editor autocomplete has been saving you. ( =

[buttonsrtoys]

Thanks Doc!!! Your feedback's super helpful yet again. Regarding C9, I'm just getting back into programming after a 10-year absence, so certainly not the best one to ask. For what it's worth, I love the freedom of being able to pull up my IDE anywhere. Most of the time its on a Chromebook on the couch after my toddler goes down. Other times, I sneaking some game programming in at work to break up the day. Right now, it's in my basement on my PC workstation. I did look at Codenvy, but there JS support is almost non-existent. Regarding version control, I haven't explored the interface too much extent, but I believe there's support for Github, which I suspect most users are using for controlling their versions. There's also an automatic Time Machine-like backup for each file, so if you do something silly, you can slide the time bar back up to 10 days or so to retrieve something you messed up. I haven't done any non-JS with it yet except for HTML. I think it's biggest user base are website developers, but don't quote me on anything I'm telling you. It's more just my impressions from using it so far.

 

The interface is nice and it uses ESLint or some equivalent to flag bad code before a run. One funny thing is it lets you set breakpoints but then they don't work when you run the code. Instead, they point you to the browsers environment for stepping through code, setting breakpoints, etc. So, I debug as best I can using C9's IDE and then run it inside C9, which provides a "Pop out into new window" button that creates a new Chrome window with the app running. Then, if it crashes or I want to walk through it, I use Chrome's Developer Tools.

 

My IDE experience 10 years ago was on Visual Studio and CodeWarrior and I don't feel like I'm missing anything major that I had back then. My one nit is that C9 doesn't "see" JS functions and classes in other files, so as you saw, you need to tell each file that your using something defined in another JS file. E.g., if you have the line 

this.group.physicsBodyType = Phaser.Physics.ARCADE

you need the comment 

/* global Phaser */

at the top of the file. Otherwise, your get a yellow [!] next to your line with the hover comment "warning, Phaser not defined". The warning's harmless, but if you don't put in the comment, you'll have these warnings peppered about and when there's one you really care about, you'll likely overlook it. That's why you saw all those "global" comment at the top of my file. I'm guessing they'll fix this in the future and it's not a big deal in the meantime.

 

Also, it's free! Provided your user space is less the 500Mb or so. Also, I was up and running in 5-10 minutes. Very easy environment to find your way around.

 

Regarding your latest feedback, I implemented your suggestions and update my code. Regarding putting sound in the DeviceMgr, I'm likely missing something, but felt like I needed a singleton to manage creating, playing, and stopping sounds. So, if a class wants to play a sound, it calls DeviceMgr.playSound('ring'). If 'ring' has not been instantiated, DeviceMgr creates an instance. Otherwise, it indexes the instance and plays it. I didn't want the classes tracking these instantiations. Or where you asking why I grouped this in with DeviceMgr instead of spinning off a separate manager? Which would be a valid comment. Or maybe a lazier/better fix would be to a better name for DeviceMgr, like IOMgr?

 

Anyway, thanks a ton!

[drhayes]

I help to run a Coder Dojo in my area to teach kids to code because I like being a helpful know-it-all. ( ;  Any tiny hurdle can blow their efforts out of the water; I had this one kid who was crying because the computer was low on batteries and had a fussy trackpad. Like that.

 

Cloud9 is a godsend under those circumstances. One of the other mentors has used it a lot but I haven't had a lot of experience with it. Was just starting to get into it. So thanks for the feedback!

 

I was asking about the sound 'cuz I've not done a ton of it and, right now, I've got something similar going on. I have this one Phaser.Plugin that I wrote (basically a shooting manager) that is responsible for playing the sounds (and firing the bullets, keeping them in groups, etc). I had to do that for some of the sounds because they don't sound great overlapped and I wanted some space between them; harder to coordinate with a bunch of independent sprites. I was curious if it was something similar for you. ( =

[buttonsrtoys]

Good on you for teaching kids to code. That's much needed instruction these days and JS seems like the perfect language for kids.

 

Regarding the sound, I wasn't sure how much managing Phaser did so asked a general question that Rich clarified for me -- http://www.html5gamedevs.com/topic/17006-need-some-clarification-on-the-soundmanager/. Based on his response, I went with a simple manager class.