League of Extraordinary FoundryVTT DevelopersWasp

Tagger Code Review

Would anyone be keen to code review Tagger? I'd like to git better 😄 https://github.com/Haxxer/FoundryVTT-Tagger

Calego•987d ago

I'll see about giving you some feedback today 🙂

Wasp•987d ago

Thanks!

Calego•986d ago

Some thoughts for you, gonna split these into several so they can be replied to: Documentation Recommend referring to these "objects" as "PlaceableObject" to be consistent with Foundry API terminology. https://foundryvtt.com/api/PlaceableObject.html Object Oriented Code is a 👍 I would make Tagger a class with basically all static methods instead of an object. What you have works, it just "looks wierd." E.g. how this might be useful:

class Tagger {
  static getTags (...)

  static getByTag () {
    // ...
    let tags = this.getTags();
  }
}

class Tagger {
  static getTags (...)

  static getByTag () {
    // ...
    let tags = this.getTags();
  }
}

Architecturally, you have 2 'concerns' here: 1. The handling of the actual tags on the documents 2. The patches to the existing UI elements These don't interact with eachother much from what I can tell and I'd consider splitting them up into two different classes:

class Tagger {}

class TaggerConfig {
_applyHtml
_handleTokenConfig
_handleTileConfig
_handleDrawingConfig
_handleGenericConfig
}

class Tagger {}

class TaggerConfig {
_applyHtml
_handleTokenConfig
_handleTileConfig
_handleDrawingConfig
_handleGenericConfig
}

Pulling the 'config' stuff out into its own class would let you make methods inside that class which you then pass to renderFormApplication and closeFormApplication, overall wouldn't give you much but it's something to think about for structuring code. It also makes window.Tagger less heavy. You've got some patterns in here that I personally wouldn't do or recommend. Admittedly I don't really understand why I avoid these things, it's something that was drilled into me and I've never really questioned. Mutating things tends to cause headaches, it's not generally a good practice to mutate the arguments passed to a function within that function.

    getTags(inObject) {
        if (inObject?.document) inObject = inObject.document;
        let tags = inObject?.getFlag(MODULE_NAME, FLAG_NAME) ?? [];
        return Tagger._validateTags(tags);
    },

    getTags(inObject) {
        if (inObject?.document) inObject = inObject.document;
        let tags = inObject?.getFlag(MODULE_NAME, FLAG_NAME) ?? [];
        return Tagger._validateTags(tags);
    },

So you're mutating the inObject argument if it has a document property. This could cause you a headache if you did something like this down the line:

let foo = token;
const tags = getTags(foo); // foo is mutated (I think)
foo.document.update(something); // error, document no longer on `foo`

let foo = token;
const tags = getTags(foo); // foo is mutated (I think)
foo.document.update(something); // error, document no longer on `foo`

Instead, consider playing it safe like so:

    getTags(inObject) {
        const relevantDocument = inObject?.document ?? inObject;
        const tags = relevantDocument?.getFlag(MODULE_NAME, FLAG_NAME) ?? [];
        return Tagger._validateTags(tags);
    },

    getTags(inObject) {
        const relevantDocument = inObject?.document ?? inObject;
        const tags = relevantDocument?.getFlag(MODULE_NAME, FLAG_NAME) ?? [];
        return Tagger._validateTags(tags);
    },

(also recommend using const until you can't) (double checking that mutating function arguments is actually prone to this kind of headache)

Calego•986d ago

Yeah, this article goes more into that whole shitshow: https://techblog.commercetools.com/mutating-objects-what-can-go-wrong-7b89d4b8b1ac

Medium

Mutating Objects: What Can Go Wrong?

We all know that mutating objects can lead to unwanted side effects, and can introduce hard to trace bugs, and make code hard to test. In…

Calego•986d ago

Mutation has more implications in some UI frameworks (like react) than it does in Foundry, so you're probably fine here. Like I said this is something that's been driven into me and I don't fully understand why (I work in react-land). There's a lot of _validateObjects being used which indicates that you don't trust your data is being stored right. Nothing wrong with that but it feels a bit unnecessary. If I had to guess, this stems from how you're injecting the DOM into the forms. Since you're giving the input a name, the stored value on form save would be a string:

flags: {
  Tagger: {
    tags: 'foo, bar' // or whatever the user enters
  }
}

flags: {
  Tagger: {
    tags: 'foo, bar' // or whatever the user enters
  }
}

However, you're looking for an array, so you call _fixUpTags on applicationClose to remedy this. That feels a little janky. I've got two alternatives to consider: A) Don't hook on closeFormApplication, but instead on preUpdate<WhateverDocument>. Inside the preUpdate hook you can mutate what's about to be saved to the document. This has the added benefit of happening whenever the document is saved, not necessarily on close. Some info: https://foundryvtt.wiki/en/migrations/foundry-core-0_8_x#for-update-operations B) Don't give the injected input a name, and instead use some other identifier. Then in your closeFormApplication, use the html argument to manually grab the input's value, and call setTags yourself. A+ for all the helper methods you have in here and their documentation. Inspires me to actually document my own stuff (some day... maybe...). I am ~~contractually~~ obgligated to point out that the "recommended best practice" for sharing an API from a module is by putting it on game.modules.get('my-module').api. But tbh, this is just so much more usable. It's unlikely that anyone will have a name collision with you, yolo. You should update your LICENSE to have your desired name instead of "Repository Owner". Use of fieldset to make your input stand out is neat. Looks solid. single letter variables are evil Alright @wasp that's all I got for you. This is a nice little module, neatly made. My main takeaways for you are "I hope I leave you with a vague sense of dread regarding mutation because that's what my mentors instilled in me all those years ago."

