Closed kakaroto closed 4 years ago
Hey Kakaroto, Thanks for the review! I'm not the most experienced programmer yet, with less practical experience than I want. I appreciate you taking the time out of your day to provide a code review! Any future reviews will be accepted gladly!
Almost all the advice you offered I took. I hope the next time I work on something I can follow better design practice. Thanks!
You're welcome! I'll do another review later, but quick notes : your module.json includes src/hooks.js which doesn't exist and and your config.js still has all the roll tables data.
Been reviewing the code to see what it does and have some comments.. unfortunately, since this isn't from a pull request, or a single commit with all the code, I can't do random review comments on a file, and I don't want to put comments spread over all the various commits. So I'm creating an issue for it here.
Note: this isn't a critique of your code, and you don't have to listen to any of my suggestions as they might not match your own coding style, but maybe something helps you here if you find it more practical and hadn't thought of it, or you see it as a potential fix for a bug that hasn't appeared yet. Some of the comments might also be irrelevant if you switch to using a compendium roll table, but I'll still make the comment in case it helps you notice something else.
Starting with
criticalLoot.js
:Order your functions in a more logical order. Your constructor calls
init()
thenfindTable()
, but you havefindTable()
defined beforeinit()
. I'd move the init function right after the constructor so someone reading the code doesn't need to jump all over the place. HavinggenerateTable
right afterfindTable()
(where it's used) is good though.Line 13 https://github.com/JacobMcAuley/critical-fumble/blob/07e46e91d03dc73635cdb6771479b4ff560ca388/src/criticalLoot.js#L13
.bind(this)
at the end. The arrow key function ensures your function runs in the same context/score as the parent function.Line 15 https://github.com/JacobMcAuley/critical-fumble/blob/07e46e91d03dc73635cdb6771479b4ff560ca388/src/criticalLoot.js#L15
find
doesn't return a promise so you don't need toawait
game.tables
instead ofRollTables.collection
as I think that's what's mostly used in the rest of foundry.js, though this would be a matter of preference===
instead of==
since you're comparing two strings here.Line 16, use strict comparison. Or easier in this case, just
if (!result)
would be enough tooLine 17, you use the
`Critical-Fumble ${tempTable}`
again here for the table name, probably better to use a variable for it so you don't have a typo and you're sure you use the same name for thefind
as well as for the_generateTable
.Smaller details such as
tempTable
not being a table but just being the upper case table name..result
not being a result but being the actual table,findTable()
not finding a table but actually populating thetables
variable (findTable implies that it tries to find a single table somewhere) and generating missing tables, soensureTables
would be a better name as it ensures all the tables are created.init
since it's part of the initialization instead of as a separate call in the constructor (that's just a matter of preference)Object.keys().forEach
, you could dofor (let [tableId, tableData] of Object.entries(CRITICAL_FUMBLE_LOOT))
for exampletable.name
getter instead oftable.data.name
if (x) {
thanif(x){
Overall, I'd rewrite
findTable
this way :Note that I always do the
this.tables.push(table)
in here, and that assumes that the_generateTable
only generates the table and doesn't add it tothis.tables
as well which simplifies code for both functions.generateTable
function looks good. Just small things, likecreate
andcreateTableResult
would benefit from being one field per line to make it easier to read.folder: null
, it might be a good idea to have all of your tables in aCritical-Fumble
folder for ease of organizing.this.tables.push(table)
since we do it in_ensureTables
above.init()
function, you don't need to try/catch in the hook. The core code already try catches all hook callbacks and prints the error/stack if an exception is thrown.actorData
as argument, when it's anupdateToken
hook, so really should have been atokenData
token.actor.id
could crash becausetoken.actor
is nullIn
validateData
token.actor.id
in theinit
only to then do anActor.collection.get(actorId)
? I'd suggest you usetoken.actor
directly.token, id, data
as argumentquickData
token.actor
not being null here, so,if (!token.actor || token.actor.isPC) return;
table != undefined
can again be justif (table)
but if you prefer to explicitely do it, then use strict comparison!==
_onTokenUpdate
or something like that, becausevalidateData
is not very useful as a name and it does more than just "validate"In
isValidTarget
:forEach
within aforEach
is hard to read/understand, it would probably be better to also use more array-like methods, likemap
andreduce
for example!!result
to returntrue
if the result is not undefined.getTableByCR
, looks good generally butisZeroHealth
because it's used after it in the order of operations ofvalidateData
.throw
at the end is also something I'm a bit conflicted about.. it makes sense because it should never happen but on the other hand, you could just return null because you can't find a table for that specific CR (whatever its value is).tableName
when it's actually a table. SotableName.data.name
makes no sense. It should really betable.data.name
getter instead of accessing data directly, so
this.tables.find(table => table.name === ...);`isZeroHealth
:isActorDead
maybe ?actor.actorData
which would never work? I'm not sure at all how this is actually working (haven't tested the module yet so maybe it's not actually). It should beactor.data
, or maybe you were looking attoken.data.actorData
? ~~actor
as argument name when it should really beupdateTokenData
which can be very confusing!isActorDead
also doesn't work, and it should be named maybe something likeisTokenKilled
updateData.actorData.data.attributes.hp.value
will probably crash most of the time because token updates will often happen (when you move a token for example) without those fields being set, and you'll get acan't access data of undefined
error. You should use :getProperty
insteadin
rollTable
:result.img
before you check ifresult !== null
result.flags
without checking if result is valid.. you should really just do at the beginingif (!result) return;
critical_fumble_actions
flag without checking for its existance. ThehandleRolls
method expects an array, so you should really do :let parsedRolls = result.flags['critical_fumble_actions'] || []
generateTables
method but the flags are wrong. The format for flags isscope: key: value
, where the scope is the module id. So it should be :result.flags["critical-fumble"]["actions"]
which in this case means that you need an extra step to ensure that "critical-fumble" key is defined, so probably better to use get property :handleRolls
looks good too, I would just :this._roll
returns the total, so uselet total
instead oflet roll
since it would imply that return value is aRoll
class.coins
array to becoins.push({amount: total, currency: rolls[i].description})
instead ofcoinAmount
andcoinType
(the array is already for coins, so no need to prefixcoin
into all the fields too_roll
is for some reason an arrow function here, probably a mistake/old refactored code. Make that into a regular method. Otherwise, looks good._coinDistributePrompt
is good. Personally preference, I wouldn't dolet d = new Dialog(..); \n d.render(true)
I would just do :new Dialog(...).render(true)
_coinToString
. I would just put a space between the amount and the currency. Personal preference here too._coinDistribute
assumes there's only one GM and that all users are for a player (no observer, no assistant GM, etc..).var
instead oflet
for thelength
variablelength
which islength - 1
is confusing, better rename it astotalPlayers
let totalPlayers = game.users.entities.reduce((acc, u) => { if (!u.isGM && u.character) acc += 1; return acc;}, 0)
Math.floor
instead ofMath.ceil
and keep track of how many coins were shared and print how much is left over. (i.e: 5 coins and 4 players, 1 coin is left over instead of disappearing in the abyss and instead of giving 2 coins per player with theMath.ceil
)parseInt()
which will already return an integer, so you don't needMath.ceil
also, it's probably a lot less efficient than usingMath.floor
forEach
for the users, which can be confusing. Use loops for both to make it easier to read.Out of class. I would just do
new CriticalLoot()
and have theHooks.on("ready")
done in itsinit
method which would call the_ensureTables
. You also don't need to store the result of thenew
I assume.That's it for this file. I'll do a really quick pass over the
criticalFumble.js
nowA lot of similar comments here with regards to method/variable naming, the
findTable
andinit
Instead of
_validString
should beisAttackRoll
the
validateData
should first check that themessage.type === CHAT_MESSAGE_TYPES.ROLL
, and I think you can usemessage.roll
in that case without having toJSON.parse
The roll has an
options.damageType
on its parts? Didn't know that! Interesting :)in
rollTable
, Don't donew Audio
, instead use :AudioHelper.play({src: sound});
Ah you overload the d20 roll.. that's why the part has damage type.. that really shouldn't go there in my opinion because it's contaminating the json.
I won't reviewing the overloaded roll for now because I don't want to diff it, but I notice you do
data.item.damage...
but if there's a d20 roll that happens that isn't on an item, that will result in a crash ofcan't access damage of undefined
and d20 rolling stops working.Overall, I don't see any kind of checks for
isGM
which means that when a token update happens every player will try to roll from the fumble table. Also, every player when they first login, if they can't find the table, they'll try to create it themselves. Also, every player will try to create and distribute loot... Those things should only happen if the current player is the GM.