(1) I really don't see the point of having two modules. Or rather,
there is an antipoint.
Many years ago I recommended an -export_to directive so that a
module could export
selected materials to a few friends instead of to the whole world.
It's still not th
Therefore, since combattables exports basetohit/3 to combat: any
module anywhere
can call basetohit/3. You may have plans to add more stuff in the
future, but right
now, combattables has no good reason to exist.
(2) Runtogetherwords arehardtoread. What is a combattable thing? If
I swing a sword
at a wall, does that make walls combattable? In this case, I
suspect you mean
combat_tables. Runningyourwords togetherintroduces ambiguity.
For example,
in magicuserillusionist0/0, is that (magic, user, illusionist) or
(magic-user, illusionist)
or (magic, user-illusionist) -- as opposed to say
system-illusionist or what?
You could write magic_user@illusionist@0 and be perfectly clear.
But there is a
structuring issue here that suggests NOT doing that. (See below.)
(3) Comments should not be harder to understand than the code they apply to.
"For monster hitdice under 2 express Level as a decimal value."
What are hitdice? Are these hitdice that are unusually big, or are they
hitdice that are produced by, owned by, or intended for monsters?
What does "decimal value" mean here? (Bearing in mind that Erlang has
no support whatever for decimal arithmetic, all Erlang numbers
being binary.)
(4)
monster_offset(Level) ->
if
Level < 0.5 -> 0;
Level < 1 -> 1;
Level < 1.5 -> 2;
Level < 2 -> 3;
true -> ((Level) div 2) + 3
end.
To start with, I see indentation in steps of 8 here, which is
eye-jarringly large.
I always used to hate it when people told me "do not use TAB characters for
indentation." I knew what I was doing. What an idiot I was. I
quickly discovered
that I *hated* it when other people did it, and as the Silver Rule puts it,
"what is hateful to you, do not do to others."
The real problem here is that I cannot tell what Level is supposed to be.
From ((Level) div 2) + 3, Level *must* be an integer or you will
get a run-time
exception. But from 1 =< Level and Level < 1.5 being a possibility, Level
*must* be a float. So I suppose
Level is a float less than 2 or an integer.
That is a very strange type.
Looking at basetohit/.3 (and what is a baseto and who or what is
hitting it? I mean,
I know what a basenji is, so could a baseto be something like that?)
it appears that in every other case, Level MUST be an integer. So why in
this case alone may it be a float?
(5) Why are there parentheses around Level in ((Level) div 2) + 3?
basetohit(Class, Level, AC)
when (Class == cleric) or (Class == druid) or (Class == monk) ->
element( (Level+1) div 3, clericdruidmonk0() ) - AC;
Why are there those wholly unnecessary and confusing parentheses around
(This == that) tests?
(Level + 1) div 3 must be an integer in the range 1 .. 7, so
(Level + 1) must be an integer in the range 3 .. 23, so
Level must be an integer in the range 2 .. 22.
In the next clause, (Level + 1) div 2 + 1 must be in the range 1 .. 10, so
(Level + 1) div 2 must be in the range 0 .. 9, so
Level + 1 must be in the range -1 .. 19, soranger
Level must be in the range -2 ... 18.
For thief or assassin, Level must be in the range 1..24.crimin
Since the *range* (and even in one case, the *type*) of Level is different
for each kind of thing, the *meaning* of Level appears to be
different, so it
is misleading to split it out as a separate apparently independent
data value.
(6) The whole structure of baseto_hit/3 feels wrong. I finally found a way to
explain it. You are HIGHLIGHTING what you want to IGNORE.
What you actually need is something like
{ religious, RLevel, cleric|druid|monk}
{ warrior, WLevel, fighter|paladin|ranger }
{ criminal, CLevel, thief|assassin }
{ goetic, GLevel, magic_user|illusionist }
{ monster, MLevel}
baseto_hit({religious, RLevel, _}) -> ...
baseto_hit({warrior, WLevel, _}) -> ...
...
baseto_hit({monster,MLevel}) -> ...
with no guards needed at all. I shan't say that you should never
have "or" in
a guard, but I will suggest that it is a last desperate measure.
(7) AC is subtracted in each clause, so maybe what you really need is
% combat.erl
thac0(Class, Level, AC) ->
combat_tables(Class, Level) - AC.
or rather
thac0(Combatant, AC) ->
combatant_table:baseto_hit(Combatant) - AC.
(8) All that element((Level + X) div Y + Z, foo()) stuff makes me nervous.
There *ought* to be something that could be factored out, but then the
way Level varies between major categories suggests that there is less
here than meets the eye. It certainly *looks* like patching around a
problematic representation. I think that a clear comment explaining
what Levels are and why they are grouped this way is needed.
I hope this helps. The issues are not really Erlang-specific issues, but
general mostly-functional programming issues.