[Freeciv-Dev] Re: (PR#8754) effects patch

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8754 >

Vasco Alexandre da Silva Costa wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=8754 >
> 
> On Sun, 18 Jul 2004, Mike Kaufman wrote:
> 
>><URL: http://rt.freeciv.org/Ticket/Display.html?id=8754 >
>>
>>here are a couple of things. by no means exhaustive...
>>
>>all functions in effects.c should have comments in the their headers.
> 
> Uh, this isn't true for the rest of the code, I don't see why the effects
> code should be special in this regard.

It is supposed to be true for the rest of the code.  See the "comments" 
section in doc/CodingStyle.  The fact that you people (tm) keep putting 
in new functions without headers doesn't change this.

Although, as long as the patch is in a state of flux it may be a waste 
of time to write headers for functions that may be changed.  But they 
need to be written before committing.

>>rename get_effect_buildings() -> get_buildings_with_effect()
>>rename get_building_effect() -> get_effects_of_building()
>>rename get_building_effects() -> get_effect_types_of_building()
> 
> 
> This seems a bit verbose... How about:
> get_buildings_with_effect()
> get_building_effects()
> get_building_effect_types()
> 
> ?

We need to decide whether to use "building" or "improvement".  The 
current standard is "improvement" so it should be 
get_improvements_with_effect() or get_imprs_with_effect().

>>you should be able to make effect.value a short with no problems.
> 
> This could be done, but is it worth it? Assuming we have 400 effects, this
> is 800 bytes overhead, less than 1 KB.

Probably slower, too.  And why should this value be a short when most 
integers are integers?

>>I don't like the term "power" in source_power(), etc. Use "value" instead.
> 
> Ok.

source_value() is not descriptive enough IMO (not that source_power() is 
either).  I'd suggest something better but I have no idea what this 
function does.

jason