[Freeciv-Dev] Re: [IAMBACK] [PATCH] [1.3] cleanup of proccess_*_want() (PR#1295)

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

<snip>
> I just happen to absolutely love your poetic language and brilliant
> attributions.
> 
> As a meed, you've a TODO there now! :)

Good to hear someone appreciates my attempt at making swearing more creative.

> -  pcity->ai.a = val2;
> -  pcity->ai.f = val3;
> +  pcity->ai.attack = val2;
> +  pcity->ai.bcost = val3;
> 
> Crap names.
> 
> Hm? Tell me better.
> 
> I got nothing. I don't have to be creative. I'm a critic. It's my job to
> mock, yours to do better.
> 
> Well, then what about being concrete when mocking? :^)

I still don't like the existing names, but I've got no better ideas.

> You won't give my poor soul a rest..

What are we paying you the big money for? *Tongue in cheek*.
 

> > Index: ChangeLog

<snip>
>  /************************************************************************** 
> -...
> +What would be the best defender for that city?
>  **************************************************************************/
>  static void process_defender_want(struct player *pplayer, struct city
> *pcity,
> -                               int danger, struct ai_choice *choice)
> +                                  int danger, struct ai_choice *choice)
>  {
<snip>

{
> +        /* We first need to develop a tech required by the unit... */
> +
> +        /* Cost (shield equivalent) of gaining these techs. */
> +        /* FIXME? Katvrr advises that this should be weighted more heavily
> in
> +         * big danger. */
> +        int tech_cost = total_bulbs_required_for_goal(pplayer,
> +                          unit_types[unit_type].tech_requirement) / 4
> +                        / city_list_size(&pplayer->cities);
> +

This was great. I like the use of tech_cost. A big improvement in legibility.
        

> -static void process_attacker_want(struct player *pplayer,
> -                         struct city *pcity, int b, Unit_Type_id n,
> -                            bool vet, int x, int y, bool unhap, int *e0, int
> *v,
> -                            int bx, int by, int boatspeed, int needferry)
<snip>
> +static void process_attacker_want(struct player *pplayer, struct city
> *pcity,
> +                                  int value, Unit_Type_id victim_unit_type,
> +                                  bool veteran, int x, int y, bool unhap,
> +                                  int *best_value, int *best_choice,
> +                                  int boatx, int boaty, int boatspeed,
> +                                  int needferry)
> +{ 

You really like long parameter lists don't you? I'm not sure this is an
improvement on the original. It's absurdly long. I'll help you out by telling
you how to get rid of boatspeed. In the short run, you can use
unit_move_rate(punit) for a quick fix. In the long run, you can look at my
patches for f_s_t_k and how I use unit_move_turns to fix the problem.

<snip>
> +        } else {
> +          /* Cost for attacking the victim. */
> +          /* FIXME? Why we should multiply the cost by move rate?! --pasky
> */
> +          move_time = (warmap.cost[x][y] * victim_move_rate + move_rate - 1)
> +                      / move_rate;
> +        }
> +

No idea. If someone has an explanation for this bit of code, feel free.
     
> +      
> +      want = military_amortize(desire, MAX(1, move_time),
> +                               bcost_balanced + needferry);
> +

Much better want equation. I'd want this patch to go into CVS for this reason
alone.


__________________________________________________
Do You Yahoo!?
Yahoo! Tax Center - online filing with TurboTax
http://taxes.yahoo.com/