Wasp•986d ago

Thank you for all of the feedback! As for the _validateObjects - it's mostly used on public facing functions such as setTags, getTags, etc, which relies on provided parameters. It serves two functions - to validate incoming data, and to format it into a way that I would like to process it Ie, one object gets array-ified, and cast to its relevant document if not already a document But you're right, I may be relying on it a bit too much re: preUpdate<WhateverDocument> - how can I know what the tags I am to be setting if I don't hook on the config close? How do you propose I access the html I injected?

Calego•986d ago

I think since you have a name on that field, the raw data will be on the update object in the hook

Wasp•986d ago

Good point

Calego•986d ago

being more defensive isn't bad to be clear esp on the public functions, that makes good sense, I thought this was entirely because of the hook shennanegins maybe

Wasp•986d ago

Nope! Just appropriated to ensure it's valid 😄 Unnecessary now that I think about it, since the TaggerConfig will be 100% foundry side Damn, the code is so much cleaner now, thank you @calego !

LTL

Leo The League Lion•986d ago

@wasp gave LeaguePoints™ to @calego (#1 • 1146)

Calego•986d ago

https://giphy.com/gifs/song-windshield-wipers-glvyCVWYJ21fq

Giphy

Calego•986d ago

One more thing, more as a "FYI" than a suggestion. You can get away with these since you're using esmodules in the manifest, but if this were using scripts these would cause problems:

const MODULE_NAME = "tagger";
const FLAG_NAME = "tags";

const MODULE_NAME = "tagger";
const FLAG_NAME = "tags";

Remedy for that would be to move them into Tagger like so:

class Tagger {
  static MODULE_NAME = "tagger";
  static FLAG_NAME = "tags";
}

class Tagger {
  static MODULE_NAME = "tagger";
  static FLAG_NAME = "tags";
}

without the safety of either esmodules or being a class member, you would be prone to namespace collisions

Wasp•986d ago

Implemented, thanks!

League of Extraordinary FoundryVTT Developers Join

4KMembers

View on Discord

Want results from more Discord servers?

Add your server

socket woesnot the socket firing preUpdateSo for some reason ItemPF#_preUpdate changes aren't sticking.. what is PF1 doing that prevents this?mergeObject`mergeObject` is a beast of a function which lets you smash two objects together and that options ar select in sheetAlso, is the data actually saved on the actor? You can inspect the data in the console opened with F IC 5e DocsI'm not looking to ping about this but for anyone who sees this: I'm interested to know if anyone he custom hud elementMaybe I'm missing something simple here. I'm making my own custom hud element (NOT based on a Foundr Cover@badgerwerks how does the cover application for 5e helpers work? I know you do black magic to calcul Gitpod WorkflowHas anyone figured out a codespaces workflow?Dynamically get Object property from user inputAnyone know how I can do this? I have an Actor (`myactor`) and I'd like to have an input box which autopublishI'm trying to use @varriount's AutoPublish, but I keep getting this error. Any ideas?https://github.symlinking foundry.js and vscodeIs this something you were able to do with vscode? I'm trying right now with both WSL2 symlinks and Releases to ChangelogI just automatically made a Changelog by going to https://github.com/arcanistzed/scs/releases and ru codespacesGo to any github repository and press `.` You'll get kicked over to a `github.dev` version of the re equipmentType bonus⚠️ Breaking Changes ⚠️ The `DND5E.equipmentTypes` type of `bonus` has been removed. Any in-world `Scroll Into View troublesAnyone know why `Element.scrollIntoViewIfNeeded()` is working, but `Element.scrollIntoView()` isn't?DevMode Settings Explorer feedbackSo, I'm working on the styling for a little potential addition to Calego's 'Developer Mode' module a Overlay hiding starsHey y'all! I've run into a general CSS/design problem that I'm kinda stumped on solving. In the imag ToDontThingswe should have threaded a long time ago I think, whoops Layer Manipulationpresumably a module doing this would execute on canvasReady or something for every client right?Document TimestampsIs there a way to find the timestamp of the last edit made to a Document?

Gaming

Programming

Tagger Code Review