[Freeciv-Dev] (PR#4420) New traderoute rules

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

[bursig - Fri Jul 18 09:09:46 2003]:

> Dnia 2003.07.18 06:28, Jason Short napisa³(a):
> 
> > - Why do so many checks do
> > 
> >   can_cities_trade(...) && can_establish_trade_route(...)
> > 
> > ?  If can_establish_trade_route returns TRUE isn't that all you need
> > to know?  It should be...
> > 
> No, becouse I split existing can_establish_trade_route to 2 parts...
> 1) check ownership and distance of cities
> 
>            (pc1 && pc2 && (pc1 != pc2) && (pc1->owner != pc2->owner
>          || map_distance(pc1->x, pc1->y, pc2->x, pc2->y) > 8));
> 
> 2) check exist of traderoutes ( I add check for replace abbility )
> 
> current both checks are make in can_establish_trade_route, I remove 1) 
> from this function to avoid duplicate of check.
> TRUE in first check give us information that we can trade and we can 
> try check traderoute ability.
> 
> Trade ability and Traderoute ability are different things but secound 
> require TRUE in first. Basicaly Traderoute ability is extension of 
> Trade ability.

I understand.  I am saying that can_establish_trade_route() should only
return TRUE if both things test true - it shouldn't just check the
second and rely on the caller to test the first.  Doing it your way may
save a few cycles (not much!) but is less readible and more error-prone.

> > - get_caravan_enter_city_trade_bonus should be cleaned up; there are 
> > a lot of old worthless comments left in.
> > 
> be my guest

OK.

> > - have_cities_trade_route has an odd implementation.  Why do you 
> > check both cities values if it doesn't matter?
> > 
> no it does matter...
> this can speed up exit from check loop when existing traderoute is foud 
> in secound city

Or it can slow down the exit if the existing traderoute isn't found in
either city...but whatever.

> Here is new version with get_city_min_trade_route clean.

Much better.

jason