10 Jan, 2009, Igabod wrote in the 1st comment:
Votes: 0
Hey, I submitted a snippet here that is supposed to be a selfclass command for low4 muds. I'd like some constructive criticism and ideas on how to improve it.

Any help given to me will be credited in my next release. You can find the snippet HERE. Thanks in advance for any help.
12 Jan, 2009, zazel wrote in the 2nd comment:
Votes: 0
You could create an array which holds all the possible arguments, then check to see if the class arg is one of those, then just have a single function that sets the class, and have just a couple ifs to handle things like vamps which need special lines.

Functionally its the same, just a way to do the same in less code.
12 Jan, 2009, zazel wrote in the 3rd comment:
Votes: 0
i just realized that what i said might not be possible as ANSI C dosent do key'd arrays…let me think of a workaround
12 Jan, 2009, zazel wrote in the 4th comment:
Votes: 0
<edit>
was a bunch of pseudocode here…
</edit>

http://fluxstorm.net/snippet/selfclass.t...
13 Jan, 2009, ajrillik wrote in the 5th comment:
Votes: 0
Thanks for the snippet Igabod.
15 Jan, 2009, Igabod wrote in the 6th comment:
Votes: 0
Snippet has been updated to fix a few problems that Kline pointed out to me. You can download the updated version whenever it gets approved.

I'll update the snippet even further when I figure out an efficient way of reducing the redundant code. If anybody has any ideas on how to improve this snippet please feel free to post them either here or on the comments for the snippet itself. Also if you find any bugs please let me know.

Thanks Kline and The_Fury for your input.
15 Jan, 2009, Lobotomy wrote in the 7th comment:
Votes: 0
Giving the snippet a quick glance, I do have a few suggestions for you:

A) Move the check "if (IS_NPC(ch)) return;" to the very start of the function. No need to waste CPU time running one_argument if it's just going to kick out should an NPC somehow access it.

B) Turn your syntax information display into a single "stc" (send_to_char, I assume) call.

Before:
if( arg1[0] == '\0' )
{
stc( "Syntax: selfclass <class>\n\r", ch );
stc( "\n\r", ch );
stc( "Class being one of:\n\r", ch );
stc( "vampire werewolf demon ninja drow monk highlander\n\r", ch );
stc( "\n\r", ch );
return;
}

After:
if( arg1[0] == '\0' )
{
stc( "Syntax: selfclass <class>\n\r"
"\n\r"
"Class being one of:\n\r"
"vampire werewolf demon ninja drow monk highlander\n\r"
"\n\r", ch );
return;
}


C) Compress similar ifchecks to cut down on code repetition.

Example:
struct some_example_table
{
char * name;
int class;
};

const struct some_example_table example_table[] =
{
{ "monk", CLASS_MONK }
{ "etc", CLASS_ETC }
};

