03 Dec, 2009, BrainFRZ wrote in the 1st comment:
Votes: 0
In my (desparately needed… :redface:) refactoring of my 725-line do_look function, I decided I would also convert it into C++, so I could use its improved string handler. In the process, I came across one_argument(), and decided to convert it too, because .c_str() is const. I was wondering if anyone had any ideas on how to improve this rewrite. Thanks in advance! :biggrin:

Here's the original one_argument() from SMAUG FUSS 1.9:
char *one_argument( char *argument, char *arg_first )
{
char cEnd;
int count;

count = 0;

while( isspace( *argument ) )
argument++;

cEnd = ' ';
if( *argument == '\'' || *argument == '"' )
cEnd = *argument++;

while( *argument != '\0' || ++count >= 255 )
{
if( *argument == cEnd )
{
argument++;
break;
}
*arg_first = ( *argument );
arg_first++;
argument++;
}
*arg_first = '\0';

while( isspace( *argument ) )
argument++;

return argument;
}


Here's my version:
std::string one_argument( std::string &argList, std::string &firstArg )
{
// Remove all leading spaces from argument list
for( size_t spaces = 0; argList[spaces] == ' '; ++spaces )
argList.erase(argList.begin());

char delim = ' ';
if( argList[0] == '\'' || argList[0] == '\"' )
{
delim = argList[0];
argList.erase(argList.begin()); // Remove opening quote
}

size_t dPos = argList.find_first_of(delim, 1);

firstArg = argList.substr(0, dPos);

if( delim != ' ' )
++dPos; // Remove closing quote

// Return list of remaining arguments without any leading spaces
for( ++dPos; argList[dPos] == ' '; ++dPos )
;
return argList.substr(dPos);
}
03 Dec, 2009, Davion wrote in the 2nd comment:
Votes: 0
Here's Murk++'s one_argument.

/*
* Pick off one argument from a string and return the rest.
* Understands quotes.
*/
std::string one_argument (const std::string & argument, std::string & arg_first)
{
char cEnd;
std::string::const_iterator argp = argument.begin();

arg_first.erase();
while (argp != argument.end() && isspace (*argp))
argp++;

cEnd = ' ';
if (*argp == '\'' || *argp == '"')
cEnd = *argp++;

while (argp != argument.end()) {
if (*argp == cEnd) {
argp++;
break;
}
arg_first.append(1, (char)tolower(*argp));
argp++;
}

while (argp != argument.end() && isspace (*argp))
argp++;

return std::string(argp, argument.end());
}


Only thing different here, is it sets it to all lowercase.
03 Dec, 2009, David Haley wrote in the 3rd comment:
Votes: 0
Also (without looking into this too much more) the Murk++ version correctly adds a const attribute to 'argument'.
03 Dec, 2009, BrainFRZ wrote in the 4th comment:
Votes: 0
That isn't really what I asked? =\ I don't mean to come across bone-headed or anything, especially on my first thread, but Murk++'s version seems to be an identical port from C to C++. If I'm wrong, would someone point out where it can be improved?

Quote
Also (without looking into this too much more) The Murk++ version correctly adds a const attritube to 'argument'.

Mine can't, because it automatically trims the leading spaces from argument automatically. I don't always use one_argument to strip the first argument from the argument list. The reason is for things like "look at brainfrz" vs "look in brainfrz".
03 Dec, 2009, Davion wrote in the 5th comment:
Votes: 0
Case correction and constness correction aren't improvements? Are you looking for a more specific type of improvement? (ie. speed, readability)
03 Dec, 2009, BrainFRZ wrote in the 6th comment:
Votes: 0
Davion said:
Case correction and constness correction aren't improvements?


Well, as I said, I didn't make 'argList' a const reference because I want to strip the leading spaces. Also, I don't really want to switch the case of the input, because it isn't always going to be "correct". What I mean is, what if you're using one_argument() in a profanity filter on channels? You don't want to completely butcher the case, or at least I don't think you would.

Davion said:
Are you looking for a more specific type of improvement? (ie. speed, readability)

Aye, that's what I was really looking for. :3 Also, I wanted to optimize my use of the std::string class. I don't mean to say that Murk++'s version is wrong – I don't think it's possiible for something to be "wrong" unless it's blatantly unclear, has memory leaks, or some other form of error, which Murk++'s snippet obviously doesn't have. My main reason behind converting this thing into C++ is not just to refactor it but also to get a much better understanding/knowledge of the language. I learn by doing real projects, not toy exercises. ^.^;

What I'm looking for is tips on where I can further optimize what I've done, increase its readability, and make the code as compartmentalized as possible without nuking performance.
03 Dec, 2009, David Haley wrote in the 7th comment:
Votes: 0
Quote
Well, as I said, I didn't make 'argList' a const reference because I want to strip the leading spaces.

This is a task you should be doing elsewhere, not one_argument. It's "extra magic" that the function is doing, when nominally all it should be doing is picking off arguments.

Quote
Also, I don't really want to switch the case of the input

Well, this is easy enough to do or not do.

Quote
My main reason behind converting this thing into C++ is not just to refactor it but also to get a much better understanding/knowledge of the language. I learn by doing real projects, not toy exercises. ^.^;

If your goal here is only to use as many std::string methods as possible, then I think you're transforming the task into a toy exercise. :wink:

Quote
What I'm looking for is tips on where I can further optimize what I've done, increase its readability, and make the code as compartmentalized as possible without nuking performance.

Well, I think this is all a little contrived, but you could do things like:
- don't use a for loop to strip spaces. Instead, use the 'find' method, get the iterator, and use erase up until that iterator.
- similar story for trailing spaces (find_first_not_of or whatever it's called)
- use a more explicit variable name than dPos

For efficiency, you hurt yourself by using erase (which means shuffling stuff around in memory) instead of maintaining position indices. You hurt yourself even more by calling erase one by one, instead of erasing ranges at a time. (The latter entails one shift-down of characters; the former entails one per character erased.)
03 Dec, 2009, Twisol wrote in the 8th comment:
Votes: 0
I haven't coded in C or C++ heavily in a fair bit, so forgive me if I'm a bit dense here…

BrainFRZ said:
Well, as I said, I didn't make 'argList' a const reference because I want to strip the leading spaces.

Aren't lines 13 and 14 of the Murk++ version effectively stripping the leading spaces? In your version, I'm a bit leery of modifying the parameters directly, since you're actually changing the original values passed in, and it doesn't seem like it's this function's job to do that.

David Haley said:
- similar story for trailing spaces (find_first_not_of or whatever it's called)

For trailings, I'd rather use find_last_not_of and look for the last non-space character.
03 Dec, 2009, Kayle wrote in the 9th comment:
Votes: 0
BrainFRZ said:
Quote
Also (without looking into this too much more) The Murk++ version correctly adds a const attritube to 'argument'.

Mine can't, because it automatically trims the leading spaces from argument automatically. I don't always use one_argument to strip the first argument from the argument list. The reason is for things like "look at brainfrz" vs "look in brainfrz".


This statement irks me. If there's one thing I've learned, it's to never make a function do more than it needs to. If the functions job is to strip of a word in an argument. That's what the function needs to do. It doesn't need to do anything extra. To that extent, the thing being passed to have the argument stripped off should ALWAYS be const. Ignoring constness is bad. And only leads to problems down the road. The Murk++ argument is pretty good. Although, I don't know that I would strip case from things, but the rest of the Murk++ one looks pretty good to me.
0.0/9