[crossfire] Duplicate code

[Mark Wedel]

Alex Schultz wrote:
> Hi,
> 
> I just ran some duplicate code checking stuff over the cf code base, and
> made a little report of some of the more significant cases. Perhaps we
> should store this list on the wiki or something so people can easily
> check off dealing with aspects?

  Some quick notes, without digging in too deeply on these:

> 
> Alex Schultz
> 
> random_roll() at line 66 in common/utils.c
> random_roll65() at line 111 in common/utils.c
>     Some duplicated code. Not sure at a glance if it can be merged.

  Actually, that is random_roll64().

  In practice, all calls to random_roll() could just be replaced with calls to 
random_roll64(), which the extra bits on the 64 bit version most likely being 
dropped (think that is what happens if you cast a larger size to a smaller).

  It is possible that the 64 bit version is more costly, cpu wise, than the 32 
bit version.  Probably not an issue.

> ----
> roll_stats() at line 821 in server/player.c
> swap_stat() at line 872 in server/player.c
>     swap_stat() seems to duplicate some code for resetting some things
> like level. To me this looks redundent.

  Hopefully, at some point, those just completely go away with client side 
player creation, so I wouldn't be too concerned about cleaning those up.


> ----
> find_skill_by_name() at line 147 in server/skill_util.c
> find_skill_by_number() at line 197 in server/skill_util.c
>     Code to find the correct skilltool object is duplicated. That should
> probably be moved to it's own function.
> ----

  yes - I'd say the main portion is a little further down - the code that 
actually readies the skill_tool and finds the name, etc.  Since that is about 20 
lines that is the same.

  The block within the for loop doesn't seem to make sense - that is just a 
single line that does the same thing - putting that in a function wouldn't be 
that useful.  The rest of the for loop itself probably can't be changed much, 
since it is comparing different things (number vs name).

> line 306 in include/global.h
> line 8 in include/xdir.h
>     Handling for HAVE_DIRENT_H is in both. Should be removed from one.

  Doing a quick grep, it doesn't appear xdir.h is in fact used by anything.  So 
that file could just get deleted and it shouldn't affect anything.


> ----
> fire_bolt() at line 304 in server/spell_attack.c
> fire_bullet() at line 629 in server/spell_attack.c
>     Identical 14 line code block used for handling reflecting bullets
> and bolts. Should probably be moved into it's own
>     function.

  yes - reflect_object() may make sense.

> ----
> socket/*.c
>     In many places there are significant amounts of duplicated code to
> send or recieve similarly formatted commands. Perhaps
>     more helper functions for socket stuff is in order? Not much that
> could be done other than add some more helper functions
>     which isn't a high priority.

  Some of that could also be because of different revisions of similar commands 
(item, item1, etc).  If the old ones are removed, that may clean things up bit.

  At some point, it becomes harder to figure out if a bunch of:

  if (protocol) ...
  else if (protocol2)
    if (protcol3)

  Type of things become cleaner with that type of logic, or just duplicating the 
similar code elsewhere but not having those if's is clearer (so you can just 
read down the function and see very clearly what it is doing).

> ----
> kill_player() at line 2677 in server/player.c
> kill_player() at line 2900 in server/player.c
>     Identical 17 line code chunks used for removing poisoning and
> confusion, once for in battlefields and once for outside. A
>     seperate function should be made for this. Perhaps a two functions,
> one for poison and one for confusion.

  Probably even better is one function that takes an option on what to remove, 
and then returns true/false based on of it actually does anything.

  This might also be useful in some of the healing and praying over altar code also.

> ----
> line 111 in common/init.c
> line 696 in include/spellist.h
>     spellpathnames is defined identically in each. It should only be
> defined in one. Probably best to remove it from init.c

  Actually, except for that array, there is nothing in the spellist.h file that 
is actually used anymore.

  Doing a quick grep shows that spellist.h isn't being used by any function, so 
could also just get deleted.  Otherwise, having duplicate initializers in .h 
files tend to result in duplicate symbols (the linker doesn't care/know that 
they are the same value) - that can be avoid by declaring them static, but then 
the data is being reproduced in every file, increasing file size some (and 
compilers will often complain about static values no being used also)