Closed cwastche closed 2 weeks 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 20, 2024 2:44pm |
โฑ๏ธ Estimated effort to review [1-5] | 2 |
๐งช Relevant tests | No |
๐ Security concerns | No |
โก Key issues to review | None |
Category | Suggestion | Score |
Enhancement |
Provide examples for better clarity on best practices mentioned___ **The documentation mentions usingsuper::IBuildingContract to minimize imports, which is a good practice. However, it would be beneficial to provide a brief example or snippet showing this usage directly in the context for clearer understanding.** [eternum-docs/docs/pages/development/contracts.mdx [36]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-303816324aed144bd3565f9b334d183140a2ee95bb61fc7f106fbffd09379efdR36-R36) ```diff -- use of `super::IBuildingContract` to minimise imports and make it clear where the interface is defined. +- use of `super::IBuildingContract` to minimise imports. For example, `use super::IBuildingContract;` at the beginning of your module. ``` Suggestion importance[1-10]: 9Why: Adding examples to documentation significantly enhances understanding and practical application of the best practices mentioned, making this a valuable improvement. | 9 |
Expand on broad statements to provide clarity and context___ **The addition of the line "Everything is fungible until it's not." could be expanded toprovide more context or explanation. This statement is quite broad and might confuse readers without additional information.** [eternum-docs/docs/pages/game/introduction.mdx [15]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-e24a7c2b045bcbf98876bc5ba114a1b483034b0b63af186c553672ef1472320fR15-R15) ```diff -Everything is fungible until it's not. +Everything in the game is considered fungible until specific conditions or rules apply. ``` Suggestion importance[1-10]: 8Why: Providing more context to broad statements enhances the clarity and understanding for readers, making the documentation more informative and user-friendly. | 8 | |
Add a description to the "Layouts" section for consistency and clarity___ **Consider adding a brief description for the "Layouts" section to maintain consistency withother sections, which all include descriptive text.** [eternum-docs/docs/pages/development/client.mdx [21]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-a0a27d15f8da90d9daacecf2d3de7c2246c139edad29c0327c7bcd2e63fd5956R21-R21) ```diff -- Collection of containers composed into a full layout +- Collection of containers composed into a full layout. This section should outline the overall page structure and flow. ``` Suggestion importance[1-10]: 7Why: The suggestion improves consistency and clarity by adding a description to the "Layouts" section, aligning it with other sections that include descriptive text. However, it is a minor enhancement and not crucial. | 7 | |
Best practice |
Use nullish coalescing to handle falsy values more accurately___ **Consider using nullish coalescing (?? ) instead of logical OR (|| ) to handle cases where structure.category might be an empty string or other falsy values that are valid. This ensures that only null or undefined values are replaced by the empty string, preserving other falsy values like 0 or false which might be valid in some contexts.**
[client/src/hooks/helpers/useEntities.tsx [124-128]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-655bd94eaba19bfcec06d7b736d2e64944c31d55f2b38819201a2b0b612787e4R124-R128)
```diff
const name = realm
? getRealmNameById(realm.realm_id)
: structureName
- ? `${structure?.category} ${structureName}`
- : structure?.category;
+ ? `${structure?.category ?? ''} ${structureName}`
+ : structure?.category ?? '';
```
Suggestion importance[1-10]: 8Why: This suggestion improves the handling of falsy values by using nullish coalescing, which is a best practice for ensuring that only null or undefined values are replaced. This can prevent potential bugs where valid falsy values like 0 or false might be incorrectly handled. | 8 |
Use a switch statement to replace nested ternary operators for clarity___ **Replace the nested ternary operators with a switch statement for better readability andmaintainability when determining the modelIndex based on hexType .**
[client/src/ui/components/construction/ExistingBuildings.tsx [134-142]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-b8b13d61f7fc906dae1ff3473650c5893b1d3971d1b65e062a1fb4c6ed5e749bR134-R142)
```diff
-hexType === HexType.BANK
- ? ModelsIndexes.Bank
- : hexType === HexType.SHARDSMINE
- ? ModelsIndexes.ShardsMine
- : hexType === HexType.HYPERSTRUCTURE
- ? ModelsIndexes.Hyperstructure
- : hexType === HexType.UNFINISHEDHYPERSTRUCTURE
- ? ModelsIndexes.UnfinishedHyperstructure
- : ModelsIndexes.Castle;
+let modelIndex;
+switch (hexType) {
+ case HexType.BANK:
+ modelIndex = ModelsIndexes.Bank;
+ break;
+ case HexType.SHARDSMINE:
+ modelIndex = ModelsIndexes.ShardsMine;
+ break;
+ case HexType.HYPERSTRUCTURE:
+ modelIndex = ModelsIndexes.Hyperstructure;
+ break;
+ case HexType.UNFINISHEDHYPERSTRUCTURE:
+ modelIndex = ModelsIndexes.UnfinishedHyperstructure;
+ break;
+ default:
+ modelIndex = ModelsIndexes.Castle;
+}
```
Suggestion importance[1-10]: 8Why: Replacing nested ternary operators with a switch statement enhances readability and maintainability. This is a best practice for handling multiple conditions and makes the code more understandable. | 8 | |
Standardize the file naming convention to improve consistency___ **Consider using consistent naming conventions for files in the documentation. The naming of'system_name.cairo' should follow the same pattern as 'tests.cairo', which uses all lowercase. This will help maintain consistency and readability in the documentation structure.** [eternum-docs/docs/pages/development/contracts.mdx [28]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-303816324aed144bd3565f9b334d183140a2ee95bb61fc7f106fbffd09379efdR28-R28) ```diff -- system_name.cairo +- systemname.cairo ``` Suggestion importance[1-10]: 7Why: The suggestion to standardize file naming conventions is valid and improves consistency and readability. However, it is a minor enhancement and not crucial for functionality. | 7 | |
Maintainability |
Extract reducer function to improve code readability and maintainability___ **To improve readability and maintainability, consider extracting the reducer function usedin the reduce method into a separate named function. This helps in isolating logic, making the code easier to test and understand.** [client/src/hooks/helpers/useRoads.tsx [130-137]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-f2f204c5b7d8065573c295320644cc3ddd7413354c9aef7db93258e226a5f094R130-R137) ```diff -roads.reduce( - (acc, road) => { - if (!acc[road.destinationRealmName] || acc[road.destinationRealmName].usageLeft < road.usageLeft) { - acc[road.destinationRealmName] = road; - } - return acc; - }, - {} as { [key: string]: RoadInterface }, -) +function selectRoadWithHighestUsage(acc, road) { + if (!acc[road.destinationRealmName] || acc[road.destinationRealmName].usageLeft < road.usageLeft) { + acc[road.destinationRealmName] = road; + } + return acc; +} +const uniqueRoads = roads.reduce(selectRoadWithHighestUsage, {} as { [key: string]: RoadInterface }); ``` Suggestion importance[1-10]: 7Why: Extracting the reducer function into a separate named function enhances readability and maintainability by isolating the logic, making the code easier to test and understand. However, it does not address any critical issues. | 7 |
Refactor nested ternary operators into a function for better clarity___ **Refactor the nested ternary operators into a separate function to improve code clarity andmaintainability. This function can then be used to determine the user status based on latestActivity .**
[client/src/ui/components/cityview/realm/trade/DirectOffers/DirectOffersExplorerPopup.tsx [233-239]](https://github.com/BibliothecaDAO/eternum/pull/968/files#diff-52423cdc998d716ffb1c594ce96c9cd9e72a60d85f2df80c1d5d57ee1feff7f6R233-R239)
```diff
-latestActivity === undefined
- ? "offline"
- : latestActivity < 86400
- ? "online"
- : latestActivity < 259200
- ? "recently"
- : "offline";
+function getUserStatus(latestActivity) {
+ if (latestActivity === undefined) return "offline";
+ if (latestActivity < 86400) return "online";
+ if (latestActivity < 259200) return "recently";
+ return "offline";
+}
+const userStatus = getUserStatus(latestActivity);
```
Suggestion importance[1-10]: 7Why: Refactoring the nested ternary operators into a separate function improves code clarity and maintainability. This makes the logic easier to follow and reduces the complexity of the component. | 7 |
PR Type
formatting
Description
Changes walkthrough ๐
21 files
resources.ts
Format nested ternary operators for readability.
sdk/packages/eternum/src/constants/resources.ts - Adjusted indentation for nested ternary operators.
BattleView.tsx
Format nested ternary operators for readability.
client/src/ui/modules/military/battle-view/BattleView.tsx - Adjusted indentation for nested ternary operators.
useRoads.tsx
Format reduce function for readability.
client/src/hooks/helpers/useRoads.tsx - Adjusted indentation for reduce function.
ExistingBuildings.tsx
Format nested ternary operators for readability.
client/src/ui/components/construction/ExistingBuildings.tsx - Adjusted indentation for nested ternary operators.
ListSelect.tsx
Format nested ternary operators for readability.
client/src/ui/elements/ListSelect.tsx - Adjusted indentation for nested ternary operators.
DirectOffersExplorerPopup.tsx
Format nested ternary operators for readability.
client/src/ui/components/cityview/realm/trade/DirectOffers/DirectOffersExplorerPopup.tsx - Adjusted indentation for nested ternary operators.
global.ts
Remove trailing whitespace.
sdk/packages/eternum/src/constants/global.ts - Removed trailing whitespace.
ArmyMenu.tsx
Remove trailing whitespace.
client/src/ui/components/worldmap/armies/ArmyMenu.tsx - Removed trailing whitespace.
vocs.config.ts
Standardize string quotes.
eternum-docs/vocs.config.ts - Changed single quotes to double quotes.
useEntities.tsx
Format nested ternary operators for readability.
client/src/hooks/helpers/useEntities.tsx - Adjusted indentation for nested ternary operators.
BattleActions.tsx
Remove trailing whitespace.
client/src/ui/modules/military/battle-view/BattleActions.tsx - Removed trailing whitespace.
index.html
Standardize doctype declaration.
client/hex/index.html - Changed doctype declaration to lowercase.
index.html
Standardize doctype declaration.
client/map/index.html - Changed doctype declaration to lowercase.
index.html
Standardize doctype declaration.
client/index.html - Changed doctype declaration to lowercase.
index.css
Format CSS for consistency.
client/src/index.css - Adjusted indentation and removed trailing whitespace.
contracts.mdx
Remove extra newline.
eternum-docs/docs/pages/development/contracts.mdx - Removed extra newline.
index.mdx
Standardize string quotes and add newline.
eternum-docs/docs/pages/index.mdx
client.mdx
Add missing newline.
eternum-docs/docs/pages/development/client.mdx - Added missing newline at end of file.
introduction.mdx
Remove extra newlines.
eternum-docs/docs/pages/game/introduction.mdx - Removed extra newlines.
example.mdx
Add missing newline.
eternum-docs/docs/pages/example.mdx - Added missing newline at end of file.
getting-started.mdx
Add missing newline.
eternum-docs/docs/pages/getting-started.mdx - Added missing newline at end of file.