Implement Math Marauders simulation #2
Conversation
Summary of ChangesHello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers the foundational simulation for 'Math Marauders,' a wave-based strategy runner. It establishes the core game loop, managing player progression through mathematical gates and combat encounters. The changes focus on building a robust, testable backend for the game's mechanics, complemented by a functional HTML-based user interface that allows for interaction and visualization of the simulation's state. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed pull request that implements the core simulation for Math Marauders. The code is well-structured into distinct systems, follows modern JavaScript practices, and includes a good set of unit tests for the core deterministic logic. My review includes a few suggestions for improving maintainability and clarity, and addresses a potential game balance issue. Overall, this is a great foundation for the project.
| const volley = this.battles.calculateArrowVolley(this.currentArmy); | ||
| let message = `Retreat gate cleared. ${this.currentArmy} soldiers remain.`; | ||
| if (volley > 0) { | ||
| const remaining = this.battles.applyArrowVolley(volley); | ||
| message += ` Volley hits for ${volley}. Enemy remaining ${remaining}.`; | ||
| } | ||
| this.ui.setStatus(message); |
There was a problem hiding this comment.
In #resolveRetreatGate, an arrow volley is calculated and applied immediately after clearing a gate. This is in addition to the periodic arrow volleys fired by the timer set up in #startArrowLoop. This "double volley" (one on gate clear, one on timer) might not be intended and significantly impacts game balance by making the retreat phase easier than specified in the original design document, which only mentioned a timed volley. If this extra volley is intentional, it would be good to document it. Otherwise, it should probably be removed to align with the intended difficulty.
| const volley = this.battles.calculateArrowVolley(this.currentArmy); | |
| let message = `Retreat gate cleared. ${this.currentArmy} soldiers remain.`; | |
| if (volley > 0) { | |
| const remaining = this.battles.applyArrowVolley(volley); | |
| message += ` Volley hits for ${volley}. Enemy remaining ${remaining}.`; | |
| } | |
| this.ui.setStatus(message); | |
| this.ui.setStatus(`Retreat gate cleared. ${this.currentArmy} soldiers remain.`); |
| const ratio = survivors / optimal; | ||
| let stars = 1; | ||
| thresholds.forEach((threshold, index) => { | ||
| if (ratio > threshold) { | ||
| stars = index + 2; | ||
| } | ||
| }); | ||
| return Math.min(thresholds.length + 1, stars); |
There was a problem hiding this comment.
The current implementation using forEach is correct, but it can be made more concise and declarative by using array methods like filter. This improves readability by more directly expressing the logic of "count how many thresholds were passed".
const ratio = survivors / optimal;
const thresholdsPassed = thresholds.filter((t) => ratio > t).length;
const stars = 1 + thresholdsPassed;
return Math.min(thresholds.length + 1, stars);| optimal, | ||
| stars, | ||
| }); | ||
| this.stars = this.persistence.updateStars(this.currentWave, stars); |
There was a problem hiding this comment.
To improve separation of concerns, the logic for updating star ratings should be handled here in the controller rather than in the PersistenceManager. This makes the controller the single source of truth for the stars state.
This change assumes PersistenceManager.updateStars is removed and PersistenceManager.saveStars is used instead, as suggested in a related comment.
const previousBest = this.stars[this.currentWave] ?? 0;
if (stars > previousBest) {
this.stars[this.currentWave] = stars;
this.persistence.saveStars(this.stars);
}| updateStars(wave, stars) { | ||
| const current = this.loadStars(); | ||
| const previous = current[wave] ?? 0; | ||
| if (stars > previous) { | ||
| current[wave] = stars; | ||
| this.saveStars(current); | ||
| } | ||
| return current; | ||
| } |
There was a problem hiding this comment.
The updateStars method mixes business logic (updating stars if the new score is higher) with persistence logic. For better separation of concerns, this logic should be moved to the GameController. This class would be more maintainable if it only handled the raw loading and saving of data from localStorage, exposing just loadStars() and saveStars(stars).
| function buildBasicOperation(rng) { | ||
| const choice = randomInt(rng, 0, 3); | ||
| switch (choice) { | ||
| case 0: | ||
| return addOperation(randomInt(rng, 2, MAX_OPERATION_OFFSET)); | ||
| case 1: | ||
| return subtractOperation(randomInt(rng, 1, MAX_OPERATION_OFFSET - 2)); | ||
| case 2: | ||
| return multiplyOperation(randomInt(rng, 2, MAX_OPERATION_FACTOR)); | ||
| default: | ||
| return divideOperation(randomInt(rng, 2, MAX_OPERATION_FACTOR + 1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The values -2 and +1 used to adjust the random range for subtraction and division operations seem arbitrary. These "magic numbers" can make the code harder to understand and maintain, especially when balancing gameplay. It would be beneficial to either extract them into named constants with explanatory names (e.g., SUBTRACT_OFFSET_MAX_ADJUSTMENT) or add a comment explaining the reasoning behind these specific adjustments.
| #buildLayout() { | ||
| const container = document.createElement('div'); | ||
| container.className = 'hud'; | ||
|
|
||
| const header = document.createElement('header'); | ||
| header.className = 'hud__header'; | ||
|
|
||
| this.waveLabel = document.createElement('span'); | ||
| this.waveLabel.className = 'hud__wave'; | ||
| this.waveLabel.textContent = 'Wave —'; | ||
|
|
||
| this.phaseLabel = document.createElement('span'); | ||
| this.phaseLabel.className = 'hud__phase'; | ||
| this.phaseLabel.textContent = 'Phase: —'; | ||
|
|
||
| header.append(this.waveLabel, this.phaseLabel); | ||
|
|
||
| const status = document.createElement('div'); | ||
| status.className = 'hud__status'; | ||
|
|
||
| this.playerLabel = document.createElement('div'); | ||
| this.playerLabel.className = 'hud__army hud__army--player'; | ||
| this.playerLabel.textContent = 'Player: 0'; | ||
|
|
||
| this.enemyLabel = document.createElement('div'); | ||
| this.enemyLabel.className = 'hud__army hud__army--enemy'; | ||
| this.enemyLabel.textContent = 'Enemy: 0'; | ||
|
|
||
| this.starsLabel = document.createElement('div'); | ||
| this.starsLabel.className = 'hud__stars'; | ||
| this.starsLabel.textContent = `${STAR_SYMBOL.repeat(5)}`; | ||
|
|
||
| status.append(this.playerLabel, this.enemyLabel, this.starsLabel); | ||
|
|
||
| const gatePanel = document.createElement('section'); | ||
| gatePanel.className = 'hud__gate-panel'; | ||
|
|
||
| this.gateLabel = document.createElement('h2'); | ||
| this.gateLabel.className = 'hud__gate-title'; | ||
| this.gateLabel.textContent = 'Gate'; | ||
|
|
||
| const optionRow = document.createElement('div'); | ||
| optionRow.className = 'hud__gate-options'; | ||
|
|
||
| this.leftLabel = document.createElement('div'); | ||
| this.leftLabel.className = 'hud__gate-option hud__gate-option--left'; | ||
| this.leftLabel.textContent = 'Left'; | ||
|
|
||
| this.rightLabel = document.createElement('div'); | ||
| this.rightLabel.className = 'hud__gate-option hud__gate-option--right'; | ||
| this.rightLabel.textContent = 'Right'; | ||
|
|
||
| optionRow.append(this.leftLabel, this.rightLabel); | ||
|
|
||
| this.slider = document.createElement('input'); | ||
| this.slider.type = 'range'; | ||
| this.slider.min = '0'; | ||
| this.slider.max = '100'; | ||
| this.slider.value = '50'; | ||
| this.slider.className = 'hud__gate-slider'; | ||
|
|
||
| this.choiceLabel = document.createElement('div'); | ||
| this.choiceLabel.className = 'hud__choice'; | ||
| this.choiceLabel.textContent = 'Slide to choose'; | ||
|
|
||
| this.gateButton = document.createElement('button'); | ||
| this.gateButton.className = 'hud__button'; | ||
| this.gateButton.type = 'button'; | ||
| this.gateButton.textContent = 'Engage Gate'; | ||
|
|
||
| this.statusLabel = document.createElement('p'); | ||
| this.statusLabel.className = 'hud__log'; | ||
|
|
||
| const playButton = document.createElement('button'); | ||
| playButton.className = 'hud__button hud__button--primary'; | ||
| playButton.type = 'button'; | ||
| playButton.textContent = 'Play Wave'; | ||
|
|
||
| gatePanel.append( | ||
| this.gateLabel, | ||
| optionRow, | ||
| this.slider, | ||
| this.choiceLabel, | ||
| this.gateButton, | ||
| playButton, | ||
| this.statusLabel | ||
| ); | ||
|
|
||
| this.slider.addEventListener('input', () => this.#updateChoicePreview()); | ||
| this.gateButton.addEventListener('click', () => this.#commitChoice()); | ||
| playButton.addEventListener('click', () => this.onPlay()); | ||
|
|
||
| container.append(header, status, gatePanel); | ||
|
|
||
| this.popup = document.createElement('div'); | ||
| this.popup.className = 'hud__popup hidden'; | ||
| container.append(this.popup); | ||
|
|
||
| this.#updateChoicePreview(); | ||
| return container; | ||
| } |
There was a problem hiding this comment.
The #buildLayout method is over 100 lines long and handles the creation of the entire UI structure. This can be difficult to read and maintain. Consider refactoring this into smaller, more focused private methods, each responsible for building a specific part of the UI (e.g., #buildHeader, #buildStatusPanel, #buildGatePanel). Alternatively, using HTML <template> elements could provide a cleaner separation of structure and logic.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| const optimalInfo = this.waveData.optimal.forward.checkpoints[this.forwardIndex]; | ||
| const result = this.gates.resolve({gate, choice, army: this.currentArmy}); | ||
| this.currentArmy = result.value; | ||
| this.flocks.setPlayer(this.currentArmy); | ||
| const enemy = optimalInfo.enemy; |
There was a problem hiding this comment.
#resolveForwardGate uses the enemy count from the generator’s optimal checkpoints even when the player picks the other option. When optimalInfo.enemy is derived from the best gate but the player selects a weaker gate, the ensuing skirmish still uses the larger enemy size, often wiping out the player even though the weaker option should simply lead to fewer survivors. This makes non‑optimal choices unrealistically lethal and disconnects combat difficulty from the GateSystem.resolve results. Consider deriving the enemy size from result.value (the actual choice) instead of optimalInfo.enemy.
Useful? React with 👍 / 👎.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68e37728ec74832c89d0b7000dc8e5eb