Closed ponderingdemocritus closed 3 months ago
The latest updates on your projects. Learn more about Vercel for Git โ๏ธ
Name | Status | Preview | Comments | Updated (UTC) |
---|---|---|---|---|
eternum | โ Ready (Inspect) | Visit Preview | ๐ฌ Add feedback | Jun 24, 2024 0:36am |
โฑ๏ธ Estimated effort to review [1-5] | 3 |
๐งช Relevant tests | No |
๐ Security concerns | No |
โก Key issues to review |
Possible Bug: The logic for filtering out 'Lords' from the resources might not be correctly implemented. The condition resource.id !== ResourcesIds.Lords assumes that 'Lords' is a resource identified by an ID, which should be verified. |
Code Clarity: The use of perLords in the price calculations is unclear and potentially misleading. It's important to ensure that the variable names accurately reflect their purpose and the type of values they hold. | |
Performance Concern: The repeated calculations and filters inside the map function for each resource could be optimized. Consider calculating values outside the map function or using memoization to avoid unnecessary recalculations. |
Category | Suggestion | Score |
Robustness |
Add error handling for the
___
**Add error handling for the | 9 |
Maintainability |
Refactor repeated filtering logic into a separate function___ **Refactor the repeated filtering logic into a separate function to enhance code reusabilityand readability. This change will make the code cleaner and easier to maintain.** [client/src/ui/components/trading/MarketModal.tsx [150-156]](https://github.com/BibliothecaDAO/eternum/pull/990/files#diff-60c160393d9cf2b5da3291c081dd0d823b49d1cb1dcdda811544686c0ddd347eR150-R156) ```diff -const askPrice = resourceBidOffers - .filter((offer) => (resource.id ? offer.makerGets[0]?.resourceId === resource.id : true)) - .reduce((acc, offer) => (offer.perLords < acc ? offer.perLords : acc), Infinity); -const bidPrice = resourceAskOffers - .filter((offer) => offer.takerGets[0].resourceId === resource.id) - .reduce((acc, offer) => (offer.perLords < acc ? offer.perLords : acc), Infinity); +function getBestPrice(offers, resourceId) { + return offers + .filter((offer) => offer.takerGets[0].resourceId === resourceId) + .reduce((acc, offer) => (offer.perLords < acc ? offer.perLords : acc), Infinity); +} +const askPrice = getBestPrice(resourceBidOffers, resource.id); +const bidPrice = getBestPrice(resourceAskOffers, resource.id); ``` Suggestion importance[1-10]: 8Why: This suggestion enhances code reusability and readability by refactoring repeated logic into a separate function. It is a good practice for maintainability and reduces code duplication. | 8 |
Improve variable naming for clarity___ **Consider using a more descriptive variable name thanperLords for clarity and maintainability. The name perLords does not clearly convey what the variable represents. A more descriptive name could be pricePerLord , which explicitly states that the variable holds the price per Lord unit.** [client/src/ui/components/trading/MarketModal.tsx [150-152]](https://github.com/BibliothecaDAO/eternum/pull/990/files#diff-60c160393d9cf2b5da3291c081dd0d823b49d1cb1dcdda811544686c0ddd347eR150-R152) ```diff const askPrice = resourceBidOffers .filter((offer) => (resource.id ? offer.makerGets[0]?.resourceId === resource.id : true)) - .reduce((acc, offer) => (offer.perLords < acc ? offer.perLords : acc), Infinity); + .reduce((acc, offer) => (offer.pricePerLord < acc ? offer.pricePerLord : acc), Infinity); ``` Suggestion importance[1-10]: 7Why: The suggestion improves code readability and maintainability by using a more descriptive variable name. However, it is a minor change and does not address any critical issues. | 7 | |
Best practice |
Use a constant for default BigInt value___ **Consider using a constant for the defaultBigInt("0") value used in multiple places to avoid magic numbers and improve maintainability.** [client/src/ui/components/trading/MarketOrderPanel.tsx [165]](https://github.com/BibliothecaDAO/eternum/pull/990/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR165-R165) ```diff -entityId={entityId || BigInt("0")} +const DEFAULT_ENTITY_ID = BigInt("0"); +entityId={entityId || DEFAULT_ENTITY_ID} ``` Suggestion importance[1-10]: 6Why: Using a constant for the default BigInt value is a good practice that improves maintainability by avoiding magic numbers. However, it is a minor improvement. | 6 |
User description
PR Type
Enhancement
Description
Qty
column and excludingLords
from filtered resources.depth
calculation for market resources to display the number of offers.MarketOrderPanel
to include adepth
property in theMarketResource
component.Changes walkthrough ๐
MarketModal.tsx
Enhance market resource sidebar with quantity and depth
client/src/ui/components/trading/MarketModal.tsx
Qty
column to the market resource sidebar.Lords
from filtered resources.depth
calculation for market resources.MarketOrderPanel.tsx
Update market order panel with depth and acceptance logic
client/src/ui/components/trading/MarketOrderPanel.tsx
depth
property toMarketResource
component.canAccept
logic to determine if an order can be accepted.