24 Sep, 2008, Davion wrote in the 22nd comment:
if you use strdup on rank, make sure you free it
24 Sep, 2008, Fizban wrote in the 23rd comment:
Just curious, maybe it's just me, but after looking at ROMs do_who I'm rather disgusted, it suffers from massive bloat (ie. why the hell are things like (MAX_LEVEL - 1) not defined in a .h file somewhere and things like if ( arg[0] == '\0' ) just seem cryptic to me when compared to if (!*arg) because the latter doesn't confuse people who don't understand numbering in an index (ie. why arg[3] references the 4th character and not the third). Am I the only one that thinks the code is absolutely, messy or is ROM's do_who coded in an acceptable manner in most people's eyes?
24 Sep, 2008, David Haley wrote in the 24th comment:
I agree that the levels should be #defined somewhere, but I disagree about !*arg being so much obviously and absolutely clearer. Actually, I prefer the indexed version… You don't need to worry about confusing people with 3 vs. 4 when you're talking about zero. Additionally, keeping it with characters reinforces the concept that we're talking about strings, not boolean values.
In general though I turn off my style judging when looking at most MUD code… I'd never get anything done if I made the code conform to good style conventions (where good == something a high-standards professional software developer would produce).
In general though I turn off my style judging when looking at most MUD code… I'd never get anything done if I made the code conform to good style conventions (where good == something a high-standards professional software developer would produce).
24 Sep, 2008, Skol wrote in the 25th comment:
DavidHaley said:
No need to sprintf when you just want to grab a string constant…
Note proper placement of const.
The sprintf method given is dangerous anyhow, because
(a) it uses magic numbers for the maximum size
(b) if the maximum size increases, the code will cause a buffer overflow
It's also wasteful because it's not necessary to do any copying when you already have all the strings lying around as constants, and all you want is to remember which constant is relevant.
const char* class;
class = class_table[wch->class].who_name;
if (class == NULL) { class = "Unknown"; }
Note proper placement of const.
The sprintf method given is dangerous anyhow, because
(a) it uses magic numbers for the maximum size
(b) if the maximum size increases, the code will cause a buffer overflow
It's also wasteful because it's not necessary to do any copying when you already have all the strings lying around as constants, and all you want is to remember which constant is relevant.
I hear you on the sprintf method, one thing though, since they're going to output it via sprintf (line by line) later on in the code, why even mess with it?
Rather, wait until the sprintf statement and simply make it %s, then class_table[wch->class].who_name == NULL ? "Unknown" : class_table[wch->class].who_name. That make sense?
24 Sep, 2008, David Haley wrote in the 26th comment:
IMO having temporary variables makes the printing cleaner. Compare
("…%s…", class_name)
and
("…%s…", …, class_table[wch->class].who_name == NULL ? "Unknown" : class_table[wch->class].who_name
Since using the temporary variable is very easy (just literally assign the appropriate pointer to class_name) I think the first one is better than the second. It's much more obvious what the intention is: you don't have to actually read what that condition is doing.
("…%s…", class_name)
and
("…%s…", …, class_table[wch->class].who_name == NULL ? "Unknown" : class_table[wch->class].who_name
Since using the temporary variable is very easy (just literally assign the appropriate pointer to class_name) I think the first one is better than the second. It's much more obvious what the intention is: you don't have to actually read what that condition is doing.
24 Sep, 2008, Skol wrote in the 27th comment:
Thanks David, I think I can see places in the code where it'd very much make sense to use that approach over the old sprintf (or in lieu of part of it, magic buffer sizes and all that issue heh). It seems we still end up doing the same steps, point to it, make sure it's not NULL, then spit it back out, just that the const char approach doesn't tie up extra (or overrun). That sound about right?
Appreciate the feedback btw, I have been solo as a coder for a few years and it's easy to pick up habits. Was much easier to keep my code clean when I had the pro-programmer looking goin 'eww, fix that, yuck, and go read AB on C again'. heh.
Appreciate the feedback btw, I have been solo as a coder for a few years and it's easy to pick up habits. Was much easier to keep my code clean when I had the pro-programmer looking goin 'eww, fix that, yuck, and go read AB on C again'. heh.
24 Sep, 2008, David Haley wrote in the 28th comment:
Skol said:
It seems we still end up doing the same steps, point to it, make sure it's not NULL, then spit it back out, just that the const char approach doesn't tie up extra (or overrun). That sound about right?
Yup, that's exactly it. Instead of copying the string, you just store a pointer to it. (Unless, of course, you actually want to modify it, in which case you need a copy…)
Skol said:
Appreciate the feedback btw, I have been solo as a coder for a few years and it's easy to pick up habits. Was much easier to keep my code clean when I had the pro-programmer looking goin 'eww, fix that, yuck, and go read AB on C again'. heh.
No problem. :smile: My approach is to try to always write clean cod,e even if it's "just for me" – that way I avoid the bad habits, and in fact develop good habits. Over time it becomes much easier to write clean code from the first go.
24 Sep, 2008, gorex wrote in the 29th comment:
I feel like all of this still doesn't solve my problem.
25 Sep, 2008, David Haley wrote in the 30th comment:
Why aren't you able to do whatever you're trying to do? I thought we were talking about the above because it was getting in your way or you were getting errors because of it…
25 Sep, 2008, gorex wrote in the 31st comment:
i'm able to remove the switch statement and print out the CLASS just fine. I want to use that switch statement for wch->level replacing level 60 with IMP, 59 with DEM and so on.
25 Sep, 2008, David Haley wrote in the 32nd comment:
I guess I'm not seeing where the problem is. Why can't you use the switch statement that switches on level and assigns a string to a variable, and then print that variable when you display the who line?
26 Sep, 2008, gorex wrote in the 33rd comment:
Am I seriously not making any sense? I can't figure out another way to explain it. The comments on page 2 bring the code back to how it originally executed [209 Drow IMP] <–display after execution.
209 being the level, Drow being race, and IMP being my "switched" class (if it wasn't switched it would display "Cle", cleric). Once again, that is how it originally was. Now, I'm trying to have it appear as [IMP Drow Cle] .
IMP now being wch->level switched, the race, and Cle the class. By comment out the switch for class I am able to have the "Cle" (Cleric displayed). However, I cannot get the 209 to switch to IMP, or 208 to switch to DEM, and so on. Please refer to the code post on page 2, to see how the switch statement looks now with "rank".
209 being the level, Drow being race, and IMP being my "switched" class (if it wasn't switched it would display "Cle", cleric). Once again, that is how it originally was. Now, I'm trying to have it appear as [IMP Drow Cle] .
IMP now being wch->level switched, the race, and Cle the class. By comment out the switch for class I am able to have the "Cle" (Cleric displayed). However, I cannot get the 209 to switch to IMP, or 208 to switch to DEM, and so on. Please refer to the code post on page 2, to see how the switch statement looks now with "rank".
26 Sep, 2008, Kayle wrote in the 34th comment:
Well, without copying the level into a string, you're not going to be able to get this to work as the code is written. You can't store a string (IMP, etc) as an int and print it as an int (string won't display on %d).
If you want this to work, you'll have to copy wch->level into a string. And then if they're a IMP or whatever, you then switch it out.
If you want this to work, you'll have to copy wch->level into a string. And then if they're a IMP or whatever, you then switch it out.
26 Sep, 2008, Fizban wrote in the 35th comment:
Kayle said:
Well, without copying the level into a string, you're not going to be able to get this to work as the code is written. You can't store a string (IMP, etc) as an int and print it as an int (string won't display on %d).
If you want this to work, you'll have to copy wch->level into a string. And then if they're a IMP or whatever, you then switch it out.
If you want this to work, you'll have to copy wch->level into a string. And then if they're a IMP or whatever, you then switch it out.
That's what was done, if I remember he was getting an error from: case MAX_LEVEL - 0 : rank = "IMPL"; break; but I have no clue as I see no possible issue with that line (char rank[3]; was the definition of rank).
26 Sep, 2008, David Haley wrote in the 36th comment:
You can't assign "IMPL" to a type char rank[3]. Rank should be defined as a const char* in that case.
As for how to display "[IMP Drow Cle]", well, just make three variables: one for the first thing, to which you assign based on level, one for the second, to which you assign based on class, and finally one for the third, to which you assign based on class. Is there a particular problem, or is the problem that you don't know how to even start approaching this task?
As for how to display "[IMP Drow Cle]", well, just make three variables: one for the first thing, to which you assign based on level, one for the second, to which you assign based on class, and finally one for the third, to which you assign based on class. Is there a particular problem, or is the problem that you don't know how to even start approaching this task?
26 Sep, 2008, Fizban wrote in the 37th comment:
DavidHaley said:
You can't assign "IMPL" to a type char rank[3]. Rank should be defined as a const char* in that case.
As for how to display "[IMP Drow Cle]", well, just make three variables: one for the first thing, to which you assign based on level, one for the second, to which you assign based on class, and finally one for the third, to which you assign based on class. Is there a particular problem, or is the problem that you don't know how to even start approaching this task?
As for how to display "[IMP Drow Cle]", well, just make three variables: one for the first thing, to which you assign based on level, one for the second, to which you assign based on class, and finally one for the third, to which you assign based on class. Is there a particular problem, or is the problem that you don't know how to even start approaching this task?
That was a typo, he had "IMP", not "IMPL".
26 Sep, 2008, David Haley wrote in the 38th comment:
Doesn't matter. You can't assign a string constant like that to an array of characters. (Besides, "IMP" is 4 characters, not three, since you have to count the trailing \0.)
26 Sep, 2008, gorex wrote in the 39th comment:
Figured it out. Thanks.
Note proper placement of const.
The sprintf method given is dangerous anyhow, because
(a) it uses magic numbers for the maximum size
(b) if the maximum size increases, the code will cause a buffer overflow
It's also wasteful because it's not necessary to do any copying when you already have all the strings lying around as constants, and all you want is to remember which constant is relevant.