Terrain Generator - KhanAcademy

Question

I'm new to JavaScript, and I've been learning off Khanacademy. I wrote the following basic terrain generator and I'm wondering if it could use any improvements. Here's the code:

var res = 7;

var area = [];

var camP = [random(500,1000), random(500,1000)];

var setArea = function()
{
    area = [];
    for (var y = -200; y < 200; y+=res)
    {
        var row = [];
        for (var x = -200; x < 200; x+=res)
        {
            row.push(noise((x+camP[0])/200, (y+camP[1])/200));
        }
        area.push(row);
        row = [];
    }
};

var drawPix = function(v, x, y){
    if (v < 0.49)
    {
        fill(random(30,40), random(100,128), random(20,30));
    } 
    else 
    {
        fill(random(0,10), random(40,50), random(245,255));
    }
    rect(x*res,y*res,res,res);
};

var drawArea = function()
{
    background(255, 255, 255);
    noStroke();
    for (var y in area)
    {
        for (var x in area[y])
        {
            drawPix(area[y][x],x,y);
        }
    }
};

setArea();
drawArea();

Here's an example of output:

Answer 1

Welcome to Code Review! There are quite a few things I can talk about here.

First off, your variable names need to be more descriptive. All of these variables should have more descriptive names:

var res = 7;
var area = [];
var camP = [random(500,1000), random(500,1000)];
area = [];
for (var y = -200; y < 200; y+=res)
for (var x = -200; x < 200; x+=res)
var drawPix = function(v, x, y){

It is preferable not to use single letter variable names in any language, because they are not helpful to understanding what the code is doing. It can be very difficult to come up with appropriate names for variables. This is one of the biggest challenges of programming, but it is an important one to do properly.

Your method names could also be more explicit. They should describe exactly what they are going to do as concisely as possible. So these method names:

var setArea = function()
var drawPix = function(v, x, y){
var drawArea = function()

Should be changed to names such as this:

var createMap = function()
var drawOnePixel = function(v, x, y){
var drawMap = function()

Those names may not be perfect, but they are closer to what would be expected.

---

Edited to reflect the comments below:

The comparison with a float that you have here is fine:

if (v < 0.49)

However, you should know that if you try to compare a float directly such as

if (v == 0.49)
if (v != 0.49)

You may run into issues. A better and more lengthy description of why this is can be found here: What's wrong with using == to compare floats?

Also please see the comment below for more information.

One idea I had to deal with this problem was to simply use integers for everything like this, and avoid using floats. I am not sure yet if this is a better practice overall, but it is something to consider. If you were doing this if (v == 49) or if (v == 490) then you would never run into this issue.

---

I like that you are creating the map in one function, and rendering the map in another. This is good practice and it is a good separation of concerns. Keep it up!

Answer 2

It's nicely done! One thing I'd pick on though is that your code is full of magic numbers:

var camP = [random(500,1000), random(500,1000)];
// ...
for (var y = -200; y < 200; y+=res)
// ...
for (var x = -200; x < 200; x+=res)
// ...
row.push(noise((x+camP[0])/200, (y+camP[1])/200));
// ...
if (v < 0.49)
// ...
fill(random(30,40), random(100,128), random(20,30));
// ...
fill(random(0,10), random(40,50), random(245,255));

Put these numbers into variables, defined at the top. By doing so you will effectively give them names, which could explain what they are.

This is especially recommended for values that you use repeatedly, for example 200.

And when a value depends on another (derived from another), that relationship can be expressed in the definition. For example, the difference in the parameter pairs in random(0,10), random(40,50), random(245,255) is suspiciously 10, probably for a reason, which would become clear in the variable definition. I've no idea how fill works, so these names are probably completely inappropriate, but to illustrate what I mean:

var FILL_RGB_RANGE = 10;
var FILL_R = 0;
var FILL_G = 40;
var FILL_B = 245;

fill(
  random(FILL_R, FILL_R + FILL_RGB_RANGE),
  random(FILL_G, FILL_G + FILL_RGB_RANGE),
  random(FILL_B, FILL_B + FILL_RGB_RANGE)
);

Answer 3

Couple of quick things I noticed:

• You've declared area[] twice, once as a global and once inside of drawPix().
• for( ... in ...) is for stepping through objects than arrays, take a look at forEach()
• Bit more commenting would be nice, although this could be negated by the better property naming and use of constants as mentioned above.
• Try to be consistent in your use of parenthesis in function declarations in whether you put them on the same line as the function or not (I won't dare state a preference, Sorry Doug).
• Run it through jshint.com is always instructive.