[Freeciv-Dev] Re: teamslite9

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Thu, 24 Oct 2002, Raimar Falke wrote:
> Noise.

Well, it fixes stuff. It shouldn't really be there, but it makes the code
better and it takes effort to remove.

> > +Quickly register team info received from server in client.
>
> Quickly? This assumes that there is also a slow way?!

Yes:

> > +/***************************************************************
> > +Set a player to a team. Removes previous team affiliation,
> > +creates a new team if it does not exist.
> > +***************************************************************/
>
> > +void team_add_player(struct player *plr, const char *name)

I _could_ use this also in the client, but that would be seriously slow.

> > +  if (team_id == TEAM_NONE) {
> > +    assert(FALSE);
> > +  }
>
> Whats wrong with assert(team_id!=TEAM_NONE)?

Too weak, I forgot to add exit() and error message...

> Check that this is only called in pregame. Since you stated above that
> the client trusts that the team association doesn't change during a
> game.

I did? The client part is written so that it works no matter how and when
you change it in the server. The server part would work fine from this
part of the API's perspective - the problem lies higher up in the API
hierarchy (changing alliances etc).

> Here and in some other places you use extensively parentheses. This is
> not bad per se. But usually (not sure about this, I don't have a
> statistics handy) we omit them.

Here you can see what Pascal does to a programmer... It is a bad habit -
I'm trying to lay it off.

> > --- common/packets.h        2002/10/19 04:02:07     1.126
> > +++ common/packets.h        2002/10/24 13:21:30
> > @@ -460,6 +460,8 @@
> >  int playerno;
> >  char name[MAX_LEN_NAME];
> >  bool is_male;
> > +int team;
>
> > +char team_name[MAX_LEN_NAME];
>
> Why? Base question: is it possible that the team of a player changes
> or not?

The low-level API is written so that team changing is possible, although
this is unintentional - I do not think teams should change after pregame.

> > +team_iterate(team) {
> > +  int numplayers = 0;
> > +  int count = 0;
>
> Looks like count should be renamed to num_total_players and numplayers
> to num_players_in_team.

numplayers -> team_members... ok

count, no. This is not used to count the total number of players, it is
used to refer to the currently considered player in an array, hence
"count" is accurate, IMHO.

> > + N_("Sets a player as member of a team. If no team specified, the "
> > +    "player is set teamless. Use \"\" if names contain whitespace. "
> > +    "A team is a group of players that start out allied and fight "
> > +    "together to achieve team victory.")
>
> "start out allied, with shared vision and an embassy with each
> other. Also they get each tech research by a team member at the end of
> the turn."

Actually that isn't correct - only the AI will share techs. I've changed
this to add mention of shared vision, embassies and score averaging, tho.

> > +if (str != NULL || strlen(str) > 0) {
>
> More strict checking. " " for example shouldn't be allowed.

Good point. But it isn't just " " but also "  " and maybe other invisible
characters as well? Is there a convenient way to test this?

The rest of the points (except noise) fixed. New patch will be posted
soon.

  - Per