Crossfire Mailing List Archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Coding style suggestion



From: woodruff@cadence.com (Ken Woodruff)

>    foo = pl->stats.Str

vs

>    #define get_player_strength(PL) ((PL)->stats.Str)
and
     #define set_player_strength(PL, val) ((PL)->stats.Str = (val)) ?
     #define get_object_strength(PL) ((PL)->stats.Str) ?

>    foo = get_player_strength(PL);
> 
> This has many benefits, I'll try to outline a few:

Yes. It has advantages, but most of them requires that macros are used
everywhere in code. And that means changing all existing code.  D_LOCK(pl) 
is global macro for preventing inventory redraws, but it's not used 
much in the (new?) code.

>    - If you actually need a method you can implement one by swapping
>      a function for the macro.  For example if we wanted strength to
>      decrease with a players age (say 5% per year over 50) you could
>      simply write a get_player_strength() function that implemented 
>      this algorithm.  Otherwise you would have to add another piece
>      of code after each use of the Str field.

I don't think this is a good example, even idea behind it is. I think
that get_function should return real value without modifications and
set_function should make all modifications. It goes too complicated
if some functions make modifications and others don't (no way to
access the real value). query_value() ?? use flag for this, put
adding (afterwards) flags to get_ functions isn't resonable.

>    - A lot of object types use the fields in strange ways.

If I remember right then converters, altars amd some map functions
already uses this at least locally. Of course these should be globally
defined to avoid conflicts.


I think that using indirect access to attributes is a good idea, but
it also should be considered very carefully when/if implemented. A few 
things to note:

  - There should be minimum difference between players and monster.
    E.g. handle similar can_use_bow for monsters and players, also
    Str should be same effect for monsters and players. So does it
    make sense have get_player_str and get_object_str or is later
    enough.

  - How those access functions are designed and named?  Is it 
    get_player_str(pl) or query_value(pl, STR)?  The later is 
    similar to the current flag handling.  The proposed name
    get_player_strength(PL) is too long, IMHO. Example:

    new_wc = get_player_wc(op) - get_player_level(op) - get_thaco_bonus (op);
    set_player_wc (op, new_wc)
    vs
    op->stats.wc -= (op->level + thaco_bonus[op->stats.Str]);

    The both examples are readable, but since those values are used
    in many calculations, they could be shorter (get_[pl_]str[ength]).
    Compare op_is_player(op) vs is_player(op), since in crossfire all
    are objects, is there need op_ (ob_?) prefix? The current flag macros
    repeat FLAG part twice, and it makes code just harder to read, IMHO.

  - It has been discussed before that object structure should be cleaned,
    and some how get memory requiments lower.  It would require change
    access to attributes, so it would make sense consider both the same
    time.  I don't know if Mark has some idea how this should be done.

  -Tero