3
\$\begingroup\$

I would like to know if this code of the database for my Pokemon clone game follows general practices in syntax and general readability. I feel like I could cut down on my comment usage, and place comments in other places. All variables here are static, and as far as I've read static variables must be Uppercase. As for the constant variables, they should be ALL_UPPERCASE. I don't follow neither conventions, as you will see. I'm using PlayCanvas. Any feedback is appreciated.

var Database = pc.createScript('database');

// this is where all the static objects are stored

canInteract = true;
isPlayerIdle = true;
isSpeechFinished = true;
speechLine = 0; // used for animations in the middle of a speech
curRivalBattle = 0;
currentTalkingNPC = null;
enemyScript = null; // used by the battle script trio
dirArray = [new pc.Vec2(0,-1), new pc.Vec2(0,1), new pc.Vec2(-1,0), new pc.Vec2(1,0)];
oppDirArray = [1, 0, 3, 2]; // used by NPCs to face the player when talking

// saved variables
time = 0;
money = 5000;
worldMenuState = 0; // used to show the POKEDEX and POKEMON buttons in the world menu
rivalStarter = 0;
playerName = "~";
rivalName = "RIVAL";
fColor = new pc.Color(0, 0.8, 1, 1);
sColor = new pc.Color(0, 1, 1, 1);
lastPlaces = [0, 4];
lastPlaceTile = new pc.Vec2(-3.2, 4);

// specific variables (all are saved)
rivalBattleChecks = [false]; // this array keeps track of all rival's battles (as they should only be done once)
hasOldManDemo = false; // has the grandpa showed a demo at least once?

// option variables (all are saved)
optionVars = [0, 0, 0, 0, 0];

// controls
dirKeys = [pc.KEY_DOWN, pc.KEY_UP, pc.KEY_LEFT, pc.KEY_RIGHT];
buttonA = pc.KEY_A;
buttonB = pc.KEY_S;
buttonSt = pc.KEY_ENTER;

Database.prototype.initialize = function() {
    // saved variables for easiear access of the following entities and scripts
    database = this;

    root = this.app.root;
    HUD = root.findByName('HUD');
    menuList = this.entity.script.menuList;
    world = root.findByName('World');
    pkmnFunctions = HUD.script.pkmnFunctions;
    player = root.findByName('Player').script.player;
    debugText = HUD.findByName('Debug Text');
    speechBox = HUD.findByName('Speech Box').script.speechBox;
    battleIntro = HUD.findByName('Battle Scene').script.battleIntro;
    trainerSprites = this.app.assets.find('Trainer frames').resource;
    backPkmnSprites = this.app.assets.find('Pokemon (back) frames').resource;
    frontPkmnSprites = this.app.assets.find('Pokemon (front) frames').resource;
    this.menu = HUD.findByName('World Menu');
};

Database.prototype.update = function(dt) {
    kb = this.app.keyboard; // shortens all input code, for readability
    time += dt;
    if(kb.wasPressed(buttonSt) && isPlayerIdle) {
        if(player.enabled) {
            HUD.findByName('World Menu').script.worldMenu.changeState(true);
        }
        else if(HUD.findByName('World Menu').enabled) {
            HUD.findByName('World Menu').script.worldMenu.changeState(false);
        }
    }
};

// item ID, item amount
inventory = [[2, 1]];

pStatXp = [[0, 0, 0, 0], [0, 0, 0, 0],  [0, 0, 0, 0],  [0, 0, 0, 0], [0, 0, 0, 0], [0, 0, 0, 0]];
statusNames = ['OK', 'PSN', ''];
typeNames = ['BUG', 'DRAGON', 'ELECTRIC', 'FIGHTING', 'FIRE', 'FLYING', 'GHOST', 'GRASS', 'GROUND', 'ICE', 'NORMAL', 'POISON', 'PSYCHIC', 'ROCK', 'WATER'];

// player pkmn's stats during a battle (for stat moves)
pBattleStats = [[0,0,0,0], [0,0,0,0], [0,0,0,0], [0,0,0,0], [0,0,0,0], [0,0,0,0]];

