Code deduplication #138

Misheus · 2025-09-12T20:59:18Z

Move duplicated functions into their own files, so they are declared once and can be require()d and used in multiple places.

I tested it and pretty sure it works fine, but I still need to make sure I didn't break anything. Also I think I missed some duplicated code. Any ideas of how to further improve the codebase in this direction are welcome.

Fixes #51, #133 #132

… require() and used in multiple places

…ld've been from the beginning

…v files. And call to dotenv was after env vars vare used.

…ryone bug

Misheus · 2025-09-13T15:26:28Z

I tested everything and it should work, but I ended up changing more stuff than I initially planned.

p2r3

Just a few nitpicks and comments, but overall great job. Good code, and some much needed refactors.

I've only looked at the code for now, I'll approve and merge this once I've tested it.

p2r3 · 2025-09-23T16:51:12Z

common.js

+    .replaceAll("@everyone", "@everyone")
+    .replaceAll("@here", "@here");


To be explicit, the zero-width space should probably be denoted with its escape sequence (\u200B), rather than pasted directly into the code.

pages/live/main.js

p2r3 · 2025-09-23T16:54:09Z

pages/main.js

Style nitpick: added lines are missing semicolons.

p2r3 · 2025-09-23T16:54:33Z

pages/profile/main.js

-    };
-
-  }
+  const whoami = await handleWhoami()


Style nitpick: missing semicolon.

p2r3 · 2025-09-23T16:58:18Z

routines/live.js

      // Announce the beginning of COTD on Discord
-      // HACK: Hardcoded role ID for now, must fix ASAP!!!
-      await discord(["live", `<@&1363135650572009523> The [Chamber Of The Day](<https://epochtal.p2r3.com/live>) is starting!\n${mapString}`], context);
+      await discord(["live", `${CONFIG.DISCORD.ROLE.COTD}The [Chamber Of The Day](<https://epochtal.p2r3.com/live>) is starting!\n${mapString}`], context);


Missing space between role ping and text?

On a tangentially related note, it's not immediately clear that config ROLEs hold ping-formatted strings. Before checking, I assumed it just held the role ID, and so thought this was a mistake.

I was thinking that ping might be @everyone, which doesn't follow the same <@&...> pattern. Now I see my approach having some flaws. But I'm not sure what would be the solution. Storing just the id in CONFIG.DISCORD.ROLE.COTD and CONFIG.DISCORD.ROLE.COTD ? "<@&" + CONFIG.DISCORD.ROLE.COTD ">" : "@everyone"? But that would move the default value from config.ts. Or maybe rename it to, let's say, PING instead of ROLE and keep it in <@&...> format? But then should the envvar hold just ID or ping-formatted string?

The space, though, is missing. I was literally thinking about not missing the space and missed the space.

soni801

There's a lot to say. First of all, great job on this. as p2r3 mentioned, this pr does many long-needed code optimizations. However, even though it's a great starting point, theres still quite a bit of nitpicks/changes that would make sense to make when you're restructuring like this in the first place.

I've made a few commends on individual files and lines, but I also have a few comments to add overall:

I don't necessarily like that /pages/shared.js is used in the backend. Yes, I know that the point is to deduplicate code, and it works well for that purpose, but the file location implies that it's only used in the frontend.
- The file should be placed further up in the file tree, but I don't know if there is a good way to solve this though, given that web clients need access to this file. Try to look into mapping files from outside the webserver root dir into a servable file locator?
- Honestly it doesn't matter that much, and it's fine to be left like this, but again when the PR is all about improving code structure I feel like this is an important point to take into account.

Unrelated to the actual changes, I'll also make some comments on your version control:

Generally great commit naming. Thank you :D
Many of your commit titles are too long however, causing them to be hard-wrapped into the commit body. Best practice is considered to keep commit titles <50 characters. (I'll add this to the contribution guidelines at some point, don't worry)
- If you do feel like you want more context, write a short title and explain context in the commit body yourself, to avoid the hard-wrapping :)

I'm honestly gonna say I'm too lazy to test this, mostly because of the enourmous scope of changes this PR does. Just to confirm, you've tested that every feature this PR touched is working as before?

