League of Extraordinary FoundryVTT Developers•3y ago

Tagger Code Review

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

14 Replies

Calego•3y ago

I'll see about giving you some feedback today 🙂

Wasp•3y ago

Thanks!

Calego•3y 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•3y 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•3y 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•3y 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•3y ago

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

Wasp•3y ago

Good point

Calego•3y 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•3y 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 !

Leo The League Lion•3y ago

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

Calego•3y ago

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

Giphy

Calego•3y 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•3y ago

Implemented, thanks!

Gaming

Programming

Tagger Code Review