I like the "with spaces" option myself. It's unfortunate that the particular example you came up with has the line wrap, or was that perhaps intentional?

In any case, the style I do not like looks like this:

if (setsockopt(fd,SOL_SOCKET,SO_REUSEADDR,(char *)&x,sizeof(x))<0)
    {
         perror("Init_socket: SO_REUSEADDR");
         close(fd);
         exit(1);
    }

ie, no spaces at all.

I would also suggest the "with spaces" style. It makes code considerably easier on the eyes, I find.

I never put spaces around a type specifier. I don't put spaces in general inside parentheses unless it is a complicated expression and the spaces help legibility. This is partly why I never use a program to blindly re-indent my source code; I use an editor to do it as I write it, and then I can tweak it if so inclined. I believe that indenting/spacing should serve the end goal of clarity, and not be an end goal in and of itself.

So in this case I prefer the lower one, but had your expressions been more complicated (e.g. involving nested expressions, or more complex operations) I might have preferred spaces.

EDIT: after rereading some things that others said, I should specify that I very strongly believe in spacing "words" – the spaces I don't believe in are ones where there is sufficient spacing already. For example, in the code

… (char*) …

the type specifier is already separated from the rest by spaces. I should also note that I tend to not put spaces in between the star and the type being pointed to, because I believe that the semantic unit is "pointer to (type)", and not "type that is modified later by pointer". So,

char* c;

is IMO preferable to:

char * c;

both of which are very strongly preferable to:

char *c;

This topic is so all over the place, I love it.

In any event, I think we could all argue about preferences forever, at the end of the day it should really matter most to those working on the project on a regular basis - so it doesn't really matter what my thoughts on the matter are, for instance.

On a side-note, though, auto-indent programs or "code beautifiers" inevitably drive me nuts because they do things like this:

if ( condition1 == condition2 ) {
   x = function();
} else {
   x = altFunction();
}

And that manner of bracing is just plain evil.

MacGregor, no it wasn't an intentional choice… but I'm happy it did because it does illustrate that the extra whitespace isn't free. Of course, this assumes we want to stick with the default 78 column width. My terminal is 137 columns wide with a font still big enough for my bad eyesight to resolve, so I certainly won't complain if we wanted 96 or even 132 as a width.

David, I agree for type casting, but unfortunately indent doesn't give the flexibility to allow arbitrary parens to have spaces added, but disallow them inside the typecast. I think it's probably the lesser of two evils to suffer with the extra spaces than to have them absorbed elsewhere.

Vassi, that's one option I can control. The settings I have put braces on their own lines, so the above would become:

if ( condition1 == condition2 )
{
    x = function();
}
else
{
    x = altFunction();
}

I want to come up with a .indent.pro file that can be included with the source, so that modifications can be reformatted to fit in with the overall style, which shouldn't be too radical a departure from the original. Nothing's perfect, but I think it's more important to keep everything looking like it belongs together, than have it exactly right.

if (setsockopt( fd, SOL_SOCKET, SO_REUSEADDR, (char *) &x, sizeof(x) ) < 0)

I think alternating the whitespaces can enhance readability.

Definitely braces on their own lines.

I'd let the user wrap the lines. It's much easier to put them in than take them out.

I like what Sandi posted, and incidentally it's basically what I do so I have zero bias. :smile:

As for braces, I tend to do this:

if ( condition ) {
    x = function();
}
else {
    x = altFunction();
}

but then this:

if ( condition )
{
    someLogic();
    someMoreLogic();
}
else
{
    otherLogic();
    moreOtherLogic();
}

the difference being that in the former the bodies have just one entry. I don't like syntax taking up 5/8 of the space, and only 3/8 being interesting. The 'else' isn't really "interesting" because I can see the change in condition from the indent. Of course, this means that I have to occasionally rearrange braces if bodies grow or shrink. When in doubt, I leave the opening brace on the same line, but always put the "else" on a new line w.r.t. the closing brace.

If you're worried about saving vertical space, get rid of the curley braces altogether in your first example.