I'll wait for @p2r3 to test these changes, and then I might do some minor tweaks of my own, before ultimately merging.

I'm also gonna add these for Github to auto-associate this PR to issues:

Fixes #132
Fixes #133
(the way you formatted your initial comment, only the first issue would be linked).

soni801 · 2025-11-29T12:00:40Z

pages/stream/common.js

@@ -0,0 +1,35 @@
+/**
+ * Requests a bunch of stuff for the ui to render and initializes WebSocket connection


"a bunch of stuff" is way too vague for the comment to really be necessary. It should either
a) specify what it actually requests
b) rewrite the comment to exclude that part and describe what the function does in a different way.

Where i see A to be the best option.

soni801 · 2025-11-29T12:04:26Z

pages/stream/common.js

+  const leaderboard = await (await fetch("/api/leaderboard/get")).json();
+  const config = await (await fetch("/api/config/get")).json();
+  const users = await (await fetch("/api/users/get")).json();


This is a pretty compact way to write this, and it kinda makes sense. However, double await looks very unclean and makes it take longer to read and properly understand what the code does.

I would suggest splitting the two await's into different steps, clearly outlining what every step does. This does come at the cost of slightly higher overhead (declaring intermediate varialbes), but that difference in overhead will be so small that it'll never be noticable in actual execution. For me, that is a completely acceptable tradeoff for more readable code. @p2r3 thoughts on this?

I'm pretty sure this is how it's done in base Epochtal code, likely not Myshy's idea.

I think it's readable enough, so to me the main drawback is the lack of support for cleaner error handling I suppose. But we're not doing error handling for this anyway! 😂

Eh sure, i mean its not the end of the world by any stretch of the imagination, but again, the scope of this PR includes making the code more readable. There is no real-world drawback of splitting these awaits other than the 2 minutes it takes to write.

It just seems like a missed opportunity to me to leave it as this.

soni801 · 2025-11-29T12:05:46Z

pages/stream/common.js

+   * Sends WebSocket events to the stream controller event topic, which the UI, game, and controller listen to
+   * @param {unknown} data Data to send, converted to a JSON string


Worlds tiniest nitpick lmao, but we usually add an empty comment line between these two lines

soni801 · 2025-11-29T12:13:31Z

routines/epochtal.js

Replacing VOTING_MAPS_COUNT with CONFIG.VOTING_MAPS_COUNT several times in this file is kind of a regression in my mind. VOTING_MAPS_COUNT should still be defined inline and be referred to as it originally was, however, instead of being initialized to 5, it should be set to CONFIG.VOTING_MAPS_COUNT.

Referring to a local variable that then refers to some global config makes it easier to locally overwrite the variable if that is needed for some reason at some point, and adds negligible overhead.

Generally this is just the mindset that the more abstraction/references, the better (to a certain extent of course). Define once, but for every "level deep", allow to overwrite said definitions for all underlying usages.

...I feel like i explained that in a horrible way. Does that make sense?

soni801 · 2025-11-29T12:16:27Z

util/points.js

+  if (!(cat in profile.statistics)) profile.statistics[cat] = [];
+  if (!(cat in profile.weeks)) profile.weeks[cat] = [];
+
+  profile.statistics[cat].push(catDeltaElo[cat][steamid]);
+
+  profile.weeks[cat].push(weekNumber);
+  if (!profile.weeks.total.includes(weekNumber)) {
+    profile.weeks.total.push(weekNumber);
+  }


I am kind of missing just a few small comments here explaining the code flow.

Don't get me wrong, this is simple code, but i wanna be able to see how it works without needing to read it. Again, just a tiny nitpick to slightly aid developer experience.

soni801 · 2025-11-29T12:19:32Z

util/workshopper.js

-  // Return the raw data if requested
-  if (raw) return details;
+  // Return the raw data if requested or return error
+  if (raw || typeof details === "string") return details;


Why are we checking if it is a string? (should be clearly described in the comment)

soni801 · 2025-11-29T12:21:08Z

.env.example

 # This defines how far back in time the curation algorithm fetches maps from.
 # If left unset, defaults to 604800 (one week)
 #CURATE_SECONDS=
+#The amount of maps in the voting pool


Super duper extremely tiny nitpick: add leading space here lol

soni801 · 2025-11-29T12:51:41Z

config.ts

+        ROLE: {
+            /**
+             * The ID of the Discord role to ping for announcements
+             */
+            ANNOUNCE: process.env.DISCORD_ROLE_ANNOUNCE ? "<@&" + process.env.DISCORD_ROLE_ANNOUNCE + ">" : "@everyone",
+            /**
+             * The ID of the Discord role to ping for Chamber Of The Day
+             */
+            COTD: process.env.DISCORD_ROLE_COTD ? "<@&" + process.env.DISCORD_ROLE_COTD + ">" : "@everyone"
+        },


