No more doors or walls in middle of room #1805

Merged
merged 2 commits into from Jun 29, 2013

Projects

None yet

3 participants

@Vollch
Vollch commented Jun 28, 2013

No description provided.

@FunnyMan3595

Personally, I would be hesitant to merge this, because you've traded a long, opaque block of code for a slightly-longer, still opaque block of code. I could puzzle out what used to happen and how you've changed it, but the next person who came across this method would be just as clueless as I am now. Since you already seem to understand what's going on here, could I persuade you to reformat and comment it for the rest of us?

See 1f4459...a7df33 for an example of the kind of changes I'm talking about. The code is identical, it's just been reformatted and explained. I find that a few minutes spent doing that makes the code a whole lot easier to work with, even for myself.

@Vollch
Vollch commented Jun 28, 2013

I'm not sure what should be commented here. All used variables already commented above changed block, all major shapes commented too, and there is nothing but placing walls and doors. Yeah, it is kind of opaque, but explain something like ter_set(rn, rng(cw + 1, bw - 1), t_door_c); as "placing door at random point between central and bottom walls" looks excessive to me.

@FunnyMan3595

The main concern of good commenting is to explain the overall flow of the code. Ideally, you should be able to strip out everything but the comments and the brackets, and still be able to understand what's going on. That doesn't mean that you should explain exactly what each line does (which would be pretty pointless), but rather that you should walk the reader through the overall algorithm at each step.

Compare this:

if (bw - cw > 4) { // Too big for a bathroom, not big enough for 2nd bedrm
 house_room(this, room_bathroom, rn, bw - 4, rw, bw); 
 for (int i = rw - 1; i > rn && i > mw; i--) 
  ter_set(i, cw, t_floor);
 ter_set(rng(rw - 1, rn + 1), bw - 4, t_door_c);
 ter_set(rn, rng(cw + 1, bw - 5), t_door_c);
} else {
 house_room(this, room_bathroom, rn, cw, rw, bw); 
 if (one_in(5))
   ter_set(rng(rw - 1, rn > mw ? rn + 1 : mw + 1), cw, t_door_c);
 else 
   ter_set(rn, rng(cw + 1, bw - 1), t_door_c);
}    

With this:

if (bw - cw > 4)
{
    // Too big for a bathroom, but not big enough for a 2nd bedroom

    // Make it a bathroom anyway, but give the excess space back to
    // the room below.
    house_room(this, room_bathroom, rn, bw - 4, rw, bw);
    for (int i = rw - 1; i > rn && i > mw; i--)
    {
        ter_set(i, cw, t_floor);
    }

    // Add a door on the bottom wall.
    ter_set(rng(rw - 1, rn + 1), bw - 4, t_door_c);
    // And one on the left wall.
    ter_set(rn, rng(cw + 1, bw - 5), t_door_c);
}
else
{
    // Small enough to be a bathroom; make it one.
    house_room(this, room_bathroom, rn, cw, rw, bw);

    if (one_in(5))
    {
        // Low chance of putting the door on the top wall.
        ter_set(rng(rw - 1, rn > mw ? rn + 1 : mw + 1), cw, t_door_c);
    }
    else
    {
        // Otherwise, put the door on the left wall.
        ter_set(rn, rng(cw + 1, bw - 1), t_door_c);
    }
}

Take a look at how that is without any of the code:

{
    // Too big for a bathroom, but not big enough for a 2nd bedroom
    // Make it a bathroom anyway, but give the excess space back to
    // the room below.
    {
    }
    // Add a door on the bottom wall.
    // And one on the left wall.
}   
{
    // Small enough to be a bathroom; make it one.
    {
        // Low chance of putting the door on the top wall.
    }
    {
        // Otherwise, put the door on the left wall.
    }
}

Now I'm not certain if I've decoded it right, but If someone gave you that (in context) and asked you to fill in the code, you could. It probably wouldn't be an exact match for what's written right now, but it would preserve the essence of the code, because the comments told you what was going on and why at each step. Compare that to the existing version, likewise stripped:

{
 // Too big for a bathroom, not big enough for 2nd bedrm
} {
}

With that, all you know is that under some conditions, the space isn't suitable for either a bathroom or bedroom. You have no idea what to do in that situation, or whether the other half is supposed to be a bathroom or bedroom. And you have no idea how many doors to add, or which walls to put them on.

Again, comments exist to explain the overall flow of code. Stripping out the code itself is a good test of whether you've accomplished that. If you can still understand the process without the code itself, the comments are sufficient, and adding more would be excessive. If the comments still read like code, you've probably been too literal, and you should go back and simplify them. And if what's left is a few confusing fragments, the code isn't well-commented yet.

@FunnyMan3595 FunnyMan3595 added a commit to FunnyMan3595/Cataclysm-DDA that referenced this pull request Jun 29, 2013
@FunnyMan3595 FunnyMan3595 Merge PR #1805 112e7a7
@Vollch Vollch referenced this pull request Jun 29, 2013
Closed

Missing door inside house #1814

@kevingranade kevingranade merged commit 195350b into CleverRaven:master Jun 29, 2013
@Vollch Vollch deleted the unknown repository branch Jun 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment