[Freeciv-Dev] Review of (PR#4659)

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

Cameron,

Here is the reveiw of your patch (not by me).

> ==========================================================================
> 
> I. Testing
> 
> 1. Was the patch tested?
>    (yes / no;  if no, why)

Yes.  

 
> 2. Which clients were used to test the patch?

gui-gtk-2.0, but as this is a map gen patch, this shouldn't make a difference.

> 3. Does the patch work / do what it promises to? 
>    (yes / no;  if no, what in particular fails)

Yes.  I ran the game with the suggested parameters and waited long
enough for the explorers to finish the islands and checked that land
area was roughly the same (side note: nice work on the endgame report,
saved me having to eyeball the land area size).  The land areas varied
from 61000 to 58000 which is 5% difference.  It also looked roughly the
same.  So the patch seems to work.


> ==========================================================================
> 
> II. Code correctness
> 
> 1. Does the patch adhere to the style guide, 
>    see http://www.freeciv.org/lxr/source/doc/CodingStyle? 
>    (yes / no;  if no, list some of the diff file line numbers)

Yes.  
 
> 2. Is the patch well commented / easy to read?
>    (yes / no;  if no, what parts are confusing)

The patch seemed easy to read.  No comment added for new function 
map_clear_all_specials

> 3. Are there coding errors in the patch? 
>    (list)

It sets min_specific_island_size to 0 for generators 3 and 4.  Originally
the code required that these be at least 10% of the size that was requested.
The 10% comes from:
-   while (!create_island(i--, pstate) && i * 10 > islemass) {
The new 0% comes from:
+                 pstate, 0);
and related.  This should be ... pstate, 10);... to preserve existing 
gen 3 and gen 4 behavior.

> 4. What can be improved / simplified / done differently? 
>    (list)

Can't think of anything.

> ==========================================================================
> 
> III. Comments
> 
> 1. Any other comments you wish to make.

It might be worthwhile to consider extending this patch to gen 3 and gen 4.
(Of course, those generators are used less than gen 2 so it doesn't matter
as much).

> ==========================================================================
> 
> IV. Verdict
> 
> 1. Should the patch be
>   a) rejected completely
>   b) split - elaborate
>   c) revised and resubmitted for review
>   d) revised and then committed
>   e) committed as it is

d) Revised and then committed.  

The patch should be changed so that it tries to make the primary 
islands in gen 3 and gen 4 at least 10% as that was the previous 
behavior, or a good justification changing the behavior needs to be made.

There should be a comment for map_clear_all_specials.