Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm making a text-based RPG in JavaScript.

It works but I'd like to know how to improve it, like code-wise.

Also, I want to know how to make it run on anything other than Internet Explorer.

JavaScript:

var canvas = document.createElement("canvas");
    canvas.id = "canvas";
    var ctx = canvas.getContext("2d");

    canvas.width = 1280;
    canvas.height = 720;

    document.body.appendChild(canvas);

var bgReady = false;
var bgImage = new Image();

bgImage.onload = function () {
    bgReady = true;
};
bgImage.src = "images/background.bmp";

var inventory = new Array();
var map = new Array();
var T = 404;

var events = ["nothing","enemy","shop"];
var gamestate = "";
var lines = new Array();
var text = "";
var textposx =32;
var textposy = 33;

var playerx = 50;
var playery = 0;
var dead = false;
var weapon = new Array();
var armor = new Array();
var shield = new Array();

var mapsizex = 100;
var mapsizey = 100;
var finished = false;
var enemytypes = new Array();

player = new Object()
player.health = 300;
player.gold = 0;
player.xp = 0;
player.level = 0;

// weapon object
function item(I,wname,acc,dmg,cat,b1,b2) {
          I = I || {};
          I.type = cat; // type 1 = weapon, 2 = armor, 3 = healing item, 4 = stat enchancements
          I.name = wname;
          I.catergory = cat;
          I.rating = acc;
          I.damage = dmg;
          I.bonus = new Array(b1,b2); //defence bonus,armor bonus
          return I;
          }

weapon.push(item(0,"short sword",100,60,"weapon",0,0));
armor.push(item(0,"leather armor",0,0,"armor",30,3));

//after adding a new stat, dont forget to edit the spawnMonster() function!
function enemy(I,ename,hp,acc,dmg,def,arm,exp,lvl){
  I = I || {};
  I.name = ename;
  I.health = hp;
  I.rating = acc;
  I.damage = dmg;
  I.defence = def;
  I.armor = arm;
  I.level = lvl;
  I.xp = exp;
  I.type = 1;
  return I;
}
//enemy database
//level 1 
enemytypes.push(enemy(0,"Thief",200,65,15,8,0,250,1));
enemytypes.push(enemy(0,"Zombie",250,60,10,15,0,300,1));
enemytypes.push(enemy(0,"Fallen",180,80,15,20,3,350,1));
enemytypes.push(enemy(0,"Yeti",325,70,10,10,0,450,1));
enemytypes.push(enemy(0,"Skeleton",150,60,10,10,0,200,1));
enemytypes.push(enemy(0,"Lion",125,120,25,10,0,300,1));


function randString()
{
    var rtext = "";
    var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

    for( var i=0; i < 5; i++ )
        rtext += possible.charAt(Math.floor(Math.random() * possible.length));

    return rtext;
}

function spawnMonster(place)
{
map[place]=new Array();
var x = Math.round(place/100);
var y = place%mapsizey;
var numgenmons = 1+Math.round(Math.random()*5);
for (var i = 0; i<numgenmons; i++){
var num = Math.round(Math.random()*5);
map[place].push(enemy(0,enemytypes[num].name,enemytypes[num].health,enemytypes[num].rating,enemytypes[num].damage,enemytypes[num].defence,enemytypes[num].armor,enemytypes[num].xp,enemytypes[num].level)); 
}
//name,hp,acc,dmg,def,arm,lvl
}

generate = function(){
for (var i = 0; i < (mapsizex*mapsizey); i++){
var r = Math.random();
if(r<0.5){
spawnMonster(i);
//50 % chance of encountering a monster/enemy
}

}
finished = true;


}
generate();

function eventCheck(){
var tempname = map[playerx*mapsizex+playery][0].name;
if(tempname!=undefined){
text = "you encountered a "+tempname+" !";
write();
gamestate = "enemy"; 
}

}

doShit = function(){

if(gamestate=="enemy"&&dead==false){
fight();
}
}

function targetCheck(){
T = 404;
for (var i=0;i<10;i++){
if(map[playerx*mapsizex+playery][i]!=undefined){
T = i;
return;
}
}

}