// ID, level, currentHP, maxHP, status, attack, defense, speed, special
ePkmn = [[-1, 0, 0, 0, 0, 0, 0, 0, 0], 
         [-1, 0, 0, 0, 0, 0, 0, 0, 0], 
         [-1, 0, 0, 0, 0, 0, 0, 0, 0], 
         [-1, 0, 0, 0, 0, 0, 0, 0, 0],
         [-1, 0, 0, 0, 0, 0, 0, 0, 0],
         [-1, 0, 0, 0, 0, 0, 0, 0, 0]];

// pkmnID, name, level, xpPoints, currentHP, maxHP, status, attack, defense, speed, special, isTraded
pPkmn = [[-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false],
         [-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false],
         [-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false],
         [-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false],
         [-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false],
         [-1, "??????????", 0, 0, 0, 0, 0, 0, 0, 0, 0, false]];

// move ID, curernt PP (haha)
pMoves = [
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]], 
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]], 
         ];

// same as pMoves
eMoves = [
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]], 
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]],
          [[-1, -1], [-1, -1], [-1, -1], [-1, -1]], 
         ];

// all arrays below will be added with more of their respective data throughout the development of the game
itemNames = ['OAK\'S PARCEL', 'POTION', 'SUPER POTION', 'HYPER POTION', 'MAX POTION'];

// (xy: position, zw: destination), next direction, place, variation ID (if needed)
teleTiles = [[new pc.Vec4(-3.2, 4, -15.2, 1.6),    -1, 1], // Pallet Town to Mom House 0 
             [new pc.Vec4(-11.2, 6.4, -18.4, 6.4),  0, 2], // Mom House 0 to Mom House 1
             [new pc.Vec4(-18.4, 6.4, -11.2, 6.4),  2, 1], // Mom House 1 to Mom House 0
             [new pc.Vec4(3.2, 4, -24, -5.6),      -1, 3, 0], // Pallet Town to Generic House #0 (Rival's Sister)
             [new pc.Vec4(2.4, -0.8, -15.2, -8.8), -1, 4], // Pallet Town to Oak's Lab
             [new pc.Vec4(8, 50.4, 20, 40.8),      -1, 7, 0], // Viridian City to Poke-Mart #0
             [new pc.Vec4(3.2, 45.6, 20, 48),      -1, 8, 0], // Viridian City to Poke-Center #0
             [new pc.Vec4(1.6, 53.6, -24, -5.6),   -1, 3, 1]];

// colors actually range 0-1, but for ease of use the max value here is 255
// entrance type: 0 - two tiles horizontal, 1 - two tiles vertical
// xy: tile pos, 1st Color, 2nd Color, entrance type, places
placeTiles = [[new pc.Vec2(0.8, 8.8), new pc.Color(0, 200, 255, 1),  new pc.Color(0,255,255,1),    0, 0, 5], // Route 1 to Pallet Town
              [new pc.Vec2(0.8, 9.6), new pc.Color(127, 247, 31, 1), new pc.Color(87,183,247,1),   0, 5, 6, 0], // Pallet Town to Route 1
              [new pc.Vec2(0.8, 37.6), new pc.Color(157, 255, 61, 1), new pc.Color(137,233,255,1), 0, 5, 6], // Route 1 to Viridian City
              [new pc.Vec2(0.8, 36.8), new pc.Color(127, 247, 31, 1), new pc.Color(87,183,247,1),  0, 5, 6,  0]]; // Viridian City to Route 1 


// (xy: min X and Y, zw: max X and Y), min level, max level, encounter chance (by 100%), encounter ratio (by pokedex ID)
wildTiles = [[new pc.Vec4(0.4, 8, 1.6, 11.2), 4, 7, 25, [18, 15]], // Pallet Town - Route 1
             [new pc.Vec4(-4, 12, -1.6, 12.8),  4, 7, 25, [18, 15]], // Route 1 
             [new pc.Vec4(-2.4, 13.6, 0, 14.4),  4, 7, 25, [18, 15]], // Route 1 
             [new pc.Vec4(2.4, 12, 4.8, 12.8),  4, 7, 25, [18, 15]], // Route 1 
             [new pc.Vec4(4, 13.6, 6.4, 14.4), 4, 7, 25, [18, 15]], // Route 1 
             [new pc.Vec4(2.4, 16.8, 4.8, 19.2),  4, 7, 25, [18, 15]]]; // Route 1 

