Mephiles / torntools_extension

A browser extension for Torn.com
GNU General Public License v3.0
106 stars 60 forks source link

Faction and Company ID fixes #790

Closed Kwack-Kwack closed 2 months ago

Kwack-Kwack commented 2 months ago
Sashank999 commented 2 months ago

We should keep the listener as changing between companies on joblist.php will trigger the custom event and have the ID updated.

Sashank999 commented 2 months ago

You should also add caching to the company ID call as it will mostly not change.

Kwack-Kwack commented 2 months ago

hashes.get returns null when the key/value pair isn't found, and isIntNumber(null) returns true, causing the final two case checks to always return true. Perhaps the built-in isIntNumber should also do a null check first? Would love to check this before making any changes

function isIntNumber(number) {
        if (number == null) return false; // number == null is true for null and undefined, but false for 0 and NaN.
        // could also do a strict check as undefined is caught by the function below, it doesn't really matter
    return !isNaN(number) && isFinite(number) && number % 1 === 0; // already existing logic
}

You're right about the company pages, I knew there must have been a reason for having it in the first place. The only issue is this event listener also fires on opening the employees tab on your own company page, so I'll likely just add a pathname check in the callback.

Good call with the caching, I'll get that done as well, thanks!

Sashank999 commented 2 months ago

Yes, a check so that the custom event listener is applied only for job listings should work.

I suggest isIntNumber as:

function isIntNumber(number) {
    return number !== null && !isNaN(number) && isFinite(number) && number % 1 === 0;
}

undefined already returns false so no == needed.

Kwack-Kwack commented 2 months ago

Updated, and the faction ID feature has also been updated to match.

Both features are much more readable now in my opinion.