function fight(){

targetCheck();
if(T==404||map[playerx*mapsizex+playery][T].health<0){
alert("Target not found/dead!");
}

// player attacks enemy
var c = Math.random()*weapon[0].rating;
var d =  map[playerx*mapsizex+playery][T].defence;
//comparing your attack rating to enemies' defence rating
if(c>d){
map[playerx*mapsizex+playery][T].health-=(weapon[0].damage-map[playerx*mapsizex+playery][T].armor);
text = "You succesfully attacked the enemy with "+(weapon[0].damage-map[playerx*mapsizex+playery][T].armor)+".";
write();
}else{
text = "You tried to attack the enemy, but failed to inflict any damage.";
write();
//alert(c+" : "+d);
}
if(map[playerx*mapsizex+playery][T].health <= 0){
text = "You killed the "+map[playerx*mapsizex+playery][T].name+" !";
write();
player.xp+=map[playerx*mapsizex+playery][T].xp;
if(player.xp>Math.pow((player.level+1)*10,2)){
player.level++;
}
//50% chance of loot drop
if(Math.random()<0.5){
dropLoot(map[playerx*mapsizex+playery][T].level);
}
delete map[playerx*mapsizex+playery][T];
targetCheck();
if(T==404){
text = "There are no enemies left. You won the battle!";
write();
gamestate="";
}else{
text = "You encountered a "+map[playerx*mapsizex+playery][T].name+".";
write();
}
return;
}

//enemy attacks player
var c2 = Math.random()*map[playerx*mapsizex+playery][T].rating;
var d2 =  armor[0].bonus[0];
//comparing your defence rating to enemies' attack rating
if(c2>d2){
player.health-=(map[playerx*mapsizex+playery][T].damage-armor[0].bonus[1]);
text = "The enemy succesfully attacked you with "+(map[playerx*mapsizex+playery][T].damage-armor[0].bonus[1])+" damage.";
write();
if(player.health <= 1){dead = true,text="You died!",write()};
}else{
text = "The enemy failed to attack you.";
write();
//alert(c+" : "+d);
}

}

function dropLoot(level)
{
var kaas = Math.round(Math.pow((Math.random()*level*30),1.2));
player.gold+=kaas;
text = "You found "+kaas+" gold!";
write();
}

move = function(n){
if(gamestate==""&&dead==false){

if(n==3){
if(playerx<mapsizex){
playerx++;
text = "You moved east!";
}
}

if(n==2){
if(playerx>0){
playerx--;
text = "You moved west!";
}
}

if(n==1){
if(playery<mapsizey){
playery++;
text = "You moved north!";
}

}

write();
eventCheck();
}
}


function switchWeapon(){

}

write = function(){
//error was ~= instead of !=

if(textposy<650&text!=""){
lines.push(text);
}

}
function updateText()
{
ctx.fillText("\n \n",textposx, textposy); 
ctx.fillStyle = "rgb(0, 0, 0)";
ctx.font = "24px Helvetica";
ctx.textAlign = "left";
ctx.textBaseline = "top"; //top

textposy = 10;
textposx = 33;
if(lines.length<30){var scroll = 0}else{var scroll = lines.length-30};
for (var i=scroll; i<lines.length; i++)
{
textposy += 20;
ctx.fillText(lines[i],textposx,textposy); 
}
ctx.fillText("Your health: "+player.health,850,20); 
ctx.fillText("Your gold: "+player.gold,850,50); 
ctx.fillText("Your experience: "+player.xp + " / "+Math.pow((player.level+1)*10,2),850,80); 
ctx.fillText("Your level: "+player.level,850,110); 
}
setup = function() {


ctx.clearRect(300,0,1000,1000);
if (bgReady&&finished) {
        ctx.drawImage(bgImage,0,0); //draw background first
    }  
    updateText(); // after that, draw text
//alert(map[400][0].name);

}

//setInterval(setup,250);
//setTimeout(setup,1500);
setInterval(setup,250);

HTML:

<!DOCTYPE html>
<html lang="en">

<link rel="stylesheet" href="css/style.css" type="text/css">
    <head>
        <meta charset="utf-8">
        <title>rpg game</title>
    </head>
    <body>
<script src="javascript/diablo2.js"></script>
<div class="button">
<button onclick="move(3)">Go east</button>
<button onclick="move(2)">Go west</button>
<button onclick="move(1)">Go north</button>
<button onclick="doShit()">Action</button>
</div>
<div class = "switch">
<button onclick="switchWeapon()">Switch weapon</button>
</div>

    </body>
</html>

CSS:

.button {
    position: fixed;
    bottom: 245px;
    right: 1200px;
}
.switch{
    position: fixed;
    bottom: 780px;
    right: 710px;
}
share|improve this question
    
I read your post but i'm not sure what your question is. Please clarify if you want to be able to run this on any other browser, i.e., Chrome, Firefox, or if you want to be able to run it elsewhere such as a desktop. –  Simon Minshall Dec 18 '13 at 13:57
    
