Subject: And now for something different, technical concrit
Author:
Posted on: 2016-07-13 21:41:00 UTC
Since you did sort of dare me to look at your code, and I was a TA for a computer architecture class last semester, I felt the urge to give you concrit on your thing.
- Your JS
1) Powers That Be in a bucket, indent your code! (Unless you were deliberately going for the unreadable look, of course). If I didn't know better, I'd accuse you of coding in Notepad. (Note: if you are actually coding in Notepad, I've heard good things about Notepad++ on Windows, or you can pick up Emacs (or maybe Vim, if you like second-best) if you want to jump in the deep end of coding editors, but Google around for a good config in those cases)
2) Several variable should probably be dictionaries/Objects, not arrays. The main reason for this is that arrays use memory proportional to the largest valid index (that is, if you do var a = []; a[200000] = 5, you'll also use memory space for all of the undefineds that go in slots 0 through 199999.
2b) Which brings me to my next point, indexOf(), which is, as the theorists say, O(n). That is, the more Pokemon you add, the slower it will get, especially for errors. This can be avoided by dropping pokindex entirely and making pokedex an Object (var pokedex = {}).
This will allow you to check membership by simply doing numMon in pokedex. Also, you get the Magic that is JavaScript auto-coersion (most such things are terrible ideas but it's presence is not your fault). That is, if you do foo = {1: 2}, then foo["1"] == foo[1].
3) If you ever find yourself copy-pasting the same code several times and making minor changes (especially if there are variable suffixes such as thingA, thingB, ... involved), you might want to find another way to express what you're going for.
For example, here's a very functional-programming way to go from uppercase txt to digits
var digits = txt.split("").map(function (char) { return char.charCodeAt(0).toString(); }).join("")
(plop the .toUpperCase() in there for extra golf)
3) atkset{1-4} should really be an array-of-arrays called atkSet for DRY (don't repeat yourself) reasons
4) "".concat(foo[0],foo[1]...) is foo.join("") So, I might have written var trainOut = trainlets.join("") + document.getElementById("namecode").value)
5) + works on strings, and might be advisable when there's only two
6) Verifier() is O(n) (and uppercase, which is usually reserved for constructors like Monster). To make it faster, you might want this
function setLike()
{
var ret = {};
var numArguments = arguments.length;
for (var i = 0; i < numArguments; i++) {
ret[arguments[i]] = true
}
return ret;
}
and to pull allowed out into something like ALLOWED_FROBS to avoid re-defining it every time.
7) One-based indexing makes people cry. Are you a mathematician?
8) Similarly, using n as the index for a loop is just a bit odd, since n is often the length of the thing you're looping over, so foo[n-1] feels like assignment to the last element of the array. The usual loop variables are i, then j, (and k if you're doing matrix multiply, or other crazy things)
9) On the positive side, the code is split into functions, is understandable, and uses clear variable names (!).
10) Do you wont to get modern? Do you want to add an optional feature that works for everyone other than people on old internet explorer versions? Try localStorage. It's the hot net thing, and might actually be useful? You could stick the save code in there and re-populate the page on re-load.
Your HTML/CSS:
1) Putting a style on each thing, especially if you're copy-pasting them around, is, to put it strongly, Doing It Wrong.
You can assign classes to HTML things, and then style everything that shares a class at once.
For example
<style type="text/css">
.blockimg {
width:50px;
height:50px;
position:absolute;
left: 200px;
top: 95px;
}
</style>
Then, you can put class="blockimg on all the image placeholders instead of copy-pasting. This reduces redundancy and makes it easier for you to change your mind.
2) Absolute widths in pixels make me personally sad. My eyes are perma-not-great, so I have instructed my browser to bump fonts to at least 16pt no matter what the developer wants. This means that I can't read the whole "Go(ne)" button. The exact remedy for this depends on what you're doing.
2a) em is a unit that is relative to the current font (and font size).
2b) If you're willing to rewrite everything, absolute positioning is vaguely considered not a great idea (mainly because cell phones). Many people use some variant of a grid layout (have not used their code, but looks not-bad). If you want, leaving extra padding columns on the right (empty divs) will probably keep the current look.
But, the game is pretty cool, and it's pretty good for something made in three days. (I've seen worse.)
- Tomash, who didn't plan on this getting so long, and hopes he's not being too harsh
