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)
___
crossfire mailing list
crossfire@metalforge.org
http://mailman.metalforge.org/mailman/listinfo/crossfire