OH, I ment run it locally, not on a webserver. –  anon Dec 18 '13 at 14:04
    
Im asking for code-wise advice, how the same can be done better. If there are any other ways to achieve the same , that are simpler or better suited. –  anon Dec 18 '13 at 14:05
    
I can't help you with how to make it work for other browsers and IE, but I am working on several suggestions for you on how to clean up some code mess. –  Simon André Forsberg Dec 18 '13 at 14:14
1  
@none, you are in the right place ;) –  konijn Dec 18 '13 at 14:18
show 5 more comments

3 Answers

  • You have a lot of inconsistent indentation. (See link at the bottom for an easy way to fix this)

  • Inconsistent positioning of {, are you going to place it on it's own line (common C# style) or on the previous line (common Java style)? The JavaScript conventions is to use the same conventions like Java for this.

  • Put your global variables (playerx, playery, weapon, armor, shield, mapsizex, finished...) into an object, such as game. Perhaps weapon, armor and shield should be a part of the player object?

  • Unclear variable names: I, acc, arm (there's also nothing wrong in using damage instead of dmg) Use variable names that are not too long (theDamageOfTheEnemy would be too long) but still long enough to be descriptive. (damage is fine, or sometimes enemyDamage)

  • This comment indicates a code smell:

    //after adding a new stat, dont forget to edit the spawnMonster() function!

    When you have code that needs to be edited on multiple places in order to work, something is wrong. Instead consider using a enemy(enemytype) method that creates an enemy from an object. Instead of using nine (!!) parameters you can use one object that contains nine properties. Then you won't have to keep track of the order of the parameters. enemy({ hp: 20, damage: 40, experience: 23, level: 15 })

  • Perhaps my JavaScript knowledge is a bit rusty, but I = I || {}; looks strange to me. Instead, consider using if-else to handle that logic to make it more clear what you are doing. (Disregard this one, apparently it is very common and actually seems very useful. However, see tomdemuyt's answer for a better solution)

  • This method:

    generate = function(){
    for (var i = 0; i < (mapsizex*mapsizey); i++){
    var r = Math.random();
    if(r<0.5){
    spawnMonster(i);
    //50 % chance of encountering a monster/enemy
    }
    
    }
    finished = true;
    
    
    }
    

    The readability is almost zero, use proper indentation and spacing and get rid of some empty lines:

    generate = function() {
        for (var i = 0; i < (mapsizex * mapsizey); i++) {
            var r = Math.random();
            if (r < 0.5) {
                spawnMonster(i);
                //50 % chance of encountering a monster/enemy
            }
        }
        finished = true;
    }
    

    And it suddenly becomes so much more readable! This should be applied to all your code.

  • 1+Math.round(Math.random()*5); that's not the proper way to generate a number. You've used the correct way at one place, Math.floor(Math.random() * ...). Why is this? Because the first number (0) and the last number(5) will have slightly lower probability than the others. Math.random() is guaranteed to return a number from 0 (inclusive) to 1 (exclusive). Therefore, to generate one of the numbers 1 2 3 4 5 6, you should use: Math.floor(Math.random() * 6) + 1. This will give equal probabilities to all of the numbers.

Honestly, there are more things that needs to be cleaned up in your code, this is just a start. @tomdemuyt provided more very important things.

Consider using an Online JavaScript beautifier for your code every now and then. This tool doesn't take care of refactoring your code though, such as renaming variables and organizing variables into objects, and rewriting methods to use one parameters instead of nine, but it is a handy tool to clean up some things at least.

share|improve this answer
5  
variable ||= default is a common idiom in javascript, the question is just using a less common form of that idiom. –  amon Dec 18 '13 at 14:31
    
@amon OK, thanks. Then I guess the OP can disregard that part. –  Simon André Forsberg Dec 18 '13 at 14:32
    
@amon Just curious: What do you mean by "less common form"? x = x || default is pretty common. Or are you referring to x || (x = default) as the more common one? –  Flambino Dec 18 '13 at 14:54
3  
@amon ||= isn't valid javascript syntax though, so I assume you mean x || (x = y), right? –  Flambino Dec 18 '13 at 15:12
1  
With regards to the variable defaulting: in all JavaScript I've ever seen, var x = value || default; is the only form I've ever come across. It does indeed look a bit odd at first, but it tends to be extremely common. The long form with an if/else would be extremely noisy for any more than one or two few variables. –  Corbin Dec 18 '13 at 21:15
show 2 more comments

General

One of the best ways to learn a language is to write an RPG in it, because you can write working code with just basic knowledge, and then you keep polishing for a really long time.

Since your game has a respectable 300 lines which is definitely going to grow, I would advise you to organize your code into objects.

Approach

I gave your code a read through and comment on things as I see them.

UI

You should have a UI object with an init function which will build the UI.

var UI =
{
  init : function()
  {
    var canvas = document.createElement("canvas");
        canvas.id = "canvas";
        this.ctx = canvas.getContext("2d");

        canvas.width = 1280;
        canvas.height = 720;

        document.body.appendChild(canvas);

    var bgReady = false;
    var bgImage = new Image();

    bgImage.onload = function () {
        bgReady = true;
    };
    bgImage.src = "images/background.bmp";
  },
  write : function
  {
    ....
  },
  update: function
  {
    ....
  }
}

On a side note, you probably want those magical constants 1280 and 720 placed on top of your code in constants. Furthermore, you are loading a bmp, this might be the reason that this only works in IE, considering using a png or jpg file instead.

Rogue Variables

Your code then has a number of variables that you should try to place inside an object.

These could belong to a game object:

var events = ["nothing","enemy","shop"];
var gamestate = "";
var finished = false;

Note that you should try to stick to 1 naming convention, ideally lowerCamelCase, so you would use gameState instead of gamestate or game_state.

These could belong to the UI object:

var lines = new Array();
var text = "";
var textposx =32;
var textposy = 33;

These could be long to the player object that you have already

var playerx = 50;
var playery = 0;
var dead = false;
var enemytypes = [];
var weapon = [];
var armor = [];
var shield = [];

Notice how I replaced with new Array(); with [], which is considered more appropriate.

These could belong to a dungeonMap object:

var map = new Array();
var mapsizex = 100;
var mapsizey = 100;

you could even call the property size and assign { x : 100 , y : 100 } to it. A map, in essence, is a 2d array, you should really consider using that instead of a single array, it will make things much easier.

Also, I would advise you to use a different way to init the player object:

player = 
{
  health : 300,
  gold : 0,
  xp : 0,
  level : 0,
  x : 50,
  y : 0,
  dead :false,
  inventory : []  
}

instead of generating the object and assign properties to it (DRY).

Items

On to item/weapon/armor/shield :

// weapon object
function item(I,wname,acc,dmg,cat,b1,b2)
{
  I = I || {};
  I.type = cat; // type 1 = weapon, 2 = armor, 3 = healing item, 4 = stat enchancements
  I.name = wname;
  I.catergory = cat;
  I.rating = acc;
  I.damage = dmg;
  I.bonus = new Array(b1,b2); //defence bonus,armor bonus
  return I;
}

You have realized that armor/weapon/shield is close enough that you can use the same object, plus this will give you the option to create weapons with defense bonuses and armor with attack bonuses etc. I would change this function to be a constructor though and hence drop the I argument.

function Item( name, rating, damage, type, db, ab )
{
  this.name   = name;
  this.rating = rating;
  this.damage = damage;
  this.type = type; 
  this.bonus = { defense : db , armor : ab };
}

I dropped catergory(sic) since you store that in type.

You should have constants to represent the type, you could store them in item

Item.prototype.types = 
{
  WEAPON = 1,
  ARMOR = 2,
  HEALING = 3,
  STATBONUS = 4,
}

weapon.push(item(0,"short sword",100,60,"weapon",0,0));
armor.push(item(0,"leather armor",0,0,"armor",30,3));

could then become

weapons.push(new Item("short sword",100,60,Item.types.WEAPON,0,0));
armors.push(new Item("leather armor",0,0,Item.types.ARMOR,30,3));

Enemies

See Items

Functions

randString

  • randString is interesting, I am not sure what you would do with this, since you do not seem to use it anywhere, you might as well drop it.

spawnMonster

  • This function should be part of the dungeonMap object.
  • It could really use some indenting
  • var x = Math.round(place/100); -> you dont use x
  • var y = place%mapsizey -> you dont use y
  • numgenmons could be called monsterCount
  • 1+Math.round(Math.random()*5); -> You should scour your code and put all Math.random related code into an RNG object. ( Random Number Generator, all praise him, for he giveth and taketh away ).
  • To push a new monster, I would use the built in JSON to clone the monster map[place].push( JSON.parse( JSON.stringify( enemytypes[num] ) ) );

generate

  • Not sure why you chose generate = function(){ instead of function generate(){
  • So much more could be done there, think walls, treasures, traps, quests, NPC's etc.

eventCheck

  • You should replace direct map access with a call to the dungeonMap object
  • The code does not seem to check what happens if you come back to a cell where all monsters were defeated, bug ?
  • Indenting!
  • gamestate = "enemy"; -> "enemy" should be a constant, declared at the top or as part of the game class

targetCheck

  • 404 is cute
  • since the monster are added randomly to the array, just pick the first instead of a random one, then you can just check .length() of the cell and maybe you wont even need this function anymore, since it would be a one liner

fight

Combat, the crux of RPG!

  • Whenever you write alert(something), consider console.log( something ) or write( something), this way only developers will see those messages
  • text should be a parameter of write, globals are evil in general, but especially here
  • 'c' and 'd' , could be better named
  • From a design perspective, 'd' should have a random factor as well I feel
  • [playerx*mapsizex+playery][T] & weapon[0]
    • You could just var monster = [playerx*mapsizex+playery][T]
    • You could just var weapon = player.weapon[0]
    • Then you can write
if( attackScore > defenseScore ){
  var damage = weapon.damage - monster.armor
  monster.health-= damage;
  write( 'You succesfully attacked the enemy for ' + damage + '.' );
}
  • Indent your code!
  • The code where player attacks monster, and the code where monster counter attacks should really be in 2 different functions
  • Consider to give the monster a bonus if there are several monsters in the same place ?
  • Finally, player/monster level should probably increase the attackScore ?

dropLoot

  • Just had to say I love kaas as well

move

  • INDENT!
  • consider putting the directions into an object
directions = 
{
  1 : { x : +0 , y : +1 , s : 'north' },
  2 : { x : -1 , y : +0 , s : 'west'  },
  3 : { x : +1 , y : +0 , s : 'east'  },
  4 : { x : +0 , y : -1 , s : 'south' } /* Did you omit south on purpose ? */
};
  • with that object you could check directions[ n ] to check for a valid direction and modify player.x and player.y and build a string to show where the player is going

updateText

  • This should be part of a UI object
  • indent..
  • There are a ton of magical constants that you should tidy up
  • Use newlines, dont be tempted to do this : if(lines.length<30){var scroll = 0}else{var scroll = lines.length-30};
  • fillStyle, font, textAlign, and textBaseline should be part of initialization, they dont change

Conclusion

Much work to be done, please come back after you have implemented some/all of the code review answers, it was fun to read.

share|improve this answer
2  
Remember that JavaScript uses "automatic semicolon insertion", player = and { should be on the same line, otherwise it will be buggy. –  Simon André Forsberg Dec 18 '13 at 20:12
1  
Correction: When initializing variables, there does not seem to be any automatic semicolon insertion. So there's nothing wrong with this answer. –  Simon André Forsberg Dec 18 '13 at 20:38
    
I haven't tried map[place].push( JSON.parse( JSON.stringify( enemytypes[num] ) ) ); yet, but regular map[place].push(enemytypes[num]) doesn't work for some reason, the health variable will behave strange, i.e. if you kill the enemy, ALL enemies of that type are dead in the whole game.. –  none Dec 21 '13 at 9:04
    
I tried JSON.parse( JSON.stringify( and It works! Amazing! Although I don't understand how it works, it encrypts the text or something? –  none Dec 22 '13 at 11:46
    
It is the easiest way to clone an object, it changes it to text format (JSON) and then creates a new object from it. All your monsters died because every monster pointed to the same object. Objects are pointers. –  konijn Dec 22 '13 at 15:52
add comment

As for running this code locally, your browser will be able to run it locally so long as all the resources are present and all links to those resources are relative. Yours links are already relative. It runs (for me) locally in firefox using this directory structure:

cr37672/
├── 37672.html
├── css
│   └── style.css
├── images
│   └── background.bmp
└── javascript
    └── diablo2.js

If I open the file 37672.html in my browser of choice game renders and appears to function.

This can be a useful setup especially if you have an IDE that provides you a 'live view' function where changes made in the editor cause the browser to automatically refresh.

Make life easier and structure your code.

Make your life easier and write tests -- it'll inform your design.

share|improve this answer
    
As the entire game is JavaScript-based, which runs on the client side, I don't see how latency issues could ever occur where-ever it was running? –  Simon André Forsberg Dec 18 '13 at 14:58
    
@SimonAndréForsberg I was thinking that if images or other resources were pulled in then there could be a delay. On review, this is not the case here so i'll edit that out. –  Simon Minshall Dec 18 '13 at 15:01
    
Oh, I forgot there is no mouse/click function in the game, some people say getting the x and y mouse position may cause lag/performance issues in javascript.. –  none Dec 18 '13 at 16:04
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.