[Freeciv-Dev] Re: advdomestic.c cleanup II. (PR#1157)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Fri, Dec 28, 2001 at 10:36:42PM CET, I got a letter,
where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
> >  void domestic_advisor_choose_build(struct player *pplayer, struct city
> > *pcity,
> >                                struct ai_choice *choice)
> >  {
> > -  struct government *g = get_gov_pplayer(pplayer);
> > -  int con, want, dw;
> > -  Unit_Type_id utid, iunit;
> > -  struct ai_choice cur;
> > -  int est_food = pcity->food_surplus + 2 * pcity->ppl_scientist + 2 *
> > pcity->ppl_taxman; 
> > -  int vans = 0;
> > -/* had to add the scientist guess here too -- Syela */
> > +  /* Government of the player */
> > +  struct government *gov = get_gov_pplayer(pplayer);
> > +  /* Unit type with certain role */
> > +  Unit_Type_id unit_type;
> > +  /* Food surplus assuming that workers and elvii are already
>                                                ^^^^^
> wow, a nice plural form!
It's nice, that means i didn't invented that, but Ross suggested it ;).

> >    choice->choice = 0;
> >    choice->want   = 0;
> >    choice->type   = CT_NONE;
> 
> consider introducing choice_init function, a la Ross' corecleanup patch
Nice idea... however I will do it in separate patch probably doint only this.

> > +  } unit_list_iterate_end;
> > +  city_list_iterate(pplayer->cities, acity) {
> >      if (acity->is_building_unit && acity->shield_stock >= 50 &&
> 
> use build_cost instead of 50
Good Idea (tm)

> Any general comment on the below code?
> 
> > +  city_list_iterate(pplayer->cities, acity) {
> > +    if (!acity->is_building_unit &&
> > +   is_wonder(acity->currently_building) &&
> > +        map_get_continent(acity->x, acity->y) == continent &&
> > +        acity != pcity &&
> > +   build_points_left(acity) > 50 * caravans) {
> > +      /* Desire for this building */
Well, I thought it's clear, but ok :).

> why taking local want for something built elsewhere?
Ok.

> > +    ai_advisor_choose_building(pcity, &cur);
> > +    if (cur.want > choice->want) {
> > +      choice->choice = cur.choice;
> > +      choice->want = cur.want;
> 
> use copy_if_better_choice 
Ok.

> > +      /* Allowing buy of peaceful units after much testing. -- Syela
> > */
> > +      /* want > 100 means BUY RIGHT NOW */
> > +      /* if (choice->want > 100) choice->want = 100; */
> > +      choice->type = CT_BUILDING;
..? This belongs to what?

What about abiding the "comments before code" rule, please ? ;)

Attached new version of the patch.

Thanks for review,

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
Firewall in a way is like the doorkeeper of a local pub. If you don't have your
I.D. on you, or if for some reason you do not qualify to enter, the doorkeeper
will not permit you to enter. In some extreme cases these doorkeepers will not
let you out, or at least give you a hard time before they finally let you out.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/

advdomestic.c.diff
Description: Text document