for( int a = 0; a < (# of entries in the static table. In this example, 2.); a++ )
{
if( !str_cmp( example_table[a].name, arg1 ) )
{
ch->class = example_table[a].class;
free_string( ch->lord );
ch->lord = str_dup( "" );
ch->pcdata->stats[UNI_GEN] = 1;
sprintf( buf, "%s is now a %s.", ch->name, example_table[a].name );
do_info( ch, buf );
sprintf( log_buf, "%s is now a %s", ch->name, example_table[a].name );
log_string( log_buf );
ch_printf( ch, "You are now a %s!\n\r", example_table[a].name );
do_mclear( ch, "self" );
return;
}
}

Note: Your codebase may have some sort of class table that exists already (such as class_table in Smaug, I think), so if it does you'd probably be better off using it instead, depending on how it's set up. I'm not familiar with "low4" and what all has been done with that codebase. The static tables are just an example of how to set up something for the function to use should other tables not exist.

D) Don't create multiple syntax sections; just re-call the function when necessary giving it a blank input.

Before:
stc("Syntax: selfclass <class>\n\r",ch);
stc("\n\r",ch);
stc("Class being one of:\n\r",ch);
stc("vampire werewolf demon ninja drow monk highlander\n\r",ch);
stc("\n\r",ch);
return;

After:
do_selfclass( ch, "" );
return;
15 Jan, 2009, Sharmair wrote in the 8th comment:
Votes: 0
Here is one idea of some changes you could make:
void do_selfclass(CHAR_DATA* ch, char* argument){
char arg1[MAX_INPUT_LENGTH];
const char* clname = "";

if(IS_NPC(ch))
return;
if(ch->level < 3){
*arg1 = '\0';
do_huh(ch, arg1);
return;
}
argument = one_argument(argument, arg1);
if(ch->class != 0 || IS_SET(ch->special, SPC_CHAMPION)){
stc("You already have a class!\r\n", ch);
return;
}
if(!str_cmp(arg1, "monk")){
ch->class = CLASS_MONK;
clname = "Monk";
}else if(!str_cmp(arg1, "drow")){
ch->class = CLASS_DROW;
clname = "Drow";
}else if(!str_cmp(arg1, "ninja")){
ch->class = CLASS_NINJA;
clname = "Ninja";
}else if(!str_cmp(arg1, "vampire")){
ch->class = CLASS_VAMPIRE;
clname = "Vampire";
ch->beast = 0;
SET_BIT(ch->immune, IMM_SUNLIGHT);
}else if(!str_cmp(arg1, "werewolf")){
ch->class = CLASS_WEREWOLF;
clname = "Werewolf";
}else if(!str_cmp(arg1, "demon")){
ch->class = CLASS_DEMON;
clname = "Demon";
ch->special = SPC_DEMON_LORD;
}else if(!str_cmp(arg1, "highlander")){
ch->class = CLASS_HIGHLANDER;
clname = "Highlander";
ch->pcdata->powers[0] = 1;
ch->pcdata->stats[0] = 1;
}else{
stc("Syntax: selfclass <class>\r\n"
"\r\n"
"Class being one of:\r\n"
"vampire werewolf demon ninja drow monk highlander\r\n\r\n", ch);
return;
}
free_string(ch->lord);
ch->lord = str_dup("");
ch->pcdata->stats[UNI_GEN] = 1;
sprintf(arg1, "%s is now a %s.", ch->name, clname);
do_info(ch, arg1);
log_string(arg1);
sprintf(arg1, "You are now a %s!\r\n", clname);
stc(arg1, ch);
strcpy(arg1, "self");
do_mclear(ch, arg1);
return;
}

Some notes on my changes:
I put all the common code at the end (I think you would have problems trying to put it
before the if checks). I was thinking of prefixing the input options, but figured that in a
case like this, exact matching would probably be better. This allowed me to just put the
invalid input at the end of the checks and catch the no argument case there too. You
might notice how I used just one stc() call for the invalid input message. but still having
it look in the code somewhat close to how it would look on the screen. In C/C++, if you
put two quoted strings next to each other with just whitespace between them, it is the
same as if you had just one string. I also removed some unneeded code and used just
arg1 for all the buffer work. Though most of the things I did were just style changes,
one change I made was really a bug fix (other then the /r/n stuff).
do_huh(ch, "");

And the likes are really const incorrect. When C first came out, there was no const
keyword so string literals were the same as any string. Then they decided to be a bit
sloppy with const (a quoted string is really a const char*) with literals as to not break
a lot of existing code. Some of the new compilers are getting more picky about this,
and you might start getting a warning with code like that. To fix it I set up the buffer
with the intended argument and pass that.

I few other things you could do:
I added clname to define the name to send to the player later, but as all the input
options are exact matched, and are the same as the output, you could just use arg1
(capitalizing it) and not use clname at all (you would have to add back in the 2nd
buffer though for the sprintfs that use clname now).
You could also set up an array of structs with the data for each class and use that
data for everything from the input option to what to set in one common block of
code. Using such a method would allow you to add a new class simply by adding an
element to the array (one reason I did not give an example of this is I don't know
what value the data is you don't specifically set - like ch->beast for a non vampire).
15 Jan, 2009, David Haley wrote in the 9th comment:
Votes: 0
Quote
for( int a = 0; a < (# of entries in the static table. In this example, 2.); a++ )

You can get the size of a static array like that by dividing the size of the whole array by the size of an individual structure. For example, sizeof(array var)/sizeof(type).
15 Jan, 2009, elanthis wrote in the 10th comment:
Votes: 0
A slightly more fool-proof way would be:

const size_t array_size = sizeof(array_var) / sizeof(array_var[0]);

That way you don't need to worry about accidentally forgetting to update the sizeof if you ever happen to change the type of the array elements. :)
16 Jan, 2009, Igabod wrote in the 11th comment:
Votes: 0
ok, I definitely appreciate all the help, but the problem is that it looks like you guys were looking at the old version of the snippet before the new one was approved. I've fixed a bunch of the problems that were noted already.

As to the if (IS_NPC(ch)) return; line, I hadn't even noticed I put that after the argument = one_argument line. This will be fixed and included in the next update.

Sharmair's rewrite of the selfclass function actually looks a lot better than mine but I wouldn't be able to release that since it's absolutely nothing like mine and isn't really just an improvement of my code. He should release that as his own snippet though, I'm all for giving people plenty of choices between snippets. I'll definitely be borrowing some ideas from that function but I can't just copy it.

I really like the idea of using just one stc for the entire message, I was unaware that would work but will be using that in the next update as well.

As for all the common code, I feel like an idiot now cause I tried putting all of that at the bottom and at the top, but it didn't work for me and I was wondering what went wrong, now I know it's cause I forgot to put the else at the bottom before the do_selfclass so it wasn't even getting to the stuff after that. This is what happens when you decide to try programming after working a 10 hour shift and getting no sleep the night before. This too will be fixed in the next update.

I'm sure I missed a few other valid points so I'm copy/pasting all this into notepad so I can peruse it further when I get off work. Thanks again to all of you for the ideas and for pointing out things I missed. Now, I've gotta go talk to the owner of the hotel I manage cause he just came in.
16 Jan, 2009, David Haley wrote in the 12th comment:
Votes: 0
Igabod said:
Sharmair's rewrite of the selfclass function actually looks a lot better than mine but I wouldn't be able to release that since it's absolutely nothing like mine and isn't really just an improvement of my code. He should release that as his own snippet though, I'm all for giving people plenty of choices between snippets. I'll definitely be borrowing some ideas from that function but I can't just copy it.

Well, I think it would be fairly safe to assume that he meant for you to use it, but you are right that it would be polite to make sure first. I think it would be better to have just one version with all improvements included, so IMHO it would be worth asking to include it in the version you're posting/updating.

Igabod said:
I really like the idea of using just one stc for the entire message, I was unaware that would work but will be using that in the next update as well.

It's a nifty feature of C: several string literals in a row become one long string literal. It's also nice for breaking up long strings into multiple lines.
16 Jan, 2009, Igabod wrote in the 13th comment:
Votes: 0
Well if sharmair approves I can put his version in WITH my version, they'll have the same functionality but will look quite different so I figure it's cooler if you have two different versions you can choose from in the same snippet. If he approves I'll be sure to give him full credit for it of course.
16 Jan, 2009, Sharmair wrote in the 14th comment:
Votes: 0
I don't mind if you use the code in your snippet, as an alternate or even as a base
for your continuing work.
16 Jan, 2009, Igabod wrote in the 15th comment:
Votes: 0
Alright then, thank you Sharmair. You will be duly credited in the next version.
17 Jan, 2009, Igabod wrote in the 16th comment:
Votes: 0
The third and hopefully final version of this snippet has been uploaded and is now pending approval. I want to thank everybody for their input and invite you all to check out this version as soon as it is approved. I was going to release sharmair's version along with what I came up with after all the fixes were implemented but since they actually looked very similar when I was done, I just combined his version with mine and it turned out to be pretty nice IMHO.

Again I welcome any and all constructive criticism. To Sharmair, I didn't take your advice about the do_huh(ch,""); because I wanted this snippet to be as uniform to the rest of the low4 code as possible. I am not sure how buggy doing this really is but since it works with my compiler at the moment I see no problem with it. Other than that I think I used all of your other suggestions though, so thank you very much and you were credited for your help in the newest release.
18 Jan, 2009, Igabod wrote in the 17th comment:
Votes: 0
Seems this post got bumped off the recent posts list before anybody saw it so I'm bumping it back up.
Random Picks
0.0/17