Follow-up Comment #8, bug #13621 (project freeciv):

> version 5; changes:

Some slight problems with this version:
- A better name would be load_ruleset_game_settings().
- 'astr' should not be static.
- 'astr' should not even be an astring, a stack
  allocated char array would suffice (just pass its
  maximum size too).
- It's hard to figure out what 'pastr' is used for,
  it is neither documented in any function header nor
  named according to its use.
- The LOG_VERBOSE messages really should be LOG_DEBUG.
- The critical send_server_setting() ...
  send_server_info_to_metaserver() part is duplicated
  3 times.
- Names of enum set_cmd_type are vague and belong to
  different categories; they need to be at least
  documented (e.g. above the enum declaration) so
  that programmers know what they are supposed to mean.
- Overuse of SCMD_* makes the codepath of set_command_*()
  hard to follow, i.e. parametrizing the behaviour
  in that way makes it harded to understand.
- Sometimes you cmd_reply (= notify_conn) in the
  set_command_*() function, and sometimes you
  notify_conn in its caller; in other words, what
  is the point of passing 'pastr' if you just
  cmd_reply inside it anyway.
- You replace the fast pointer checks with slow
  string comparisons (strcmp(op->name,...)) for
  no reason (yes we can debate which is better
  but the fact is that both depend on some hard-
  coded name, be it either the string name or
  the variable name, so you do not gain anything
  by changing it).
- You do not have to explicitly convert bool to
  int and vice versa in function arguments.
- You (probably) should not be calling send_*()
  functions while a ruleset is being loaded.

If any of the above is unclear or you do not know
why it would be a problem, ask and I will try to
elaborate on it.

My suggestion is to factor out into separate
functions [by "factor out" I mean take the duplicated
code and put it in one place so there is only one
copy, not make 3 copies; cf. 2x + xy + 3xz =
x(2 + y + 3z)]:
- setting value checking with bool return indicating
  success and error message buffer for reason:
    bool setting_check(const struct setting *op,
      int ibval, const char *sval, char *err, int errlen)
  with 'ibval' taking the bool or int value, and 'sval'
  taking a pointer to the new string, when applicable.
  Probably a nicer approach would be to use a union:
    union setting_data {
      int v_int;
      bool v_bool;
      const char *v_str;
  but the "one unused argument" is good enough, and besides
  if you were to use the union you might as well rewrite
  the entire setting handling code to make use of it
  (it would probably be a lot better as a result).
- setting updating:
    bool setting_update(...)
  which would be clearly marked as not doing any error
  checking (this should be handled by check()). Probably
  you should not handle the side-effects here, since this
  pollutes a nice low level function with high level
  dependencies. Probably a separate ticket should be
  made to remove the side-effects completely (e.g. aifill
  should be a command, not a setting, but this needs
  to be more closely examined).
- maybe reading from a secfile:
    bool setting_load_from_secfile(...)
  it can take a secfile prefix (i.e. the "game.") so as
  to be slightly more general, and would probably
  make use of check() and update().

Of course if you start to use the above approach
and it turns out to be worse than what you already
have, feel free to ignore my suggestions. :)

Really I would have been happy to commit the last
version, since if you dig much deeper into this
code you will probably end up making a substantial
rewrite. It's up to you if you want to continue
with this less than exciting stuff or just leave
it for later...



Reply to this item at:


  Message sent via/by Gna!

Freeciv-dev mailing list

Reply via email to