Follow-up Comment #9, patch #4830 (project freeciv):

Thanks for the updates: this is lots cleaner and easier to work with.

Applies smoothly over r25244, and builds.

Naming:
    Most of the rest of the codebase uses singular nouns for most purposes,
with plural nouns only used when specifically handling multiple things (and
not as a list).  To follow this, most uses of "multipliers" should probably be
"multiplier".

handle_player_mulitpliers()
    This probably belongs in server/plrhand.c, in a similar manner to
handle_player_rates(), handle_player_change_government, etc.

    CodingStyle mandates declaring variables at the beginning of the
scope, followed by a blank line.

multipliers.ruleset
    A null ruleset (not exposing any multipliers) needs to be supplied for all
the other rulesets (excluding experimental).  Currently, the server crashes on
start (can't load ruleset) with the default ruleset.  This crash persists even
if one starts the server with -r experimental.serv

load_ruleset_multipliers()
    I'd put this between load_game_names() and load_technology_names() just
because I like alphabetical order, but it's not important.

send_ruleset_multipliers()
    The dl_send_packet_ruleset_multiplier() call runs over 77 characters

multiplier_count
    I don't see this initialised anywhere on the client: packhand.c seems to
assume it to be 0 without checking.  I also don't understand why this is set
to 0 in game_ruleset_free(): what is the intent here?

multiplier_used
    I don't see this initialised anywhere on client or server.  Aside from the
apparent useless set to 0 in game_ruleset_free(), it also does not appear to
be used outside multipliers.c, so if probably ought be scoped to only that
file.

game.c:
    With the removal of the rest of these variables, this file no longer needs
to include multipliers.h

effects.h:
    I still like pointers to data structures, rather than indexed access to
arrays :)

get_target_bonus_effects()
    This code would benefit from modification of the comment to describe the
new behaviour.

popup_multipliers_dialog()
    I don't know much about GTK, but I would have expected this to be a static
function, or to be declared in a header file somewhere.


    _______________________________________________________

Reply to this item at:

  <http://gna.org/patch/?4830>

_______________________________________________
  Message sent via/by Gna!
  http://gna.org/


_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to