// attack types: status 0, physical 1, special 2
// name, type, attack type, power, accuracy, max PP, affected stat, stat change, status change
moves = [['TACKLE',     10, 1, 40, 100, 35],
         ['GROWL',      10, 0, 0,  100, 40, 0, -1, -1],
         ['SCRATCH',    10, 1, 40, 100, 35],
         ['TAIL WHIP',  10, 0, 0,  100, 30, 1, -1, -1],
         ['GUST',       5,  2, 40, 100, 35],
         ['LEECH SEED', 7,  0, 0,  90,  10]];

// moveID, required level
lvlPkmnMoves = [
                [[0,1], [1,1], [5, 7]], // 0
                [[]], // 1
                [[]], // 2
                [[1, 1], [2, 1]], // 3
                [[]], // 4
                [[]], // 5
                [[0, 1], [3, 1]], // 6
                [[]], // 7
                [[]], // 8
                [[]], // 9
                [[]], // 10
                [[]], // 11
                [[]], // 12
                [[]], // 13 
                [[]], // 14
                [[4, 1]], // 15
                [[]], // 16
                [[]], // 17
                [[0, 1], [3, 1]], // 18
               ];


// Bug 0, Dragon 1, Electric 2, Fighting 3, Fire 4, Flying 5, Ghost 6, Grass 7, Ground 8, Ice 9, Normal 10, Poison 11, Psychic 12, Rock 13, Water 14
// name, health, attack, defense, speed, special, type 1, type 2, yield group, base yield, iconID
pkmn = [["BULBASAUR", 45, 49, 49, 45, 65,  7, 11,  2, 64, 1], // 0
        ["IVYSAUR", 60, 62, 63, 60, 80,    7, 11,  2, 142], // 1
        ["VENUSAUR", 80, 82, 83, 80, 10,   7, 11,  2, 263], // 2
        ["CHARMANDER", 39, 52, 43, 65, 50, 4, -1,  2, 62], // 3
        ["CHARMELEON", 58, 64, 58, 80, 65, 4, -1,  2, 142], // 4
        ["CHARIZARD", 78, 84, 78, 100, 85, 4, 5,   2, 267], // 5
        ["SQUIRTLE", 44, 48, 65, 43, 50,   14, -1, 2, 63], // 6
        ["WARTORTLE", 59, 63, 80, 58, 65,  14, -1, 2, 142], // 7
        ["BLASTOISE", 79, 83, 100, 78, 85, 14, -1, 2, 265], // 8
        ["CATERPIE", 45, 30, 35, 45, 20,   0, -1,  1, 39], // 9
        ["METAPOD", 50, 20, 55, 30, 25,    0, -1,  1, 72], // 10
        ["BUTTERFREE", 60, 45, 50, 70, 80, 0, 5,   1, 198], // 11
        ["WEEDLE", 40, 35, 30, 50, 20,     0, 11,  1, ], // 12
        ["KAKUNA", 45, 25, 50, 35, 25,     0, 11,  1, ], // 13
        ["BEEDRILL", 65, 80, 40, 75, 45,   0, 11,  1], // 14
        ["PIDGEY", 40, 45, 40, 56, 35,     10, 3,  1, 55], // 15
        ["PIDGEOTTO", 63, 60, 55, 71, 50,  10, 3], // 16
        ["PIDGEOT", 83, 80, 75, 91, 70,    10, 3], // 17
        ["RATTATA", 30, 56, 35, 72, 25,    10, -1, 1, 57], // 18
        ["RATICATE", 55, 81, 60, 97, 50,   10, -1], // 19
        ["SPEAROW", 40, 60, 30, 70, 31],
        ["FEAROW", 65, 90, 65, 100, 61],
        ["EKANS", 35, 60, 44, 55, 40],
        ["ARBOK", 60, 85, 69, 80, 65],
        ["PIKACHU", 35, 55, 30, 90, 50],
        ["RAICHU", 60, 90, 55, 100, 90],
        ["SANDSHREW", 50, 75, 85, 40, 30],
        ["SANDSLASH", 75, 100, 110, 65, 55],
        ["NIDORAN#", 55, 47, 52, 41, 40],
        ["NIDORINA", 70, 62, 67, 56, 55],
        ["NIDOQUEEN", 90, 82, 87, 76, 75],
        ["NIRDORAN@", 46, 57, 40, 50, 40],
        ["NIDORINO", 61, 72, 57, 65, 55],
        ["NIDOKING", 81, 92, 77, 85, 75],
        ["CLEFAIRY", 70, 45, 48, 35, 60],
        ["CLEFABLE", 95, 70, 73, 60, 85],
        ["VUPLIX", 38, 41, 40, 65, 65],
        ["NINETALES", 73, 76, 75, 100, 100],
        ["JIGGLYPUFF", 115, 45, 20, 20, 25],
        ["WIGGLYTUFF"],
        ["ZUBAT"],
        ["GOLBAT"],
        ["ODDISH"],
        ["GLOOM"],
        ["VILEPLUME"],
        ["PARAS"],
        ["PARASECT"],
        ["VENONAT"],
        ["VENOMOTH"],
        ["DIGLETT"],
        ["DUGTRIO"],
        ["MEOWTH"],
        ["PERSIAN"],
        ["PSYDUCK"],
        ["GOLDUCK"],
        ["MANKEY"],
        ["PRIMEAPE"],
        ["GROWLITHE"],
        ["ARCANINE"],
        ["POLIWAG"],
        ["POLIWHIRL"],
        ["POLIWRATH"],
        ["ABRA"],
        ["KADABRA"],
        ["ALAKAZAM"],
        ["MACHOP"],
        ["MACHOKE"],
        ["MACHAMP"],
        ["BELLSPROUT"],
        ["WEEPINBELL"],
        ["VICTREEBELL"],
        ["TENTACOOL"],
        ["TENTACRUEL"],
        ["GEODUDE"],
        ["GRAVELER"],
        ["GOLEM"],
        ["PONYTA"],
        ["RAPIDASH"],
        ["SLOWPOKE"],
        ["SLOWBRO"],
        ["MAGNEMITE"],
        ["MAGNETON"],
        ["FARFETCH\'D"],
        ["DODUO"],
        ["DODRIO"],
        ["SEEL"],
        ["DEWGONG"],
        ["GRIMER"],
        ["MUK"],
        ["SHELDER"],
        ["CLOYSTER"],
        ["GASTLY"],
        ["HAUNTER"],
        ["GENGAR"],
        ["ONIX"],
        ["DROWZEE"],
        ["HYPNO"],
        ["KRABBY"],
        ["KINGLER"],
        ["VOLTORB"],
        ["ELECTRODE"],
        ["EXEGGCUTE"],
        ["EXEGGUTOR"],
        ["CUBONE"],
        ["MAROWAK"],
        ["HITMONLEE"],
        ["HITMONCHAN"],
        ["LIKITUNG"],
        ["KOFFING"],
        ["WEEZING"],
        ["RHYHORN"],
        ["RHYDON"],
        ["CHANSEY"],
        ["TANGELA"],
        ["KANGASKHAN"],
        ["HORSEA"],
        ["SEADRA"],
        ["GOLDEEN"],
        ["SEAKING"],
        ["STARYU"],
        ["STARMIE"],
        ["MR. MIME"],
        ["SCYTHER"],
        ["JYNX"],
        ["ELECTABUZZ"],
        ["MAGMAR"],
        ["PINSIR"],
        ["TAUROS"],
        ["MAGIKARP"],
        ["GYARADOS"],
        ["LAPRAS"],
        ["DITTO"],
        ["EEVEE"],
        ["VAPOREON"],
        ["JOLTEN"],
        ["FLAREON"],
        ["PORYGON"],
        ["OMANYTE"],
        ["OMASTAR"],
        ["KABUTO"],
        ["KABUTOPS"],
        ["AERODACTYL"],
        ["SNORLAX"],
        ["ARTICUNO"],
        ["ZAPDOS"],
        ["MOLTRES"],
        ["DRATINI"],
        ["DRAGONAIR"],
        ["DRAGONITE"],
        ["MEWTWO"],
        ["MEW"]];