I always use curly braces because if you leave them out, you run the risk of adding more statements, thinking that you are in the block, but you are not. What I described just above wasn't directly about vertical space; it's that I want the main information to be about the condition and statements, not syntax. When you get more than one statement, the blocks formed by the if\n{ and else\n{ pairs are additional visual cues.

Ok guys, I'm in the middle of redoing Satan's code, and want to see if this says what I think it says…..

if ( can_see_room(ch,room)
        &&   !room_is_private(room)
        &&   !IS_SET(room->room_flags, ROOM_PRIVATE)
        &&   !IS_SET(room->room_flags, ROOM_SOLITARY)
        &&   !IS_SET(room->room_flags, ROOM_SAFE)
        &&   (IS_NPC(ch) || IS_SET(ch->act,ACT_AGGRESSIVE)
        ||   !IS_SET(room->room_flags,ROOM_LAW)))
            break;

The room is an acceptable target IF:

The victim can see the room.
The room is NOT private
The room is NOT private in some other fashion than being not private (future bug fix?)
The room is NOT solitary
The room is NOT safe

You are an NPC
or You are marked as Aggressive – I assume for PC's that must mean PK???
or The room is NOT law

sooooo, NPC's and PK PC's are allowed to be teleported into law rooms, nobody else.

Sound about right?

Aggressive for pcs in stock rom means absolutely nothing.

edit: And I'm not sure that it should mean anything.

Also, the reason it's using is_room_private and also checking for the private flag is because is room private only comes back true when the flag is set AND there are already 2 people in the room. It's basically only calling that to get the other checks in is_room_private and then checking for the private flag because if wants true even if there is nobody in the room. (Same thing goes for solitary, but it's only 1 person allowed in the room.)

Why is the first check there, if the second check is true in all cases where the first is and more?

Right, but either the private room flag or the solitary flag being set is implied by room_is_private returning true, no? Thus it seems redundant… either check each flag because you don't care if anyone is there, or call the function because you do. I'd probably elect to check the flags, because rooms marked that way are sortof special anyways. It would be a way for people to avoid PK if the randomly teleported until they landed in an (empty) solitary room.

Hmmm, well, if aggressive isnt' used for PK, then that check is stupid, since if they're not an NPC, it shouldn't be set, and if they are… they already get a free pass.

*mutter*

I think I have the loop rewritten so it would always get a valid room on the first try, except for those pesky conditions. I'll post it here in case someone who knows ROM internals better than I has an idea. This is a rough draft, so feel free to criticize! :)

/* random room selection procedure */
ROOM_INDEX_DATA  *get_random_room(CHAR_DATA *ch)
{
    /* top_room is the number of rooms loaded.
     * room_index_hash[0..MAX_KEY_HASH] is the set of rooms.
     * Each room is stored in rih[vnum % MAX_KEY_HASH].
     */

    int hash_counts[MAX_KEY_HASH];
    ROOM_INDEX_DATA *pRoomIndex = NULL;
    int iHash = 0;
    int target_room = 0;
    int room_count = 0;
    int steps = 0;
    bool found = false;
    int tries = 0;

    /* First, we need to know how many rooms are in each bucket */
    for ( iHash = 0; iHash < MAX_KEY_HASH; iHash++ )
    {   
        for ( pRoomIndex  = room_index_hash[iHash];
              pRoomIndex != NULL;
              pRoomIndex  = pRoomIndex->next )
        {   
            hash_counts[iHash]++;
        }
    }

    do
    {   
        /* Ok, let's see how many tries we need */
        tries++;

        /* Next, we pick a random number */
        target_room = number_range(0, top_room);

        /* Now, we figure out which bucket that is in */
        for ( iHash = 0; iHash < MAX_KEY_HASH; iHash++ )
        {   
            room_count += hash_counts[iHash];
            if ( room_count > target_room )
                break;
        }

        steps = hash_counts[iHash] - ( room_count - target_room );

        /* Now, find the actual room */
        for ( pRoomIndex  = room_index_hash[iHash];
              pRoomIndex != NULL;
              pRoomIndex  = pRoomIndex->next )
        {
            /* Walk the list to it */
            if ( –steps > 0 )
                continue;

            /* Validate it */
            if ( can_see_room(ch,pRoomIndex)
                 && !room_is_private(pRoomIndex)
                 && !IS_SET(pRoomIndex->room_flags, ROOM_PRIVATE)
                 && !IS_SET(pRoomIndex->room_flags, ROOM_SOLITARY)
                 && !IS_SET(pRoomIndex->room_flags, ROOM_SAFE)
                 && ( !IS_SET(pRoomIndex->room_flags, ROOM_LAW)
                      || IS_NPC(ch)
                      || IS_SET(ch->act,ACT_AGGRESSIVE)
                    )
               )
            {
                found = true;
                break;
            }
            /* If it wasn't valid, maybe one of the adjacent ones is */
        }
        /* If not, we pick a new random number and try again */

    } while ( !found && tries < 10 );

    return pRoomIndex;
}

That's what I'm trying to say. The first check isn't true in all cases. The cases that it isn't true is what requires for this code to have the flag checks. PRIVATE flag only returns true in is_room_private if there are 2 or more people in it. And for the solitary flag if there is 1 or more people in it. Then there's other things in is_room_private like ownership and such. I certainly don't like the way the code is done, but if you removed one call or the other without adding in more code it wouldn't accomplish the same thing. So it's not totally redundant.

How about we just simply count the rooms that are valid for this function when they are created?
Then have an array of pointers of that size. Populate that array with a simple loop, then anytime we need to grab a random room just

return whatev_array[number_range(0, however_many_valid-1)];

But that's just off the top of my head. I'm sure there's a problem with it.

Edit: Also I'm not sure why you're dealing with the buckets to find a room when you have get_room_index() to do it for you.

And doesn't that return NULL after 10 tries if nothing was found? That doesn't seem right since it's poking around.

OLC would be the biggest problem, I suspect. I also considered rewriting it to pick a random number from 0..MAX_HASH_KEY and then a random number within that bucket…. but I don't know how even the bucket distribution is. :(

select vnum from rooms where not rooms.is_private and not rooms.is_solitary and not rooms.is_safe order by random() limit 1;

Oh well, the big issue is that there are multiple linked lists to walk… we could even go so far as to ditch the linked lists and make the room array just that, an array that gets extended by realloc(). I wonder how expensive it is to realloc() an array of 50,000 room pointers these days?

Well, my suggestion would be just regenerating the array on the stack each time the function is called. It can't be more than what you've got going there and it will find the correct room with a single random number guaranteed every time.

As far as OLC goes, I would think we are going to have to put something in place to keep the random room from being something newly created or in development anyways. It's something stock rom leaves out. But again, the count can be generated each time the function is called.

Yep, I haven't looked at OLC yet, but I assume there's some kind of prototype flag or editing flag that the rest of the code will have to honor so we don't randomly load half-edited orcs into the newbie zone, or send players to rooms that haven't been officially linked to the world proper.

Yeah, you're right. Since I have to traverse the lists to count them, doing the checks along the way isn't really much more work since they're all loaded in memory anyways.

Thanks!

Let's look at this logically.

Do we agree that:

room_is_private <=> private && some_other_condition(s)

where the details of the other conditions are not relevant at present.

Therefore, this question:

!room_is_private(room) && !IS_SET(room->room_flags, ROOM_PRIVATE)

is reduced to:

(!private || !some_other_condition(s)) && !private

Clearly, this is true if, and only if, private is false. The other conditions are irrelevant.

The private flag doesn't have anything to do with is "the room currently private?". The conditions that make the room private are separate from the private flag. The person who wrote that code did it because without asking the question "Is the private flag set?" we may end up selecting a room that is not private, but does have a private flag–so it could be private in the future.

By definition of rom code, for a room to be currently private it has to already have 2 people in it. That's because the call is used to determine if a player can enter the room. If you don't want to select a room that will ever become private, you have to check for the private flag as well.

I'm sorry, the logic used is sound–Even if it could be redesigned to be better.