[Freeciv-Dev] Re: Cleaning up gotohand.c (PR#1068)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Nov 30, 2001 at 08:01:33PM -0500, Ross W. Wetmore wrote:
> At 06:19 PM 01/11/30 +0100, Raimar Falke wrote:
> [...]
> >> If you have a better idea as to why an IGTER move costs SINGLE_MOVE and
> >> not MOVE_COST_ROAD on the warmap, please enlighten us.
> >
> >I have no answer. This and some other places where I only could say
> >"this is an example of a way to not do this" let me conclude that I
> >should write my own goto code.
> 
> Almost always a bad mistake. If you can't understand the original you
> probably shouldn't be writing a new version of your own. 

Why? I see it as a learning. So it would work like this (in an ideal
way):

 1) I make a new version
 2) I compare the behavior of old and new
 3) the old version is better in some aspect
 4) I find the reason in the old code (I understand a bit more of the
 code)
 5) I add this feature to the new version (documented)
 6) goto 2)

> >People (all of you) take a step back. Do you really think that you
> >transform this (primary gotohand.c) into something which will be a
> >pleasure to read? Isn't your effort better spend in encapsulation the
> >interface of gotohand.c and write a second implementation learning
> >from the first version.
> 
> Only when it is understood well enough, which is not the case yet, I 
> think. Your comment may become valid in time.
> 

> Spending some time bugfixing things is a very good learning exercise.

Ack. The question is for what? For code deciphering? Or for example
how to not do this? I think there is a point somewhere where the
rewrite is cheaper (and leads to better architectured code) than the
cleanup of the old one.

> Your more mature contributors will do this rather than submit nifty
> new things out of the blue.
> 
> <ASIDE>
> I have spent a lot of time changing "broken" code in test versions you
> have never seen, only to find out a short while later that the original
> was in fact correct or required to deal with a situation in a completely
> different area of the code. Sometimes I even learned subtle things that
> were not immediately obvious. Freeciv is heap code with much of the garbage
> thrown on it years old, but some of it isn't bad, just overgrown and
> difficult to fully comprehend all at once ;-).
> 
> This does not happen all the time, but it happens enough.
> 
> On the reverse side, I have seen "fixes" and "rewrites" go in that are 
> obviously more ego driven than good for the code. The justifications that 
> state "It is obvious that ..." or "the natural way ..." usually indicate
> the worst offenders.
> </ASIDE>
> 
> Many of these cleanups are useful in two ways. They expose code for 
> review and comment and help promote understanding. Changes to the game
> behaviour after the fact can be used to go back and revisit the changes
> if needed. This does not remove responsibility for doing a good job of 
> the original patch review, but no one or two people are liable to really
> test all cases.


> Note a good patch review means that at least two different people have
> done some serious looking at the patch, and there are no serious issues
> raised or left outstanding in a resubmit. If this is the case, someone
> had better have a very good reason to block it or subsequently give the
> patch a hard time by reopening the review phase.
> 
> Once it is clear, either from discussion or the number of patches, that
> there is a better way, *then* is the time to reimplement. But the first
> reimplementation will almost always be inferior to the original, especially
> if it is a large or complex subsystem/routine, and even if its long term 
> potential is greater.
> 
> Some rules to help with this patch ...
> 
> Note, no patch should ever be blocked on the basis of a single person's
> aesthetic view of the commentary or english. Or for anything but gross
> violations of the coding standard in the surrounding code. Suggestion
> but not exclusion here is absolutely required. Aggressive nitpicking is 
> childish anti-social behaviour. 
> 
> One can always submit followup patches to fix non-technical issues if 
> they really bother one with such followups going through the same review 
> cycle. Over time the general codebase will move to the norm that style 
> guidelines suggest - a few deviations along the way won't be noticed. 
> 
> Forcing one's personal style unduly, or using it as an unfair technique 
> to kill patches from people, or in areas of the code you are not happy 
> with seeing development is highly unprofessional. This applies to 
> reviewers as well as maintainers, and maybe moreso to reviewers and 
> competing submitters in the heat of the discussion phase.
> 
> And Raimar, if maintainers don't want to accept large, comprehensive but 
> well tested patches, or the list doesn't come up with the bodies to 
> adequately review them, but instead cry for little ones, then one should 
> accept the little ones and not cry for a large rewrite when little ones
> come in or come in in large numbers :-).

Mostly ack. We need a style guide. And more maintainers and more
active reviewers (just people who participate).

> As a general suggestion ...
> 

> When a patch comes in that needs a thorough review, it should be farmed 
> out to a primary reviewer with one or two others who are interested or 
> knowledgable helping out. These people should work with the submitter to 
> get it into reasonable state, and send back a summary report recommending 
> inclusion or not (but quick bugfixes can still be dealt with directly).

Ack. This all can be done without interference of the maintainers.

> The responsibility for insuring it has been looked at, is consistent
> with the project goals, etc, but not necessarily of doing all the testing 
> leg work effort, and finally of checking it into CVS is what a smaller
> group of maintainers should be doing.

Ack.

> If no one is interested in reviewing something, that is a review report 
> in itself :-).

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "brand memory are for windows users that think their stability
   problems come from the memory"
    -- bomek in #freeciv