regPkmn = [false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false, false];

boxedPkmn = [[], [], [], [], [], [], [], [], [], []];
boxedMoves = [[], [], [], [], [], [], [], [], [], []];
boxedStatXp = [[], [], [], [], [], [], [], [], [], []];
\$\endgroup\$
3
\$\begingroup\$

You are not declaring any of your variables. This will

  • throw errors, if the script is running in strict mode (which it should be - best to always use strict mode, it can turn hard-to-identify bugs into explicit errors that can be fixed)
  • Or, if you aren't running in strict mode, every undeclared variable will become implicitly global, as well as putting a property on the global object.

Always declare variables. In modern Javascript, declare them with const whenever possible, and let when you must reassign them. For example:

const oppDirArray = [1, 0, 3, 2]; // this will never be reassigned
// ...
let money = true; // this may be reassigned later

Most of your variables are on the top level. Even if they were declared properly, this is a potential code smell - variable scope should be as narrow as reasonable, usually. For example, your

speechLine = 0; // used for animations in the middle of a speech

sounds like it would probably be better if it were scoped only to a part of the code that handles animations or speech. The same thing can be said for most of your variables.

If you don't constrain the scope of your variables, figuring out what all a particular function has access to, should be able to access, and should be able to change given its responsibilities can become a messy headache.

