BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
36 stars 30 forks source link

prevent explore if not enough resources #1000

Closed aymericdelab closed 1 week ago

aymericdelab commented 1 week ago

User description

fixes #994


PR Type

enhancement, bug fix


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
useRealm.tsx
Add function to retrieve entity owner.                                     

client/src/hooks/helpers/useRealm.tsx
  • Added getEntityOwner function to retrieve the owner of an entity.
  • Included EntityOwner in the setup components.
  • +7/-1     
    useResources.tsx
    Refactor resource balance functions for state consistency.

    client/src/hooks/helpers/useResources.tsx
  • Moved currentDefaultTick inside functions to ensure up-to-date state.
  • Removed redundant useEffect dependencies.
  • +3/-5     
    useTravelPath.tsx
    Integrate exploration feasibility check in travel path.   

    client/src/ui/components/worldmap/hexagon/useTravelPath.tsx
  • Integrated canExplore function to check exploration feasibility.
  • Added checks for entity owner and food resources.
  • +8/-3     
    utils.tsx
    Add function to validate exploration requirements.             

    client/src/ui/components/worldmap/hexagon/utils.tsx
  • Added canExplore function to validate stamina and food resources.
  • +18/-0   

    πŸ’‘ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    vercel[bot] commented 1 week 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 11:44am
    github-actions[bot] commented 1 week ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review [1-5] 3
    πŸ§ͺ Relevant tests No
    πŸ”’ Security concerns No
    ⚑ Key issues to review Possible Bug:
    The getEntityOwner function in useRealm.tsx assumes that entityOwner?.entity_owner_id will always be non-null when called. This could potentially lead to runtime errors if entityOwner is null. Consider adding null checks or default values.
    Performance Concern:
    The canExplore function in utils.tsx performs multiple searches on the food array to find specific resources. This could be optimized by using a map or reducing the array to an object keyed by resourceId for faster lookups.
    github-actions[bot] commented 1 week ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a null check for entityOwner to prevent potential runtime errors ___ **Add a null check for entityOwner before calling getFoodResources to prevent potential
    runtime errors if entityOwner is undefined.** [client/src/ui/components/worldmap/hexagon/useTravelPath.tsx [29-30]](https://github.com/BibliothecaDAO/eternum/pull/1000/files#diff-2c0f825b699a1aa89a0e106d3bdef659bf441eb6239e41ffe08cd04d43600361R29-R30) ```diff const entityOwner = getEntityOwner(selectedEntity.id); -const food = getFoodResources(entityOwner!); +const food = entityOwner ? getFoodResources(entityOwner) : []; ```
    Suggestion importance[1-10]: 9 Why: Adding a null check is important to prevent potential runtime errors, making the code more robust.
    9
    Maintainability
    Refactor resource checks to use a loop for better scalability and maintainability ___ **Refactor the canExplore function to use a loop for checking resource requirements, which
    will make the function more maintainable and scalable if more resources are added in the
    future.** [client/src/ui/components/worldmap/hexagon/utils.tsx [50-57]](https://github.com/BibliothecaDAO/eternum/pull/1000/files#diff-770c44daa009ae4d3637fe48f885dfddf3ed664920aca4d4367177be3a83bcb0R50-R57) ```diff -const fish = food.find((resource) => resource.resourceId === ResourcesIds.Fish); -if ((fish?.amount || 0) < EternumGlobalConfig.exploration.fishBurn) { - return false; -} -const wheat = food.find((resource) => resource.resourceId === ResourcesIds.Wheat); -if ((wheat?.amount || 0) < EternumGlobalConfig.exploration.wheatBurn) { - return false; +const requiredResources = [ResourcesIds.Fish, ResourcesIds.Wheat]; +for (const resourceId of requiredResources) { + const resource = food.find((r) => r.resourceId === resourceId); + const requiredAmount = EternumGlobalConfig.exploration[resourceId.toLowerCase() + 'Burn']; + if ((resource?.amount || 0) < requiredAmount) { + return false; + } } ```
    Suggestion importance[1-10]: 8 Why: This refactor improves maintainability and scalability, which is beneficial for future code changes.
    8
    Enhancement
    Simplify the return statement by destructuring directly ___ **Consider destructuring entity_owner_id directly in the return statement of getEntityOwner
    to simplify the function and improve readability.** [client/src/hooks/helpers/useRealm.tsx [26-27]](https://github.com/BibliothecaDAO/eternum/pull/1000/files#diff-7a3368b90ccd171419e1dd68aafbbf5d20b6e3a231a12d711de4ff05ba9bed0cR26-R27) ```diff -const entityOwner = getComponentValue(EntityOwner, getEntityIdFromKeys([entityId])); -return entityOwner?.entity_owner_id; +return getComponentValue(EntityOwner, getEntityIdFromKeys([entityId]))?.entity_owner_id; ```
    Suggestion importance[1-10]: 7 Why: This suggestion improves readability by simplifying the return statement, but it is not crucial for functionality.
    7
    Best practice
    Extract repeated logic for fetching currentDefaultTick into a custom hook for better reusability ___ **Extract the repeated logic of fetching currentDefaultTick into a separate function or use
    a custom hook to avoid redundancy and improve code reusability.** [client/src/hooks/helpers/useResources.tsx [123]](https://github.com/BibliothecaDAO/eternum/pull/1000/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R123-R123) ```diff -const currentDefaultTick = useBlockchainStore.getState().currentDefaultTick; +const useCurrentDefaultTick = () => useBlockchainStore.getState().currentDefaultTick; +const currentDefaultTick = useCurrentDefaultTick(); ```
    Suggestion importance[1-10]: 6 Why: While this suggestion promotes code reusability and reduces redundancy, it is a minor improvement and not critical.
    6
    edisontim commented 1 week ago

    1000th PR πŸŽ‰