[Freeciv-Dev] Re: [Patch] Make get_city_*_bonus public

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, May 04, 2002 at 04:28:45AM -0700, Raahul Kumar wrote:
> 
> --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > The changes which are required for the dynamic CMA patch as a seperate
> > one. It makes the three methods public, adds some comments and cleans
> > them up.
> 
> It's a good clean patch. I've got a few questions:
> 
> 
> >  /**************************************************************************
> > -...
> > + Return the factor (in %) by which the shield should be multiplied.
> >  **************************************************************************/
> 
> I would prefer:
> 
> Return the %(> 100) by which the city's base shields should be multiplied. 

I can't and wouldn't ensure that the factor is always >=100. It may be
quite possible that there is a building which increase the shield
output by 50% and reduce the science output by 50%.

> > +int get_city_shield_bonus(struct city *pcity)
> >  {
> > -  int tmp = 0;
> > +  int shield_bonus = 100;
> > +
> >    if (city_got_building(pcity, B_FACTORY)) {
> > -    if (city_got_building(pcity, B_MFG))
> > -      tmp = 100;
> > -    else
> > -      tmp = 50;
> > +    shield_bonus += 50;
> > +    if (city_got_building(pcity, B_MFG)) {
> > +      shield_bonus += 50;
> > +    }
> 
> What happens if a city sells the factory, and still has a mfg plant?

It doesn't get any bonus.

> Substitute in bank/stock exchange/university/research center for
> factory. It seems the freeciv manual is wrong about the behaviour. I
> always thought that even after selling a marketplace your bank would
> be bringing in money.

This isn't a change my patch made.

> >      if (city_affected_by_wonder(pcity, B_HOOVER) ||
> >     city_got_building(pcity, B_POWER) ||
> >     city_got_building(pcity, B_HYDRO) ||
> >     city_got_building(pcity, B_NUCLEAR)) {
> > -      tmp = (3 * tmp) / 2;
> > +      shield_bonus = 100 + (3 * (shield_bonus - 100)) / 2;
> >      }
> >    }
> >  
> > -  pcity->shield_bonus = tmp + 100; <-
> 
> What's happening above? This line seems fishy to me. I realise you got rid of
> it, but what was it doing originally?

tmp in the old version was "shield_bonus - 100". The assignment of tmp
can easily be replaced with shield_bonus. However in the above
multiplication some not so nice code is necessary.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  What's nice about GUI is that you see what you manipulate.
  What's bad about GUI is that you can only manipulate what you see.