Re: [Dev-C++] Multiple Search and Replace Function
Open Source C & C++ IDE for Windows
Brought to you by:
claplace
Menu
▾
▴
Re: [Dev-C++] Multiple Search and Replace Function
|
From: Graham D. <g.d...@nt...> - 2003-06-27 19:26:57
|
Critique added below. I hope that I've been constructive - if it comes
over otherwise, please accept that that is not my intention. I get the
impression from earlier postings that you've made that you are fairly
new to STL (and possibly C++?). On that assumption, I've tried to make
as many detailed critiques as I could think of. If I'm wrong in my
assumption, again accept my apologies.
In particular, I have a real "thing" about over-commented code, as
you'll see. For one thing, I simply detest blindingly obvious comments
like "x = x + 1; // Add 1 to x". There are many reasons for my dislike
of excessive comments, including:
* comments get in the way of finding the real code (not everyone
has syntax colouring), so keep them to a necessary minimum;
* comments tend not to get updated to reflect what modified code
does, and so start lying;
* good code comments itself.
The first two there come from many painful years of trawling through
badly (over-)commented code. The last one comes from a Scott Meyers
comment: "Say what you mean and understand what you are saying". If you
can write good code, you can read good code. If you can't write good
code, then you can learn a lot by reading good code.
So, if any of my comments below seem a bit "tart", please accept my
apologies in advance: they're not meant to be, I just wanted to try to
keep them short.
By the way, I also appreciate that you may have written the code for a
rigidly defined input domain. However, my experience is that such code
lasts a couple of months before someone wants to use it more widely.
Hence, I've made a number of comments about (lack of) error checking,
even though you may argue that it's not necessary for the input you expect.
CARLSON, JAY MM1 wrote:
>For all new programmers, if you follow these programs from me you will see
>some examples of how I write code, (good or bad). The initial version of
>this program would search for a text and replace it with another text. It
>did that but I have a much larger purpose in mind. My end goal is to make a
>program (that anyone can use - although I don't know how many would want to)
>that will take a file that I want to import into Access 2000 and strip off
>all the junk headers and footers that keep the columns from importing
>without errors.
>
>The multiple search and replace function is step two in what I believe will
>be a 4 step process. Step three will be to open files and read text,
>already done, but will post later.
>Step 4 will be to combine steps 3 and 2 into a coherent program.
>
>Here is step two.
>
>// Multiple Search and Replace Function
>// For use and analysis especially for new programmers
>//
>// Praise God for His Goodness and Mercy
>// 27 June 2003 Jay A Carlson
>
>// Multiple Search and Replace Implementation and Testing
>//
>// Takes an input text and replacement text and makes it happen
>// In the future I will have an input text file and a change file
>// to end up with a new converted file.
>//
>// The search and replace specifications take the form of
>// <> <search for this> <replace with this>;
>// number of replacements (0 means replace all)
>//
>// [] [search] [replace]
>// 5
>//
>// <> <myArray[9]> <thisArray[10]>
>// 3
>//
>// ^* ^std::cout<<test[5];* ^printf("test[3]");*
>// 0
>//
>// Notice by being able to specify the start and stop delimiters for each
>// search and replace you don't have to worry about nesting problems.
>//
>// abbreviations used
>// srt -- search and replace text
>
But it isn't, is it? There is no search and replace text - there's
search text, and there's replace text and they are separate. Unless you
mean the text that is to be searched and modified....
>// arc = actual_replacement_count
>// drc = desired_replacement_count
>
Rather than add this comment, use the long names for the variables in
the code.
>
>#include <iostream> // for std::cout
>#include <string> // for std::string
>#include <vector> // guess what - for vectors
>
Anyone who needs those comments on the #includes isn't qualified to be
reading the code. (And if they're reading for learning purposes, and
don't know those includes, then they aren't going to learn anything.)
>
>// this is a function whose sole purpose in life is to get the word, or
>words
>// between the delimiters represented by the variables start and stop
>// debug -- a way of showing more or less information in a hurry
>
The function name - getword_s - tends to belie the above comment. The
description is of a form of substring, not "get word", which implies the
characters between two white space or punctuation characters.
>std::string getword_s(char start, char stop, std::string &srt) {
>
Personally, I wouldn't used unsigned for anything other than bit
manipulation. There are too many awkward implications from using unsigned.
> unsigned int first = srt.find_first_of(start) + 1;
>
What happens if find_first_of(start) returns npos? Note that
basic_string::npos is defined as
static const size_type npos = -1;
What implication does that have for the future correctness of your search?
> unsigned int last = srt.find_first_of(stop) - 1;
>
What if "stop" appears in the string earlier than "start"?
> unsigned int len = last - first + 1; // length of word(s) between
>delimiters
>
"last" should point to the next occurence of "stop" after "start". If
"last - first + 1" evaluates to < 0, then len is going to be a huge
number (one of those implications of unsigned I mentioned).
> // get the substring of srt, starting at position first (delimeter +1)
>and
> // go for len charachers. If len is larger than the string srt it
>will
> // just stop at the end.
>
More obvious comments - it doesn't take much programming experience to
see what the code does.
> std::string word = srt.substr(first, len);
>
Again, "word" is a misleading name. To repeat, what happens if first ==
npos?
> // now that we got the word(s) we wanted out between the delimiters, it
>is
> // time to remove that section so that we can work with the next part, if
>
> // any.
>
The comment is superfluous (and optimistic - many things could have gone
wrong).
> srt = srt.substr(last + 2, srt.length());
> return word;
> }
>
>// the name says it all
>void search_and_replace(std::string &text, std::string &srt,
> unsigned int drc, const bool debug) {
>
Don't make a debug flag part of the function signature - it's got
nothing to do with the function's purpose and removing it before
releasing the code is going to be a pain. Also, marking a value
parameter "const" is a waste of time.
> // I didn't want accidental spaces to cause the program to glitch
> // so in the next few lines I remove all spaces before delimiters.
> // This also simplfied getting the delimiters
> unsigned int eatwhite = srt.find_first_not_of(' ');
>
What if someone uses tab characters to separate the bits?
> srt = srt.substr(eatwhite,srt.length());
> char start = srt[0]; // the starting delimiter is now the first char in
>srt
>
Error condition not checked - what if srt.length() == 0? Undefined
behaviour can result from this.
> srt = srt.substr(1,srt.length());
>
Error condition not checked - what if srt.length() < 2?
> eatwhite = srt.find_first_not_of(' ');
> srt = srt.substr(eatwhite,srt.length());
>
What if eatwhite == npos?
> char stop = srt[0]; // the ending delimiter is now the first char in
>srt
>
What if srt.length() == 0?
> if (debug) {
> std::cout << "\nStart Delimiter = " << start;
> std::cout << "\nStop Delimiter = " << stop;
> }
> // remove the delimiter specifiers so we can look for the search
> // and replace words
> srt = srt.substr(2, srt.length()-1);
> // get the word(s) for the search part
> std::string search = getword_s(start, stop, srt);
> if (debug) std::cout << "\n";
> // get the word(s) for the replace part
> std::string replace = getword_s(start, stop, srt);
> if (debug) {
> std::cout << "\nThe word(s) to search for is/are : \"" << search <<
>"\"";
> std::cout << "\nThe word(s) to replace with is/are: \"" << replace <<
>"\"";
> std::cout << "\n";
> }
> // pos and new_pos are needed to prevent recursive replacements of
>text
> // resulting in a "loop until crash condition"
>
Actually, this comment doesn't tell me much. Why might there be a
recursive replacement going on? How do they prevent it? As someone with
20+ years programming experience, I wouldn't have associated recursion
as a problem with search and replace - there's simply a certain
condition that one has to avoid. If the code is well-written, that
avoidance will be obvious. By spuriously mentioning preventative
measures like this, you cloud the preconceptions of someone reading the
code - I'm now looking for possible recursion, rather than seeing if the
code does what it supposed to do
> unsigned int pos = 0;
> unsigned int new_pos = 0;
> unsigned int arc = 0;
> // Part one of while (before the "and")
> // Need to check to see that what your searching for is there before
> // you try to replace it. If it is not there you can't replace it,
>and
> // your program will terminate before you can view any std::cout
>data.
>
This comment is obvious and unnecessary.
> // Part two of while (after the "and")
> // While advancing through the text making replacments continue if
> // the desired number of replacements hasn't yet been made or
> // "replace all" if desired replacement count equals zero.
>
Also obvious and unnecessary
> while (text.length() > text.find(search,pos) and ((arc < drc) or (drc
>== 0))) {
>
Backing up what I said about using the long names in code: The expression
(actual_replacement_count < desired_replacement_count) or
(desired_replacement_count == 0)
would have saved me having to scroll back up the code before I could
figure out if it did what it said. It also doesn't need extra
commentary, since it's obvious from the code what it's doing. In fact,
if you set desired_replacement_count to MAX_INT (when it was zero)
before getting here, you wouldn't need the second half of the expression.
Also, although "and" and "or" are legal C++, it's usually better to
stick to && and ||, since that's what most people expect to see. What
would you think of this:
while (condition)
??<
// code
??<
(also legal).
> // If the search word is not found, it cannot be replaced!!
>
Again, obvious and unnecessary.
> // mark the place of where the next new position will be
> new_pos = text.find(search,pos) + replace.length()-1;
> // make the string replacement
>
Two more comments that add nothing to what the code says.
> text = text.replace(text.find(search,pos), search.length(),
>replace);
> // go to the new position (after the string replacement) to prevent
> // a recursive string replacement (and subsequent crash)
>
And here we get to it. Well of course you do this. It's obvious that you
need to move on past the bit you've just done - it's not worth remarking
on. And it's not recursive - it's just that it could fail to terminate.
> pos = new_pos;
> arc++; // advance actual replacement counter.
>
Yes, I know that's what "++" does.
> } // end of while
> std::cout << "\nActual Replacement Count = " << arc;
> } // end of function
>
>int main(void) {
> bool debug = true; // toggle to see more details
> std::string original = "I will be 40 40 40 40 40 next year.";
> std::string new_text = original;
> // Oh yeah - here I am experimenting with vectors of strings
> // Vectors are almost like arrays, but better
> std::vector<std::string>srt(6); // srt - search and replace text
> std::vector<unsigned int>drc(6);
>
> // Test no match condition
> srt[0] = " < > <35> <34>";
> drc[0] = 4;
> // Test replace with same size
> srt[1] = " ( ) (40) (38)";
> drc[1] = 3;
> // Test replace with smaller size
> srt[2] = "[ ] [ 38] []";
> drc[2] = 0;
> // Test replace with a bigger text
> srt[3] = "[] [next] [a very old man next]";
> drc[3] = 3;
> // Test replace
> srt[4] = "[] [40] []";
> drc[4] = 2;
> // Test replace
> srt[5] = "[] [ ] [ ]";
> drc[5] = 1;
>
> for(int i = 0; i < 6; i++) {
> std::cout << "\n\n\n\n\n\n\n\n\n\n\n\n\nSearch and Replace Text = "
><< srt[i];
> if (drc[i] == 0) std::cout << "\nDesired Replacement Count = All";
> else std::cout << "\nDesired Replacement Count = " << drc[i];
> search_and_replace(new_text, srt[i], drc[i], debug);
> std::cout << "\nOriginal text: = " << original;
> std::cout << "\nNew text: = " << new_text;
> std::cout << "\n\n";
> system("pause");
> original = new_text;
> }
> return 0;
>}
>// Praise God for His Goodness and Mercy
>// Jay A. Carlson
>//
>// Constuctive criticism will be appreciated.
>
>
>-------------------------------------------------------
>This SF.Net email is sponsored by: INetU
>Attention Web Developers & Consultants: Become An INetU Hosting Partner.
>Refer Dedicated Servers. We Manage Them. You Get 10% Monthly Commission!
>INetU Dedicated Managed Hosting http://www.inetu.net/partner/index.php
>_______________________________________________
>Dev-cpp-users mailing list
>Dev...@li...
>TO UNSUBSCRIBE: http://www.noicys.cjb.net/devcpp/ub.htm
>https://lists.sourceforge.net/lists/listinfo/dev-cpp-users
>
>
>
|
