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:
<http://gna.org/bugs/?13621>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Freeciv-dev mailing list
[email protected]
https://mail.gna.org/listinfo/freeciv-dev
