[Freeciv-Dev] Re: City Names Version 7

[Vasco Alexandre Da Silva Costa <vasc@xxxxxxxxxxxxxx>]

On Tue, 20 Feb 2001, Erik Sigra wrote:

> Get it at <ftp://ftp.freeciv.org/pub/freeciv/incoming/citynames-7.diff.bz2>.

Much nicer!


Still a couple of things... some i said earlier:
* use freeciv coding style please: 'indent -kr -i2'

this means code like this:

void func(int test)
{
  if (1) {
    foo;
  } else {
    bar;
  }
}

case in point:

Obj *append_two_lists(Obj *x1, Obj *x2) {
  if (!listp(x1)) x1 = cons(x1, lispnil);
  if (!listp(x2)) x2 = cons(x2, lispnil);
  if (x2 == lispnil) return x1;
  else if (x1 == lispnil) return x2;
  else return cons(car(x1), append_two_lists(cdr(x1), x2));
}

or:

Obj *cdddr(Obj *x) {return cdr(cdr(cdr(x)));}


* i think server/util.c can be eliminated/merged with the rest of the
files, it's kind of puny.

* there's some hashtable & linked list code there that could use freeciv
functions.

* when having stuff like this:

char *c_string(Obj *x) {
  if (x->type == STRING) {
    return x->v.str;
  } else if (x->type == SYMBOL) {
    return x->v.sym.symentry->name;
  } else {
    /* (should be internal_type_error?) */
    type_warning("c_string", x, "string/symbol", lispnil);
    return "";
  }
}

IMHO a switch() is nicer.

* comments before functions should be like this:

/**************************************************************************
  Returns the number of bytes still unread by pack_iter.
  That is, the length minus the number already read.
  If the packet was already too short previously, returns -1.
**************************************************************************/

not like this:

/* Make a game module for the given name and maybe bolt it into the include
   list of another module. */

* more variables & functions could be made static to avoid namespace
pollution.

* some function & var names could be better.

I'm sorry for giving you so much trouble but IMHO it's better to fix stuff
like this before rather than after commiting to CVS.

----
Vasco Costa