Idiomatic Erlang?

[Richard O'Keefe]

(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@REDACTED@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.

On Mon, 26 Jul 2021 at 22:43, Benjamin Scherrey <scherrey@REDACTED> wrote:
>
> Been a couple of decades since I did any Erlang coding from scratch and it seems to have improved significantly (didn't have proper strings back then for example). I have a little hobby project I'm using to get myself up to snuff so I can do some more serious work for an upcoming project. Would appreciate some feedback regarding a few issues/questions. The code can be found at https://github.com/scherrey/dnderl .
>
> If you've ever played Advanced Dungeons & Dragons you'll recognize that this is a simple implementation of the THAC0 (to-hit armour class 0) concept from the original combat tables in the Dungeon Masters Guide. So 'combat:thac0(fighter, 5, 2).' would correctly tell you that your 5th level fighter needs to roll a 14 or higher on a 20-sided die to hit an armour class 2 opponent whereas 'combat:thac0(thief, 5, 2).' would tell you that a similar level thief requires a roll of 17 or better. Here are my questions:
>
> 1) My top level module is combat.erl ( https://github.com/scherrey/dnderl/blob/master/combat.erl ). All it really does is introduce the class atom type and re-export combattables:basetohit/3 (albeit under a different name, thac0/3). Is it necessary to create a forwarding function like thac0/3 like I'm doing when I'd really just prefer to establish thac0/3 in the combattables module and re-export it from combat.erl? (Don't read anything into the different names - I just decided to rename it in the top level module because I liked it better and would be happy to use that same name throughout.) Attempting to re-export a function declared in another module gives me an error.
>
>
> 2) In combattables.erl ( https://github.com/scherrey/dnderl/blob/master/combattables.erl ) I put monster_offset/1 as its own function because I couldn't figure out syntax for how to get the result of the if statement as an intermediate variable which I would have put inside the last basetohit/3 implementation. I tried several things but nothing that seemed correct to me would compile so I had to break it out into its own function. I feel like I'm missing something obvious but perhaps it's simply not allowed?
>
>
> 3) Is the way I've overloaded basetohit/3 with guards depending on which class is passed in the preferred idiomatic way to implement such a construct in Erlang? I thought of trying 'case' or 'if' but this seemed "cleanest" to me. I generally avoid if statements whenever possible in my other programming languages like C++ & Python. Appreciate any insights for such circumstances.
>
>
> 4) Is there no such thing as a module scope for a variable in Erlang? In combattables.erl I have 4 functions that return tuples representing the combat tables for each class. My initial thought was that they would be variables that could be referenced by any function in the module but that doesn't seem to work. The tuples in this case are constant lookup tables not to be updated. Am I doing this in the "correct" idiomatic manner for Erlang?
>
>
> thanks & best regards,
>
>  -- Ben Scherrey