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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

[bursig - Thu Jul 17 22:16:07 2003]:

> Dnia 2003.07.17 20:15, Jason Short napisa³(a):
> 
> > I think can_enter_marketplace is an unhelpful name.  Perhaps
> > can_cities_trade would be better (not to be confused with
> > can_establish_trade_route - what you call "entering the marketplace"
> > is basically "trade" as opposed to a "trade route")?
> > 
> Done
> 
> > +  if (pcity_homecity->owner == pcity_dest->owner
> > +         && map_distance(pcity_homecity->x, pcity_homecity->y,
> > +                                 pcity_dest->x, pcity_dest->y) <= 8)
> > {
> > +    return FALSE;
> > +  }
> > 
> > The above code should just call can_enter_marketplace, right?
> > 
> fixed
> 
> > I also have the problem that the patch doesn't compile; see
> > unihand.c:1211.
> This is my fault becouse I send wrong version of this patch (trade3 
> insted trade4). Here is lastest version with your advices.

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

- can_establish_trade_route should have a helper function
get_city_min_trade_route().

  int get_city_min_trade_route(struct city *pcity)
  {
    int slot = 0, value = pcity->trade_value[0];
    if (city_num_trade_routes(pc1) < NUM_TRADEROUTES) return 0;

    /* find min */
    for (i = 1; i < NUM_TRADEROUTES; i++) {
      if (value > pc1->trade_value[i]) {
        value = pc1->trade_value[i];
      }
    }

    return value;
  }

thus making the function much simpler.  And why is the unused variable
'slot' in there?

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

- have_cities_trade_route has an odd implementation.  Why do you check
both cities values if it doesn't matter?

- can_unit_est_traderoute_here becomes a misnomer; it should be
can_unit_trade_here.

- remove_smallest_trade_route could also use the
get_city_min_trade_route function, but it would need an extra variable
(it does need to know the 'slot').  Hmm, probably not worth it...

- The logic of the unithand.c changes is lengthy; I haven't examined it
closely.

jason