[Freeciv-Dev] Re: [Patch] Cleanup regarding {city,unit}_owner

[Trent Piepho <xyzzy@xxxxxxxxxxxxx>]

On Thu, 23 Aug 2001, Raimar Falke wrote:
> The first patch changes constructs like "get_player(punit->owner)" to
> "unit_owner(punit)". The second patch changes constructs like
> "&game.players[punit->owner]" also to "unit_owner(punit)".

It looks like you changed city owner in the same way as unit owner, which
makes sense.

But, why not just make the owner field a player pointer?  Then you could
change:

&game.players[punit->owner]
or
get_player(punit->owner)

to

punit->owner

More efficient, less code, less typing.  And if you want to remove IDs where
they aren't needed, you're going to want to change the owner field to a
pointer anyway.

Now, I'm guessing you might say that unit_owner() can just be changed to
#define UNIT_OWNER(punit) (punit->owner)

But, it is generally considered neither necessary, nor desirable, in C
programming to create accessor functions for members of structures.  Accessor
functions like:

struct player *get_player(int playerid);
struct unit *get_unit_by_id(int id);

Are ok, because you don't know underlying data structure that player or unit
structs are contained in.  So while get_player() may just be a one-expression
array index, that could change.  get_unit in fact has changed, it used to be
that it searched an array of lists for the unit, but that was changed to a
hash table for speed.  I think it was Syela who first did that back when he
wrote the AI.  But once you have the structure in hand, it is ok to just
access its fields with ->.  It would be bad to get into a situation where 95%
of structure members are accessed with ->, and 5% have acessor functions for
no apparent reason.