20
\$\begingroup\$

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:

enter image description here

\$\endgroup\$
  • 4
    \$\begingroup\$ Is it possible to link to the actual problem description on the site? \$\endgroup\$ – David Harkness Sep 17 '14 at 23:16
  • \$\begingroup\$ @DavidHarkness What exactly do you mean? \$\endgroup\$ – Ethan Bierlein Sep 17 '14 at 23:33
  • 3
    \$\begingroup\$ Can you link to the page which describe the task of generating random terrain? \$\endgroup\$ – justhalf Sep 18 '14 at 6:25
  • 1
    \$\begingroup\$ @DavidHarkness Oh, it's not a question or an actual problem to be solved on the site. You can create programs on Khanacademy here. \$\endgroup\$ – Ethan Bierlein Sep 18 '14 at 21:15
  • \$\begingroup\$ Follow-up question here \$\endgroup\$ – 200_success Oct 2 '14 at 3:02
15
\$\begingroup\$

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!

\$\endgroup\$
  • 8
    \$\begingroup\$ Regarding comparing floating point values, this typically only arises when comparing for equality (== and !=). Comparing using less/greater than (< and >) doesn't present the same problem. Even for the "or equal to" variants, adding a threshold makes no difference as it just moves the goal post (x <= 5.0 || abs(x - 5.0) <= threshold is the same as x <= 5.0 + threshold). +1 for all the rest though, and even this is a good thing to point out in general. \$\endgroup\$ – David Harkness Sep 17 '14 at 23:15
  • 3
    \$\begingroup\$ You should remove the part about comparing floats because it's just wrong. In any and all languages. \$\endgroup\$ – slebetman Sep 18 '14 at 13:00
  • \$\begingroup\$ I will edit my answer shortly. \$\endgroup\$ – bazola Sep 18 '14 at 13:22
  • 2
    \$\begingroup\$ While I agree in general with your statement on single-letter variables being pretty awful (e.g. v above), the use of x and y seem perfectly appropriate here. (As long as they continue to represent a position in cartesian space, of course.) \$\endgroup\$ – Maple Sep 18 '14 at 14:42
  • \$\begingroup\$ Have to agree with Maple here. V isn't terrible because it's common to use v for variance, but it could be more explicit. When it comes to x and y though, it seems very silly to put xCoord or yCoord just for the sake of 'not having 1 letter names. Same with int i` for a simple for loop. Could you use something more descriptive, sure, but the use of int i tells me "this is a simple for loop" - it actually conveys a lot of information that a more formal name might not. \$\endgroup\$ – corsiKa Sep 18 '14 at 16:02
16
\$\begingroup\$

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)
);
\$\endgroup\$
  • 1
    \$\begingroup\$ I think the three arguments to fill are RGB values. v determines whether it is a land tile (which should look green) or an ocean tile (which should look blue). \$\endgroup\$ – justhalf Sep 18 '14 at 6:29
  • \$\begingroup\$ Thanks @justhalf, I renamed them to be somewhat more meaningful. It's beside the point though, this is just an example of having constants that may reuse each other to define relationships, making it easier to understand what the code does and why. If I see two numbers 40, 50, their meaning and relationship is unclear, invisible, and you can and should clarify by writing as FILL_G and FILL_G + RANGE instead \$\endgroup\$ – Stop ongoing harm to Monica Sep 18 '14 at 7:33
  • 7
    \$\begingroup\$ Yes, I agree. In fact your confusion precisely shows the necessity of good variable naming =) \$\endgroup\$ – justhalf Sep 18 '14 at 7:39
5
\$\begingroup\$

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.
\$\endgroup\$

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.