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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

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. 

>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.
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 :-).


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).

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.

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

[...]
>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>  "With a PC, I always felt limited by the software available.
>   On Unix, I am limited by my knowledge."
>    -- Peter J. Schoenster <pschon@xxxxxxxxxxxxxxxxx>

Cheers,
RossW
=====