29 May, 2009, boblinski wrote in the 22nd comment:
Thanks for the fix-ups!
Here's my direction_to_xwards()..
act_info.c:
merc.h:
How does that one work?
Here's my direction_to_xwards()..
act_info.c:
char *direction_to_xwards (int direction)
{
static char buf[12];
if (direction == 0)
sprintf (buf, "Northwards: ");
else if (direction == 1)
sprintf (buf, "Eastwards: ");
else if (direction == 2)
sprintf (buf, "Southwards: ");
else if (direction == 3)
sprintf (buf, "Westwards: ");
else if (direction == 4)
sprintf (buf, "Upwards: ");
else
sprintf (buf, "Downwards: ");
return buf;
}
merc.h:
char * direction_to_xwards args( ( int direction ));
How does that one work?
29 May, 2009, David Haley wrote in the 23rd comment:
Quote
if (!str_cmp (arg1, "n") || !str_cmp (arg1, "north"))
Another thing you can do is use the str_prefix function, which tests if one string is a prefix of the other. This would let it work with n, no, nor, nort, and north.
Quote
static char buf[12];
There's a small gotcha here. In C, a string is a sequence of characters ending with a "NUL" character (numeric value zero). Therefore, if a string has 12 visible characters, it must have room for the final zero byte. For example, the string "hello" is actually represented in memory like so:
byte 0 | 1 | 2 | 3 | 4 | 5
h | e | l | l | o | \0
Because your strings have 12 visible characters, you need buf to have 13 bytes. Actually, just for good measure, I'd give it something like 64 or even 128 so that you have room to grow in case you later decide to write "To the north, you see" or something like that.
Just for completeness's sake, a "string" in C is a pointer to the first character in the string (i.e. a pointer of type char*), and continuing until the zero byte. You can have char* pointers that don't actually define strings, which makes sense in some contexts, but for now, think of strings as starting at some byte and continuing until the zero.
29 May, 2009, Kline wrote in the 24th comment:
Just a minor note as to style preference, you can condense all your if … else blocks to a simple switch statement.
The default case lets you actually note that you received an invalid value and handle it somehow. With your current function anything that is not 0-4 will be treated as "Downwards" irregardless with that final else.
char *direction_to_xwards( int direction )
{
static char buf[128];
switch( direction )
{
default: sprintf(buf,"Error, received invalid value: %d.",direction); break;
case 0: sprintf(buf,"Northwards: "); break;
case 1: sprintf(buf,"Eastwards: "); break;
case 2: sprintf(buf,"Southwards: "); break;
case 3: sprintf(buf,"Westwards: "); break;
case 4: sprintf(buf,"Upwards: "); break;
case 5: sprintf(buf,"Downwards: "); break;
}
return buf;
}
The default case lets you actually note that you received an invalid value and handle it somehow. With your current function anything that is not 0-4 will be treated as "Downwards" irregardless with that final else.
29 May, 2009, David Haley wrote in the 25th comment:
You probably don't want to be printing an error message to the string; calling the 'bug' function might be better. It shouldn't be possible to have an invalid 'direction', so it's an indication of a serious problem somewhere.
Another thing to note is that if you have a function that only returns string literals, you can dispense with the static char buffer, and just return a const char*:
BTW, "irregardless" doesn't mean what you think it means. :tongue:
Another thing to note is that if you have a function that only returns string literals, you can dispense with the static char buffer, and just return a const char*:
const char* foo(int input)
{
if (input < 5) return "hello";
else return "goodbye";
}
BTW, "irregardless" doesn't mean what you think it means. :tongue:
29 May, 2009, boblinski wrote in the 26th comment:
Okay, how about these:
str_to_direction:
direction_to_xwards:
I feel like I may not be writting the direction_to_xwards part right (in particular the use of "const char*")…
str_to_direction:
/* merh.h */
int str_to_direction args( ( const char *arg1) );
/* act_info.c */
int str_to_direction(const char *arg1)
{
if (!str_prefix (arg1, "north"))
return 0;
if (!str_prefix (arg1, "east"))
return 1;
if (!str_prefix (arg1, "south"))
return 2;
if (!str_prefix (arg1, "west"))
return 3;
if (!str_prefix (arg1, "up"))
return 4;
if (!str_prefix (arg1, "down"))
return 5;
else
return -1
}
direction_to_xwards:
/* merh.h */
const char* direction_to_xwards args( ( int direction ));
/* act_info.c */
const char* direction_to_xwards (int direction)
{
if (direction == 0) return "Northwards: ";
else if (direction == 1) return "Eastwards: ";
else if (direction == 2) return "Southwards: ";
else if (direction == 3) return "Westwards: ";
else if (direction == 4) return "Upwards: ";
else if (direction == 5) return "Downwards: ");
else
bug ("direction_to_xwards direction not found.", 0);
return;
}
I feel like I may not be writting the direction_to_xwards part right (in particular the use of "const char*")…
29 May, 2009, David Haley wrote in the 27th comment:
I don't remember if the order for str_prefix is prefix_string, full_string or full_string, prefix_string, but that looks reasonable. In any case you'll quickly find out when you use the command if it takes prefixes or not, and can change the order if necessary.
direction_to_xwards is correct except:
- line 9 has an extra parenthesis before the semicolon
- line 13 should return the empty string, or "ERR", or something like that.
Note that the return on line 13 will be executed whenever line 11 is executed, so it might make sense to put them next to each other indent-wise.
Remember our discussion about constants? This is actually a good time to use them. You would do something like this:
This would go into your .h file. In fact, something like it might exist already.
The advantage of using constants here is that:
- it helps prevent typos – you might put in 6 instead of 5, and the compiler will be happy, but you can't put in anything but a correct constant name
- it makes the functions insensitive to the order of directions (it's unlikely they'll change, but you never know)
- it's slightly reassuring to see comparisons against known, constant values than against numbers that might seem arbitrary
- it makes it easier to search for occurrences of the direction. "1" might appear all over the code for unrelated reasons, whereas "DIR_EAST" will only appear when it's actually referring to "east".
direction_to_xwards is correct except:
- line 9 has an extra parenthesis before the semicolon
- line 13 should return the empty string, or "ERR", or something like that.
Note that the return on line 13 will be executed whenever line 11 is executed, so it might make sense to put them next to each other indent-wise.
Remember our discussion about constants? This is actually a good time to use them. You would do something like this:
#define DIR_NORTH 0
#define DIR_EAST 1
#define DIR_SOUTH 2
…
This would go into your .h file. In fact, something like it might exist already.
The advantage of using constants here is that:
- it helps prevent typos – you might put in 6 instead of 5, and the compiler will be happy, but you can't put in anything but a correct constant name
- it makes the functions insensitive to the order of directions (it's unlikely they'll change, but you never know)
- it's slightly reassuring to see comparisons against known, constant values than against numbers that might seem arbitrary
- it makes it easier to search for occurrences of the direction. "1" might appear all over the code for unrelated reasons, whereas "DIR_EAST" will only appear when it's actually referring to "east".
29 May, 2009, Davion wrote in the 28th comment:
Those already exist in ROM. Make sure to grep for them before you add them.
29 May, 2009, Sharmair wrote in the 29th comment:
As far as your 2nd function, you have to return a const char*, so that last line should be something
like:
Or even:
Depending on how you want to handle an error return. The point is, all your return paths have to
return the declared type(and it is not void like just return by itself is).
As far as the first function, why don,t you use the dir_name[] you have already shown in your code
you have and just do something like:
For that matter, I would probably either use an array instead of the 2nd function, or build the string
using the dir_name[]. I would also probably put the direction specific things in act_move.c. One last
thing, though it does not hurt if you use it, I use real C/C++ prototypes instead of the Diku macro
args to support obsolete compilers that you don't see anymore, and probably would not compile the
current code if you did find one. Another reason to ditch args is if you make your code C++ ready,
they are truly obsolete as C++ requires a real prototype. So, for my function above:
You actually might not need the txt (you don't in C++) in the declaration, but it does not hurt.
like:
return "";
Or even:
return NULL;
Depending on how you want to handle an error return. The point is, all your return paths have to
return the declared type(and it is not void like just return by itself is).
As far as the first function, why don,t you use the dir_name[] you have already shown in your code
you have and just do something like:
#define NUM_DIR 6 /* this should be in merc.h and really be used instead of 6 in all the code */
int get_dir(const char* txt){
int ct;
for(ct = 0; ct < NUM_DIR; ++ct)
if(!str_prefix(txt, dir_name[ct])
return ct;
return -1;
}
For that matter, I would probably either use an array instead of the 2nd function, or build the string
using the dir_name[]. I would also probably put the direction specific things in act_move.c. One last
thing, though it does not hurt if you use it, I use real C/C++ prototypes instead of the Diku macro
args to support obsolete compilers that you don't see anymore, and probably would not compile the
current code if you did find one. Another reason to ditch args is if you make your code C++ ready,
they are truly obsolete as C++ requires a real prototype. So, for my function above:
int get_dir(const char* txt);
You actually might not need the txt (you don't in C++) in the declaration, but it does not hurt.
29 May, 2009, boblinski wrote in the 30th comment:
Here's my lastest update:
str_to_direction
direction_to_xwards:
Looking okay now?
str_to_direction
/* merh.h */
int str_to_direction args( ( const char *arg1) );
/* act_info.c */
int str_to_direction(const char *arg1)
{
if (!str_prefix (arg1, "north")) return DIR_NORTH;
if (!str_prefix (arg1, "east")) return DIR_EAST;
if (!str_prefix (arg1, "south")) return DIR_SOUTH;
if (!str_prefix (arg1, "west")) return DIR_WEST;
if (!str_prefix (arg1, "up")) return DIR_UP;
if (!str_prefix (arg1, "down")) return DIR_DOWN;
else
return -1;
}
direction_to_xwards:
/* merh.h */
const char* direction_to_xwards args( ( int direction ) );
/* act_info.c */
const char* direction_to_xwards (int direction)
{
if (direction == DIR_NORTH) return "Northwards: ";
else if (direction == DIR_EAST) return "Eastwards: ";
else if (direction == DIR_SOUTH) return "Southwards: ";
else if (direction == DIR_WEST) return "Westwards: ";
else if (direction == DIR_UP) return "Upwards: ";
else if (direction == DIR_DOWN) return "Downwards: ";
else
bug ("direction_to_xwards direction not found.", 0);
return "ERROR";
}
Looking okay now?
29 May, 2009, boblinski wrote in the 31st comment:
Well, I didn't see Sharmair's post until after my last one.. so here's a question…
What is best out of:
—–OR—–
What is best out of:
/* act_move.c */
char *const dir_name[] = {
"north", "east", "south", "west", "up", "down"
};
/* merc.h */
int to_direction(const char* direction);
/* act_move.c */
int to_direction(const char* direction)
{
int td;
for(td = 0; td < DIR_MAX; ++td)
{
if(!str_prefix(direction, dir_name[td])
return td;
}
return -1;
}
—–OR—–
/* merh.h */
int str_to_direction args( ( const char *arg1) );
/* act_info.c */
int str_to_direction(const char *arg1)
{
if (!str_prefix (arg1, "north")) return DIR_NORTH;
if (!str_prefix (arg1, "east")) return DIR_EAST;
if (!str_prefix (arg1, "south")) return DIR_SOUTH;
if (!str_prefix (arg1, "west")) return DIR_WEST;
if (!str_prefix (arg1, "up")) return DIR_UP;
if (!str_prefix (arg1, "down")) return DIR_DOWN;
else
return -1;
}
30 May, 2009, Kline wrote in the 32nd comment:
I prefer the first route. Less typing and more use of things already provided to you in the code.
30 May, 2009, boblinski wrote in the 33rd comment:
Which is less stressful on the mud? (less work etc)
30 May, 2009, Sandi wrote in the 34th comment:
Neither one is going to be measurable. Go with the one you can read most easily. As a general rule, readability trumps efficiency concerns.
Not that I'm saying don't worry about efficiency. It still matters, especially on a shared server, and if you make a habit of being efficient, then you'll have the overhead to do inefficient things when you need to. It would be good to learn to profile now, so you have some early benchmarks to compare to later.
Things that will matter are running through the char_list or room_list (check 'mem', you'll see why), and writing to files (which is why players are discouraged from constantly saving).
BTW, my game uses player_list and fighter_list. I got the idea from a channel snippet written by Blade of -E-. He used it to make the channel code more efficient (though it doesn't include switched IMMs that would be included in the descriptor_list, so that's actually better for channels - it can be fun to have mobs talking trash to players), but I've used it for all the player-only stuff in char_update() by making a separate player_update(). By making them run at different times, it smooths out the load on the CPU.
Fighter_list is built in set_fighting(), and allows mobs to attack mobs (which is happening if you have the 'Neighborhood' area). Much more better than running through the char_list every second and a half.
Not that I'm saying don't worry about efficiency. It still matters, especially on a shared server, and if you make a habit of being efficient, then you'll have the overhead to do inefficient things when you need to. It would be good to learn to profile now, so you have some early benchmarks to compare to later.
Things that will matter are running through the char_list or room_list (check 'mem', you'll see why), and writing to files (which is why players are discouraged from constantly saving).
BTW, my game uses player_list and fighter_list. I got the idea from a channel snippet written by Blade of -E-. He used it to make the channel code more efficient (though it doesn't include switched IMMs that would be included in the descriptor_list, so that's actually better for channels - it can be fun to have mobs talking trash to players), but I've used it for all the player-only stuff in char_update() by making a separate player_update(). By making them run at different times, it smooths out the load on the CPU.
Fighter_list is built in set_fighting(), and allows mobs to attack mobs (which is happening if you have the 'Neighborhood' area). Much more better than running through the char_list every second and a half.
30 May, 2009, David Haley wrote in the 35th comment:
The second one is likely to be infinitesimally faster as it doesn't need to manage the for loop. But the efficiency gain is so trivial as to not even be worth thinking about. Sandi is completely correct that in the majority of cases, readability is much more important than efficiency. Also, the optimizer will often take care of things like this for you (like inlining the function that's just an array lookup) so you don't need to worry about it much anyway.
As for which is preferable stylistically, the first approach reuses the known list of directions, which is generally a good thing: if you change the list of directions (to include e.g. diagonals) you don't need to also change the function that converts direction strings to numbers.
As for which is preferable stylistically, the first approach reuses the known list of directions, which is generally a good thing: if you change the list of directions (to include e.g. diagonals) you don't need to also change the function that converts direction strings to numbers.
31 May, 2009, Runter wrote in the 36th comment:
Since the thread ventured into efficiency land…
This is a merc based derivative. If you want to really be more efficient then go to the aggr_update() function (which even the writer notably and admittedly by the author in the comments above it takes up a great deal of CPU, like 30-50% of all CPU used depending on the mobs in play) and rewrite the system. All this function does is govern aggressive mobs attacking.
In fact, rewrite all of your update polling systems in update.c and make them use an event system. There's a billion snippets out there for C or C++. Or just make them use black/white lists.
Regardless, as previous posters have said, efficiency of this magnitude should be the least of your worries with artefacts like the aforementioned looming around. Save yourself a headache and code primarily A) to finish with no bugs and secondarily B) to be aesthetically, efficiently pleasing. Especially when dealing with processes that take minute amounts of CPU and only called minute amounts of times. :)
[insert ramblings about using shifting to C++ from C, using a higher level languages, and love stories about Ruby]
This is a merc based derivative. If you want to really be more efficient then go to the aggr_update() function (which even the writer notably and admittedly by the author in the comments above it takes up a great deal of CPU, like 30-50% of all CPU used depending on the mobs in play) and rewrite the system. All this function does is govern aggressive mobs attacking.
In fact, rewrite all of your update polling systems in update.c and make them use an event system. There's a billion snippets out there for C or C++. Or just make them use black/white lists.
Regardless, as previous posters have said, efficiency of this magnitude should be the least of your worries with artefacts like the aforementioned looming around. Save yourself a headache and code primarily A) to finish with no bugs and secondarily B) to be aesthetically, efficiently pleasing. Especially when dealing with processes that take minute amounts of CPU and only called minute amounts of times. :)
[insert ramblings about using shifting to C++ from C, using a higher level languages, and love stories about Ruby]
31 May, 2009, Runter wrote in the 37th comment:
David Haley said:
The second one is likely to be infinitesimally faster as it doesn't need to manage the for loop. But the efficiency gain is so trivial as to not even be worth thinking about. Sandi is completely correct that in the majority of cases, readability is much more important than efficiency. Also, the optimizer will often take care of things like this for you (like inlining the function that's just an array lookup) so you don't need to worry about it much anyway.
As for which is preferable stylistically, the first approach reuses the known list of directions, which is generally a good thing: if you change the list of directions (to include e.g. diagonals) you don't need to also change the function that converts direction strings to numbers.
As for which is preferable stylistically, the first approach reuses the known list of directions, which is generally a good thing: if you change the list of directions (to include e.g. diagonals) you don't need to also change the function that converts direction strings to numbers.
I think I agree with the statement overall but I want to lay it out for the man with the question.
Both are using equal amounts of string comparisons which should be a majority of the cycles expended. Thus, the loop itself (assuming it runs to completion even) will likely be equal to one additonal similar string comparison in C. It's actually quite measurable. Considering we're talking about up to 6 string comparisons (if it has to run to completion) thats only 1/6th overall cycles expended. Quite trivial really in the grand scheme.
But if you really want to make an impact on this function–use this code
/* act_move.c */
int to_direction(const char* direction) {
register char i = 0;
while(i < DIR_MAX )
if (*direction == *dir_name[i++])
return i;
return -1;
}
This, as all of your exit names, neswud, have a unique lead character, only checks that single character before making the decision.
If you're not using this function in some funny way, that would cut cycles used by maybe 10 times.
31 May, 2009, Sharmair wrote in the 38th comment:
As far as the two functions the OP was talking about. The first will probably be smaller, but the 2nd
(as stated by others) a tiny bit faster, though being smaller, the first might make better use of the
cache and maybe make better use of the CPUs branch prediction (though, these types of things are very
hard to nail down, and really only running profile tests would tell). But the point is, the speed difference
is likely so small details like that might actually count. Both functions are functionally equivalent though.
The real reasons I like the first better is how it supports adding new directions, and even porting to
another language (I mean spoken, not computer) or another direction system (like as an example, a
space adventure that uses directions such as downspin, coreward, upspin and rimward) just by changing
the dir_name[] and changing the max constant if needed. And the changes should automatically be used
in sync by both the displaying and parsing.
Yea, that would probably be faster, but it is far from being functionally the same. You do mention how is matches
on only the first letter, but maybe you don't want 'look wall' to look west. And I don't really consider parsing for
direction and expecting it to return a not found condition and going on to parse as something other then direction
a 'funny way' use. Also, the code here really does not work at all and could even crash the calling code under the
right conditions. To both fix the code and maybe make it a bit faster:
I suppose you could even speed it up more, maybe pass the first char in a regiser and maybe something could
be done with the dir_name[] (or even have other data with just the first letter) etc. Heck, we could even get
into Abrash style asm streamlining.
(as stated by others) a tiny bit faster, though being smaller, the first might make better use of the
cache and maybe make better use of the CPUs branch prediction (though, these types of things are very
hard to nail down, and really only running profile tests would tell). But the point is, the speed difference
is likely so small details like that might actually count. Both functions are functionally equivalent though.
The real reasons I like the first better is how it supports adding new directions, and even porting to
another language (I mean spoken, not computer) or another direction system (like as an example, a
space adventure that uses directions such as downspin, coreward, upspin and rimward) just by changing
the dir_name[] and changing the max constant if needed. And the changes should automatically be used
in sync by both the displaying and parsing.
Runter said:
But if you really want to make an impact on this function–use this code
This, as all of your exit names, neswud, have a unique lead character, only checks that single character before making the decision.
If you're not using this function in some funny way, that would cut cycles used by maybe 10 times.
/* act_move.c */
int to_direction(const char* direction) {
register char i = 0;
while(i < DIR_MAX )
if (*direction == *dir_name[i++])
return i;
return -1;
}
This, as all of your exit names, neswud, have a unique lead character, only checks that single character before making the decision.
If you're not using this function in some funny way, that would cut cycles used by maybe 10 times.
Yea, that would probably be faster, but it is far from being functionally the same. You do mention how is matches
on only the first letter, but maybe you don't want 'look wall' to look west. And I don't really consider parsing for
direction and expecting it to return a not found condition and going on to parse as something other then direction
a 'funny way' use. Also, the code here really does not work at all and could even crash the calling code under the
right conditions. To both fix the code and maybe make it a bit faster:
/* act_move.c */
int to_direction(const char* direction){
register char d = *direction;
register char i = -1;
while(i < DIR_DOWN)
if(d == *dir_name[++i])
return i;
return -1;
}
I suppose you could even speed it up more, maybe pass the first char in a regiser and maybe something could
be done with the dir_name[] (or even have other data with just the first letter) etc. Heck, we could even get
into Abrash style asm streamlining.
31 May, 2009, David Haley wrote in the 39th comment:
I think it's a little dangerous to be talking about efficiency here, because we're talking about the difference between a few grains of sand and a few more grains of sand. The difference wouldn't even be noticeable in the context it's being used on a computer from 15 years ago. It's much more important at this stage to be talking about the things Sharmair or I mentioned in the second paragraphs of our posts: it's easier to expand systems that reuse constants. (There's a theme about constants emerging here… :wink:)
31 May, 2009, KaVir wrote in the 40th comment:
Sharmair said:
Yea, that would probably be faster, but it is far from being functionally the same. You do mention how is matches on only the first letter, but maybe you don't want 'look wall' to look west.
Agreed about the functionality. In addition the return value is off by 1, and the 'register' keyword is not worth using - it's only a hint, and even if the compiler uses it there's no guarantee it'll speed up the executable…it could even slow it down. Best to let your compiler worry about optimisation at that level, unless you're writing extremely specialised software.
If speed were your concern, you would be better off removing the loop and doing something like this:
#define DIR_MAX 11
char const* dir_name[DIR_MAX] =
{
"north", "down", "east", NULL, NULL,
"south", NULL, "up", NULL, "west", NULL
};
int to_direction( char const* direction )
{
int i = direction[0] % DIR_MAX;
return (!dir_name[i] || strcmp(dir_name[i], direction)) ? -1 : i;
}
However as has been pointed out already, readability is generally far more important, and tiny optimisations like the above will give no noticable benefit. If you have efficiency problems, use a profiler to find the bottleneck.
Random Picks
Start the sequence with if(), not else if().
db.c is primarily concerned with booting the mud from the files, if they wrote big words back the it would be called database.c. There are also miscellaneous functions near the end, but it would be saner to move them out rather than add more. You might put it in act_info, as the act files mostly contain player commands. Or, you could start your own file where you keep the stuff you've written yourself.
And yes, put it in merc.h. Note that RT wasn't as dogmatic about it, so there are things not included in that section. However, while it's true not everything needs to be in a header file, it sure is nice to have a big list of everything that's available for you to use.
David can be very helpful, but he's a firm believer in the "If you give a man a fish…" adage. You did a wonderful job of planning the code, and now you're well prepared to plan your next project. Keep taking his advice, and pretty soon you'll be able to do it all yourself.
(I know I wrote some code for you in another thread, but in that case people were going off in the wrong direction and you were getting confused. I finally decided writing something that did what you wanted was the only way to make my point.)