I am not a fan of this. config.ts serves the role of aggregating configuration options and providing defaults, not data processing.

I understand what you're trying to achieve here, but this is not the place to process/modify the input string (principle of separation and modules keeping to one task). The way I would solve this is to define some type of globally available function that transforms a role ID into a pingable role, or substitute it with @everyone if the input is empty/null/undefined. I think discordjs even has a builtin function for this?

soni801 · 2025-11-29T12:55:12Z

config.ts

+    /**
+     * When the Chamber Of The Day should be released
+     */
+    COTD_CRON: process.env.COTD_CRON ?? "0 0 19 * * 6",


Style nitpick: we don't traditionally use trailing commas in objects, but this isn't really an established code style. @p2r3 thoughts on this?

If the code style is to be based entirely on my preferences as the author or whatever, then yes. I don't use trailing commas in JS.

soni801 · 2025-11-29T13:20:01Z

~~Wait hang on what? Issues don't link if you mention them in a review comment??~~

~~Lemme try again.~~

~~Fixes #132~~
~~Fixes #133~~

Okay even this didnt work apparently I had to manually link them. Github sucks.

Misheus added 6 commits September 12, 2025 23:36

chore: move duplicated functions into different files, so they can be…

b83d424

… require() and used in multiple places

fix(code style): change indentation size in new files to 2 as it shou…

6ceb1b0

…ld've been from the beginning

chore: more code deduplication

222dcaf

chore: add more config options

4265321

chore: remove redundant dotenv dependency. Bun already deals with .en…

2bb7c81

…v files. And call to dotenv was after env vars vare used.

chore: move discord sanitizing into its own function and handle @ eve…

d0de04d

…ryone bug

Misheus force-pushed the code-deduplication branch from 667421e to d0de04d Compare September 13, 2025 12:44

Misheus added 2 commits September 13, 2025 18:06

fix: a bug I introduced in one of the previous commits

a215adc

chore: add DISCORD_CHANNEL_LIVE config option

4f29264

Misheus marked this pull request as ready for review September 13, 2025 15:26

p2r3 reviewed Sep 23, 2025

View reviewed changes

fix: code style

327c329

soni801 requested changes Nov 29, 2025

View reviewed changes

soni801 assigned Misheus Nov 29, 2025

This was linked to issues Nov 29, 2025

More config options #132

Open

Ping roles config option #133

Open

		.replaceAll("@everyone", "@everyone")
		.replaceAll("@here", "@here");

		@@ -0,0 +1,35 @@
		/**
		* Requests a bunch of stuff for the ui to render and initializes WebSocket connection

		* Sends WebSocket events to the stream controller event topic, which the UI, game, and controller listen to
		* @param {unknown} data Data to send, converted to a JSON string

Code deduplication #138

Are you sure you want to change the base?

Code deduplication #138

Uh oh!

Conversation

Misheus commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Misheus commented Sep 13, 2025

Uh oh!

p2r3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soni801 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p2r3 Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soni801 Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soni801 commented Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Misheus commented Sep 12, 2025 •

edited

Loading

p2r3 Nov 29, 2025 •

edited

Loading

soni801 Nov 29, 2025 •

edited

Loading

soni801 commented Nov 29, 2025 •

edited

Loading