Bay 12 Games Forum
Other Projects => Curses => Topic started by: Ihlosi on May 16, 2013, 12:51:23 pm
-
I've been looking at The Code(tm) again. And I have a question.
In several places, most prominently in creaturenames.cpp, there are huge switch/case statements that basically map one number to a string or another number. In creaturenames.cpp, a random number is mapped to a name, for example.
Is there any special reason that I missed to use a switch/case statement in these places instead of a char ** and using the random number as an index into a string of names? As in:
char *male_first_names[] = {"Aaron", "Abraham", ...};
(Don't have a compiler right now, but I think that's the syntax for an array of variable-length char strings ...)
-
That would certainly clean up and condense the code. I'm guessing that particular function has been there for quite a long time and nobody's bothered cleaning it up.
I don't see any reason not to change it.
-
Your suggestion is almost exactly what I did for Zombie Survival Squad's name generator. The only reason for the current system of generating creature names is that it's what is already in place.
Your suggested alternate approach has several benefits. In particular, we'd be able to more easily maintain and edit the name list (keeping it sorted, deleting, and inserting are cheaper in terms of human effort), and we can have the compiler track the size of the name list without needing magic numbers:
num_names = sizeof(names) / sizeof(names[0]);
So, if someone wanted to rewrite the name generator to clean it up, that'd be cool. It's low priority for me personally, simply because it has no immediate impact on the game.
-
So, if someone wanted to rewrite the name generator to clean it up, that'd be cool. It's low priority for me personally, simply because it has no immediate impact on the game.
I just installed Code::Blocks. I'll try to get the game to compile. Unfortunately, the largest part of my programming practice and experience is writing C (plus some assembly) for microcontrollers (mostly ARM, some 8051, some DSP) and dealing with the appropriate development tools. Writing for an actual computer might have the benefit of not having to deal with emulators, "hardware in development", and debugging with an oscilloscope. ;)
I found quite a few things that would make the code more maintainable/expandable without any impact on the game. Improvements here would be investments for the future, but have not immediate benefit.
Now if I could only tear myself away from playing the game. ;)
-
num_names = sizeof(names) / sizeof(names[0]);
This only works if the array uses fixed-length strings (which may actually be advisable due to being able to use the line above, even though it leaves some memory unused), e.g.
char male_first_names[MFN_COUNT][MAX_MFN_LENGTH] = {"Aaron", "Abraham", ...}
On the other hand,
char *male_first_names[] = {"Aaron", "Abraham", ...}
does not leave any memory unused, since it is technically an array of pointers into one long string, "Aaron\0Abraham\0...".
-
num_names = sizeof(names) / sizeof(names[0]);
This only works if the array uses fixed-length strings (which may actually be advisable due to being able to use the line above, even though it leaves some memory unused), e.g.
char male_first_names[MFN_COUNT][MAX_MFN_LENGTH] = {"Aaron", "Abraham", ...}
Correct me if I'm wrong, but I don't see why that would be. sizeof(names) should be the number of bytes in a pointer times the number of names; you defined it as an array of pointers, not an array of arrays.
-
The size of a string, at least in this context, shouldn't have anything to do with the size of its contents anyway.
-
Correct me if I'm wrong, but I don't see why that would be. sizeof(names) should be the number of bytes in a pointer times the number of names; you defined it as an array of pointers, not an array of arrays.
You're right. I got some things confused. That makes this approach even more elegant than I had thought.
-
So, if someone wanted to rewrite the name generator to clean it up, that'd be cool. It's low priority for me personally, simply because it has no immediate impact on the game.
I have a changed creaturenames.cpp ready to submit. ;) Now what do I do?
Then we just need to hunt down all the other places where strings are randomly picked from a list.
-
Hm, is there a source file that holds small "utility" functions that are used frequently by all parts of the code?
My approach uses a generic "pick random string function":
const char *SelectRandomString(const char **string_table, int table_size)
{
int roll;
roll = LCSrandom(table_size);
return(string_table[roll]);
}
that picks one random string from a given array, e.g.
static const char *male_first_names[] =
{
"Aaron", "Abraham", "Adam", "Adolph", ...
};
I placed it in creaturenames.cpp, but logically it should go in a more general place.
-
I have a changed creaturenames.cpp ready to submit. ;) Now what do I do?
Cool! Well, the easiest for me is that you could PM me with your Sourceforge username and I can add you as a developer on the project. Then you can SVN commit your changes.
Hm, is there a source file that holds small "utility" functions that are used frequently by all parts of the code?
Mmm, maybe src/common/misc.cpp? It's not hugely important; the code isn't really all that organized to begin with. Toady left everything in one file tens of thousands of lines of code long, and when I mapped out the revised file structure for breaking that monolith up, I had all of one year of programming experience, and hadn't worked on anything of this size before. It's a small miracle it's as organized as it is.
-
Cool! Well, the easiest for me is that you could PM me with your Sourceforge username and I can add you as a developer on the project. Then you can SVN commit your changes.
Done. I still have to get used to SVN, though. I'm using a different (proprietary) revision control system at work.
Toady left everything in one file tens of thousands of lines of code long,
*faints*
Well, we all learn (usually the hard way) from beginners mistakes. Some of mine are over ten years old and still occasionally come back to haunt me in the form of nasty assembly blobs. I've grown wiser.
-
Okay, I think I managed to commit some changes using the new selectRandomString function. Let me know what you think and then I can work on all the other occurrences.
Basically, a search for "case 0:addstr(" gives possible places where the new function might be used.
-
Hm. I keep running into code parts that are almost, but not quite, about picking strings from a selection. They modify some or all of the possible strings in some way, e.g. by adding object/creature names, pronouns, or by taking one or more of the current laws (most prominently: free speech) into account.
I wonder how much coding effort it would be to write a parser for strings that contain all of the information necessary to assemble the output string in a simple markup language, like <rnd:a|b|c> for a random choice or <freespeech|[rats]|crap|crap|crap|shit> for a choice depending on the current free speech law.
-
Hm. I keep running into code parts that are almost, but not quite, about picking strings from a selection. They modify some or all of the possible strings in some way, e.g. by adding object/creature names, pronouns, or by taking one or more of the current laws (most prominently: free speech) into account.
I wonder how much coding effort it would be to write a parser for strings that contain all of the information necessary to assemble the output string in a simple markup language, like <rnd:a|b|c> for a random choice or <freespeech|[rats]|crap|crap|crap|shit> for a choice depending on the current free speech law.
Well, at that point I wonder if you're really gaining in clarity by avoiding a switch statement? You'll take some of the lines of code out, but it seems to me that custom markup is significantly less self-documenting than function calls.
The changes to creaturenames.cpp look good. I'm guessing you already have your IDE set to three space tabs, but you're using tab characters -- you should switch to three space indentation with spaces instead of tabs, since this is how most of the project is built. Mixing tabs and spaces won't immediately suck in your editor, as long as your tab length is the same as the number of spaces used in the project, but as soon as somebody tries to read the code with a fancy visual diff tool that uses eight space tabs, the code readability is going to go to hell due to inconsistent whitespace.
-
Well, at that point I wonder if you're really gaining in clarity by avoiding a switch statement? You'll take some of the lines of code out, but it seems to me that custom markup is significantly less self-documenting than function calls.
That probably depends on finding a good way to separate game mechanics from text output. I haven't had any brilliant ideas there, mostly because my usual code doesn't involve things like text output, or even any kind of interaction with an actual human. :P
The changes to creaturenames.cpp look good. I'm guessing you already have your IDE set to three space tabs, but you're using tab characters -- you should switch to three space indentation with spaces instead of tabs, since this is how most of the project is built.
Okay, I need to check all of the editor settings again. I unchecked the "Use TAB character" and "TAB indents" fields in code::blocks editor settings and set "TAB size in spaces" to 3, and I was pretty sure I wouldn't be littering the code with tab characters this way.
-
I think I found the relevant settings in code::blocks: "Use TAB character" needs to be unchecked, but "TAB indents" needs to be checked in order to have the editor use spaces for indentations. Sounds a bit counter-intuitive.
Yet another question:
The code often assembles output strings with repeated calls to addstr():
addstr("Choose a Liberal squad member for Place ");
char num[20];
itoa(spot+1,num,10);
addstr(num);
addstr(".");
Is there any special reason/philosophy for this instead of generating the complete formatted output string first and then printing it via addstr?
E.g.:
char line_buf[80];
sprintf(line_buf, "Choose a Liberal squad member for Place %i.", (spot + 1));
addstr(line_buf);
(This could probably rolled into one function, to avoid having a separate line_buf in hundreds of functions all over the code.)
-
addstr("Choose a Liberal squad member for Place ");
char num[20];
itoa(spot+1,num,10);
addstr(num);
addstr(".");
Is there any special reason/philosophy for this instead of generating the complete formatted output string first and then printing it via addstr?
E.g.:
char line_buf[80];
sprintf(line_buf, "Choose a Liberal squad member for Place %i.", (spot + 1));
addstr(line_buf);
(This could probably rolled into one function, to avoid having a separate line_buf in hundreds of functions all over the code.)
I think this is just a quirk of how Tarn was comfortable initially coding the game; string formatting would certainly be more concise.
If you end up defining a new function to wrap the temp variable (that's what I would do), take a look at the custom overloads of addstr() and mvaddstr() at the bottom of common/commondisplay.cpp. You could decline to support std::String objects since you're just doing c-string formatting, but Log support is fairly important, and is trickier, since an overload in the style we're currently doing (optional last parameter) would conflict with the variable parameter list. In any case, I'd definitely supply a mvaddstr() variant (wraps move() and addstr() into one call) for any new addstr() variant you added.
-
addstr("Choose a Liberal squad member for Place ");
char num[20];
itoa(spot+1,num,10);
addstr(num);
addstr(".");
Is there any special reason/philosophy for this instead of generating the complete formatted output string first and then printing it via addstr?
E.g.:
char line_buf[80];
sprintf(line_buf, "Choose a Liberal squad member for Place %i.", (spot + 1));
addstr(line_buf);
(This could probably rolled into one function, to avoid having a separate line_buf in hundreds of functions all over the code.)
I think this is just a quirk of how Tarn was comfortable initially coding the game; string formatting would certainly be more concise.
If you end up defining a new function to wrap the temp variable (that's what I would do), take a look at the custom overloads of addstr() and mvaddstr() at the bottom of common/commondisplay.cpp. You could decline to support std::String objects since you're just doing c-string formatting, but Log support is fairly important, and is trickier, since an overload in the style we're currently doing (optional last parameter) would conflict with the variable parameter list. In any case, I'd definitely supply a mvaddstr() variant (wraps move() and addstr() into one call) for any new addstr() variant you added.
I always thought that "printing and adding to a log file" should have a different function from "just printing", say laddstr() or something. Like there's printf(...) and fprintf(stderr,...) for outputting to standard and error streams. Though, as I'm more of a C programmer, I don't know how acceptable would that be in C++.
But yeah, a printf-style variation on addstr would be really nice.
-
But yeah, a printf-style variation on addstr would be really nice.
Hm, I think I just realized that this might not be as easy as it sounds - functions can be declared to have a variable number of arguments, but the actual number of arguments for each call (and their type, etc) must be known at compile time. This means that you can't just hand over those arguments to another function.
A workaround might be to use some clever overloading, but that limits the new function to the given set of parameter combinations, i.e. have one printf_addstr() that takes one integer, one that takes a string, one that takes two integers, etc.
Another way to avoid each function having their own 80-char buffer for sprintf might be to make this buffer global. As long as the program is nice and single-threaded, there would not be any conflicts when accessing that buffer. I'm still not sure whether that would count as "nasty hack" or "most straightforward solution", or both. :o
-
Hm, I think I just realized that this might not be as easy as it sounds - functions can be declared to have a variable number of arguments, but the actual number of arguments for each call (and their type, etc) must be known at compile time. This means that you can't just hand over those arguments to another function.
While this is a huge barrier for directly wrapping the printf and sprintf functions, the wise minds providing the standard libraries anticipated exactly this problem and provided alternate versions that will accept your va_list directly rather than expecting you to decompose it into separate arguments. Try using vsprintf instead of sprintf!
-
While this is a huge barrier for directly wrapping the printf and sprintf functions, the wise minds providing the standard libraries anticipated exactly this problem and provided alternate versions that will accept your va_list directly rather than expecting you to decompose it into separate arguments. Try using vsprintf instead of sprintf!
Heh. Things you never learn when your code never needs to format strings ...
So a formatted version of addstr() could look like this?
void addstrf( const char * format, ... )
{
static char buffer[256];
va_list args;
va_start (args, format);
vsnprintf (buffer,256,format, args);
va_end (args);
addstr(buffer);
}
-
If you end up defining a new function to wrap the temp variable (that's what I would do), take a look at the custom overloads of addstr() and mvaddstr() at the bottom of common/commondisplay.cpp. You could decline to support std::String objects since you're just doing c-string formatting, but Log support is fairly important, and is trickier, since an overload in the style we're currently doing (optional last parameter) would conflict with the variable parameter list. In any case, I'd definitely supply a mvaddstr() variant (wraps move() and addstr() into one call) for any new addstr() variant you added.
Hm ... addstr() wrappers that include log support would probably need to take the log object as the first argument (or one of the first arguments) instead of the last.
However, making a custom overload of a library function that adds functionality not directly related to the library functions original task seems like a recipe for future headaches. Using a macro instead would clarify that there is additional functionality and provide a simple means of deactivating the additional functionality by changing the macro.
-
Ihlosi, please use unix-style line endings in your commits.
-
Ihlosi, please use unix-style line endings in your commits.
Changed end-of-line in code::blocks' editor setting to "LF". That should do it, I think.
-
But yeah, a printf-style variation on addstr would be really nice.
I think I'm ready to add one.
However, keeping logging support in the present format (log object as the last function argument) would be a bit of a hassle due to the variable argument list of vsnprintf. Moving the log object argument to the beginning of the list (before or after any x/y coordinates for a move()?) would make things easier.
-
I added printf-style versions of addstr and mvaddstr (with and without logging).
-
Just wanted to mention how convenient I'm finding the formatted addstr variants. Thanks, Ihlosi.