I have no particular interest in this programming challenge,
though I do find some of the commentary amusing. I thought it
might be of interest to do a code review. The following is not
an endorsement or a condemnation of the code. Rather it is an
informal account of code features that are problematic.
Notes on the Nilges code:
(1) At no point is there a comprehensive specification. A fair
number of corner cases are left up in the air. These include
overlapping targets, and empty targets. There is no statement
of error handling strategy.
If you mean in the code, this is FALSE. These issues were handled and
they were discussed. RTFT
And since when was a "comprehensive specification" required in the
documentation? No other code even comes close to the utility and
completeness of the comments in this code. Sure, they could be
improved: but do note that any actual improvements would make the
documentation less "readable" using the demotic (and quite ignorant)
definition of "readability" used by many posters here.
(2) There are instances of failure to protect against
dereferencing null pointers. Thus in function strLength the for
loop will dereference a null pointer if argument strInstring is
a null pointer. Similarly function replace checks strTarget but
not strMaster. ptrIndex1 is initialized to strMaster. The
while loop, while(*ptrIndex1), will dereference a null pointer
if strMaster is null.
This is correct. But note that you're subjecting the code to a more
exacting standard that any other competitor meets, or for that matter
99% of C programs (which typically will fail if passed a null
function), out of personal malice, animosity, and envy...of the fact
that I can do two things seldom mastered simultaneously: write like a
humanist and code better than you.
Seriously, how many times have you checked for a NULL parameter
explicitly?
This issue is fabricated. Even quality library programs don't compare
to NULL; instead, a failure occurs inside the library code. It's
arguably a good practice, but its implementation in my code needs a
more general strategy for handling errors.
Your goal is "destroy Nilges, because he can do our job and the guy
doesn't even know C! He can also write a complex sentence! Nilges must
die!"
The test cases do not check for null arguments nor all instances
of empty strings.
Twenty lashes with a wet noodle.
Clown.
OK, so code them in the idioms supported and post them, and I will add
them. What part of collaboration don't you understand?
(3) The TESTER macro is cumbersome and should be replaced by a
function.
It is no more cumbersome than the lumber in your brain. I'm tired of
little coding monkeys who cover up their lack of energy with big words
used unconsciously-metaphorically.
However, it could I suppose be a function as I scale up.
(4) It would be better if the test cases were moved to a
separate file - the plan would be for the main function to read
the test cases from from the test cases file and then compare
the actual test results with the expected test results.
The test software shouldn't become the main focus. It should be
simple, otherwise we get into an infinite regress.
(5) The comments are inadequate. The existing comments read
more like coding notes written on the spur of the moment to
remind the coder about what was going on. There is no statement
of the algorithm being used; the comments often reference
concepts that are not explained. For example, there is no
precise definition of segment nor of handle.
If you mean, in "read like coding notes", the change record, that is
what they are. I don't cover up my bugs, nor grin about them like
Seebie. As for the remainder, I mean to generate software by writing
first, coding later, and providing an audit trail. Of course, the
comments could and should be cleaned up were I to publish the code,
but again: you invent ad-hoc criteria, meant to be applied ONLY to
this code, out of adversarial hatred, envy and malice. In a court of
law, you'd be in contempt.
(6) The commenting format is not well chosen. The comments
break into the code and vice versa.
Incoherent.
(7) The code author uses a bizarre, idiosyncratic, and long
winded variant of Hungarian notation. It's his privilege.
Damn straight. I am the King of Hungary.
Unfortunately the long names used are not informative. For
example, the ptrIndex* variables. What on earth are they, and
what are their invariants? The only way to know what they are
is play computer, walk through the code, and reconstruct what
the coder's intent was.
No, it just needs a read of the code. ptr is a pointer MEANT TO BE
MODIFIED, as opposed to str, which is a pointer to a char meant not to
be modified. Some C programmers in these threads would suggest const
for the latter, but others have discouraged its use.
You are in fact conducting the sort of "structured walkthru" that in
my experience appeared at Bell Northern Research after the election of
Reagan, one in which issues were fabricated ad-hoc out of hatred, envy
and malice because Northern Telecom executives (some of whom later
went to jail) were forcing different BNR sites to compete with each
other so they could get fat bonuses and live like swine...while former
members of the scientific staff at Mountain View die in motels without
health insurance.
I've already explained how easy it is for a competent tech, using a
modern era (or even emacs) to globally scan/replace ptrIndex1 to
index1, or whatever he wants. If I were coding in an office that
mandated in its standard manual, or as an oral rule, either "no
Hungarian" or "Szymonyi Hungarian" as opposed to my standard (which
happens to have been invented inside IBM when Charles Szymonyi was
failing to adjust to socialism, and which was stolen and modified by
him), I would use the office standard.
What part of "red herring" don't you understand?
Then there are the ptrSegmentStruct variables. One has a suffix
of Starts, one of Prev, and one is without a suffix. Since the
Wow. His shit's all fucked up.
key idea of the implementation is to generate a linked list of
segments, each (except possibly the last) being terminated by
OK, so far.
the target string, it turns out that the ptrSegmentStructStarts
refers to the list head, ptrSegmentStruct refers to the current
pointer into the list, and ptrSegmentStructPrev refers to the
previous pointer into the list. Or perhaps not.
Or perhaps not (and screw your irony). It's a perfectly meaningful
triad of names. We need to know the first entry (Starts) to assemble
the output string. We need to walk the structure with the unsuffixed
name. And the Prev pointer, which is inactive and unused after the
scan for target loop, is initialized correctly to zero so that the
append can be written properly:
if (ptrSegmentStructPrev != 0)
ptrSegmentStructPrev->ptrNext = ptrSegmentStruct;
else
ptrSegmentStructStarts = ptrSegmentStruct;
The use of Prev is perfectly guarded in the above. Do you in fact know
how to construct a linked list (Heathfield can't)?
At some point in the distant past, Harter, you were able to write
competent code, for I saw an old 1990 program of yours that was
competent. I think you are now some sort of manager who is today
unable, unlike me, to crank this level of code in the time I
took...about two hours for the first version, and a total of 8..10
hours thereafter. I conclude that you are hand-waving and using
corporatese out of hatred, envy and malice. What's disturbing is that
the appearance of hatred, envy and malice in code reviews first
appeared in my experience on the job after Reagan was elected, and
companies made programmers into perpetual infants, always candidates
for their posts, whereas today, people air their hatred, envy and
malice recreationally and sadistically.
(8) The scheme of allocating space for each segment is mildly
inefficient, but is much better than reallocing space for the
output string. In any case it is doubtful that execution
efficiency is among the objectives of this exercise.
You are so full of hatred, envy and malice that you cannot be in the
slightest gracious about anything, can you. Instead, you fall into
neutral corporate prose.
At least you can spell "efficient".
Given the objectives, the only "inefficiency" was fixed this morning.
This was the generation of zero length segments.
As you seem to know, a realloc would involve to many memory copies,
unbounded apriori. Whereas the assembly of the final string takes M
mallocs for the segments (where M == the number of targets that are
not immediately preceded by > 1 targets) and one malloc for the final
string, and minimal motion of bytes to the new string.
(9) In some places the code is overly complex. A case in point
is:
lngNewLength += ptrSegmentStruct->lngSegmentLength +
(ptrIndex2 && !*ptrIndex2
?
lngReplacementLength
:
0);
There are several other instances of unnecessary complexity.
What you mean is that there are no longer any programmers in fact
considerate enough of their intelligent readers to take the trouble to
so format code under the (rather severe) 67 character line limit
imposed by the newsreaders. Two-dimensional formatting in fact is a
great way to exhibit operator precedence.
It is true that one of these statements is munged in the current
edition of the code (and only one), but a competent technician knows
how to fix this based on the consistency of the style, a consistency
that appears to apes as unnecessary complexity.
You see, dear chap, what you mean by "unnecessary complexity" is in
fact any evidence of thought or work. Because data processing has
become a pure cost center, executives are troubled by evidence of
thought or work on the part of which has become a pure eunuch class,
charged with pure software maintenance and the chanting of hymns and
anthems to capitalism.
The updating of the never defined Index variables is a case in
point.
(10) Only one of strNew, strNewStart is needed.
You don't know what you're talking about, and you have not diligently
or adequately studied the code. strNew is needed to index through the
new string, whereas we need to return strNewStart.
"Never defined Index variables" is gibberish.
In fine, although I expected better from you, you've Swift Boated the
code, creating a pompous document that will be used by subsequent
posters as "evidence". It's the same sort of document that Seebie used
to swift boat Schildt.
What a jerk!
(11) In conclusion, it is difficult to determine whether the
code works correctly because the design is not spelled out, the
variables are not properly documention, the testing framework is
inadequate, and the baroque nomeclature and coding complicate
analysis.
Pompous, meaningless bullshit. For example, most corporate types who
use the word "baroque" couldn't identify Baroque music or art in a
simple test. Here, it means "my God, someone is thinking originally.
Kill him!"