If you're going to declare a variable, make sure to use it later. For example, you do debugText = HUD.findByName('Debug Text');, but then never reference debugText again. If it's really not being used anywhere, might as well just delete it. (If you do keep it, the debugText variable should only be used within its initialize function - if another part of the code needs to be able to see it, call another function within initialize to pass it around, instead of reassigning a global variable.) Same thing for most of the other variables in initialize.

Consider using a linter like eslint to automatically prompt you to correct many of these potential mistakes.

In Database.prototype.update, instead of repeating HUD.findByName('World Menu').script.worldMenu.changeState multiple times, you can define a function that calls it with the desired argument, and you can save HUD.findByName('World Menu') in another variable:

if (kb.wasPressed(buttonSt) && isPlayerIdle) {
  const worldMenu = HUD.findByName('World Menu');
  const changeState = arg => worldMenu.script.worldMenu.changeState(arg);
  if(player.enabled) {
    changeState(true);
  } else if(worldMenu.enabled) {
    changeState(false);
  }
}

Commenting on the point of a variable is fine in general, though usually it'd be preferable for the point of a variable to be clearly indicated by a combination of its scope (hopefully narrow, discussed above) and its name. Don't be afraid to use descriptive names; being able to understand the code at a glance is more important than being concise. If you can't figure out a way to constrain the scope and create a name such that the meaning of the variable is obvious, a comment is not only fine, but it's probably preferable over the alternative. (But, that situation should be uncommon - usually you should be able to precisely name a variable or constrain its scope enough such that its meaning is obvious without a comment)

For example, rather than

hasOldManDemo = false; // has the grandpa showed a demo at least once?

maybe use

let oldManHasShownPokeballDemo = false;

Note that using grammar like the above results in logical checks looking extremely readable, eg:

if (oldManHasShownPokeballDemo) {
  // Then the old man has shown the pokeball demo
}

Ideally, for organizational purposes such a variable would exist in an object indicating the map state that can be looked up when required, rather than being global.

I wouldn't worry much about naming conventions, given that you're already using camelCase, which is pretty common even for completely static variables. Better to get the fundamentals of script organization down than to worry about more opinionated subjects.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, that was a lot quicker than I expected. However, I feel like long variables like 'oldManHasShownPokeballDemo' may be a little too long, which is why I decided to describe it with a comment. Are these types of names really acceptable, or should I only use them a few times? I feel like accepting these long names as a standard would clutter code. \$\endgroup\$ – Osman Mammedov Apr 27 at 2:24
  • \$\begingroup\$ If you make it shorter, it will be less obvious what it refers to, and you may need to add a comment or go to the place where it's defined to read the comment describing what the variable is. I'd make it long enough such that you'd expect someone reading the code to be able to understand what it's representing upon reading the variable name. If you define it in a narrow scope, it shouldn't have to be long - eg, in an OldMan module or object, you could define it as hasShownDemo. That would be a better choice than having a long variable name on the top level. \$\endgroup\$ – CertainPerformance Apr 27 at 6:57

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.