Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: combat revamp #1140

Merged
merged 18 commits into from
Jul 29, 2024
Merged

feat: combat revamp #1140

merged 18 commits into from
Jul 29, 2024

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Jul 26, 2024 •
edited by github-actions bot
Loading

PR Type

Tests, Enhancement

Description

  • Added comprehensive unit tests for BattleManager class.
  • Refactored army-related hooks and utilities.
  • Enhanced ArmyManagementCard component with new functionalities and improved UI.
  • Extended BattleManager class with new methods for battle management.
  • Refactored BattleActions component to use new BattleManager methods.
  • Updated StructureCard component to integrate with new battle functionalities.
  • Refactored EntityDetails component to use new hooks and added tabs for entities and battles.
  • Enhanced StructureListItem component with new battle-related functionalities.
  • Refactored BattleView component to use new BattleManager methods.
  • Updated BattleSideView component to integrate with new battle functionalities.
  • Fixed issue with resource deposit timing.

Changes walkthrough 📝

Relevant files
Tests
1 files
BattleManager.test.ts
Add unit tests for BattleManager functionalities                 

client/src/dojo/modelManager/tests/BattleManager.test.ts

  • Added comprehensive unit tests for BattleManager class.
  • Mocked dependencies and implemented various test cases.
  • Verified functionalities like battle state, health updates, and troop
    updates.
    		•  +755/-0 
    Enhancement
    9 files
    useArmies.tsx
    Refactor army-related hooks and utilities                               

    client/src/hooks/helpers/useArmies.tsx

  • Refactored army-related hooks and utilities.
  • Removed redundant Battle component references.
  • Added new hooks for fetching user armies in battle and enemy armies by
    position.
    		•  +158/-84
    ArmyManagementCard.tsx
    Enhance ArmyManagementCard with delete functionality and UI
    improvements

    client/src/ui/components/military/ArmyManagementCard.tsx

  • Enhanced ArmyManagementCard component with new functionalities.
  • Added delete army functionality with confirmation.
  • Improved UI elements and state management.
    		•  +164/-118
    BattleManager.ts
    Extend BattleManager with new battle management methods   

    client/src/dojo/modelManager/BattleManager.ts

  • Extended BattleManager class with new methods for battle management.
  • Added methods for checking battle states, updating health, and
    managing raids.
  • Refactored existing methods for better readability and performance.
    		•  +197/-35
    BattleActions.tsx
    Refactor BattleActions to use new BattleManager methods   

    client/src/ui/modules/military/battle-view/BattleActions.tsx

  • Refactored BattleActions component to use new BattleManager methods.
  • Improved state management and UI interactions.
  • Added memoization for performance optimization.
    		•  +70/-104
    StructureCard.tsx
    Update StructureCard with new battle functionalities         

    client/src/ui/components/hyperstructures/StructureCard.tsx

  • Updated StructureCard component to integrate with new battle
    functionalities.
  • Added merge troops panel and enhanced UI elements.
    		•  +47/-76 
    EntityDetails.tsx
    Refactor EntityDetails with new hooks and tabs                     

    client/src/ui/modules/entity-details/EntityDetails.tsx

  • Refactored EntityDetails component to use new hooks and utilities.
  • Added tabs for entities and battles.
  • Improved UI layout and interactions.
    		•  +76/-137
    StructureListItem.tsx
    Enhance StructureListItem with battle functionalities       

    client/src/ui/components/worldmap/structures/StructureListItem.tsx

  • Enhanced StructureListItem component with new battle-related
    functionalities.
  • Added battle action buttons and improved UI elements.
    		•  +139/-50
    BattleView.tsx
    Refactor BattleView to use new BattleManager methods         

    client/src/ui/modules/military/battle-view/BattleView.tsx

  • Refactored BattleView component to use new BattleManager methods.
  • Improved state management and UI interactions.
    		•  +65/-51 
    BattleSideView.tsx
    Update BattleSideView with new battle functionalities       

    client/src/ui/modules/military/battle-view/BattleSideView.tsx

  • Updated BattleSideView component to integrate with new battle
    functionalities.
  • Improved UI elements and state management.
    		•  +22/-39 
    Bug fix
    1 files
    DepositResources.tsx
    Fix resource deposit timing issue                                               

    client/src/ui/components/resources/DepositResources.tsx

  • Fixed issue with resource deposit timing.
  • Improved state management and UI feedback.
    		•  +1/-1     
    Additional files (token-limit)
    51 files
    useBattles.tsx
    ...                                                                                                           

    client/src/hooks/helpers/battles/useBattles.tsx

    ...

    		 +29/-62 
    useStructures.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useStructures.tsx

    ...

    		 +108/-7 
    ArmyList.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyList.tsx

    ...

    		 +65/-38 
    BattleListItem.tsx
    ...                                                                                                           

    client/src/ui/components/battles/BattleListItem.tsx

    ...

    		 +153/-0 
    __mock__.ts
    ...                                                                                                           

    client/src/dojo/modelManager/tests/mock.ts

    ...

    		 +136/-0 
    ArmyChip.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyChip.tsx

    ...

    		 +54/-28 
    Battle.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/Battle.tsx

    ...

    		 +19/-27 
    SelectPreviewBuilding.tsx
    ...                                                                                                           

    client/src/ui/components/construction/SelectPreviewBuilding.tsx

    ...

    		 +16/-9   
    BattleProgressBar.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/BattleProgressBar.tsx

    ...

    		 +22/-28 
    InventoryResources.tsx
    ...                                                                                                           

    client/src/ui/components/resources/InventoryResources.tsx

    ...

    		 +38/-30 
    Battles.tsx
    ...                                                                                                           

    client/src/ui/components/models/buildings/worldmap/Battles.tsx

    ...

    		 +18/-29 
    BattleDetails.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/BattleDetails.tsx

    ...

    		 +16/-51 
    Army.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/Army.tsx

    ...

    		 +5/-35   
    EnnemyArmies.tsx
    ...                                                                                                           

    client/src/ui/modules/entity-details/EnnemyArmies.tsx

    ...

    		 +78/-0   
    Select.tsx
    ...                                                                                                           

    client/src/ui/elements/Select.tsx

    ...

    		 +12/-14 
    ArmyInfoLabel.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx

    ...

    		 +7/-8     
    useEventHandlers.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/hexagon/useEventHandlers.tsx

    ...

    		 +8/-1     
    TroopChip.tsx
    ...                                                                                                           

    client/src/ui/components/military/TroopChip.tsx

    ...

    		 +12/-5   
    useEntities.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useEntities.tsx

    ...

    		 +5/-5     
    Entities.tsx
    ...                                                                                                           

    client/src/ui/modules/entity-details/Entities.tsx

    ...

    		 +51/-0   
    LockedResources.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/LockedResources.tsx

    ...

    		 +8/-23   
    DojoContext.tsx
    ...                                                                                                           

    client/src/hooks/context/DojoContext.tsx

    ...

    		 +23/-13 
    useTravelPath.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/hexagon/useTravelPath.tsx

    ...

    		 +8/-8     
    Battles.tsx
    ...                                                                                                           

    client/src/ui/modules/entity-details/Battles.tsx

    ...

    		 +39/-0   
    Entity.tsx
    ...                                                                                                           

    client/src/ui/components/entities/Entity.tsx

    ...

    		 +3/-4     
    useResources.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useResources.tsx

    ...

    		 +1/-16   
    WorldStructuresMenu.tsx
    ...                                                                                                           

    client/src/ui/modules/world-structures/WorldStructuresMenu.tsx

    ...

    		 +6/-6     
    ArmyHitBox.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/ArmyHitBox.tsx

    ...

    		 +7/-8     
    TradeHistoryEvent.tsx
    ...                                                                                                           

    client/src/ui/components/trading/TradeHistoryEvent.tsx

    ...

    		 +3/-3     
    EntityList.tsx
    ...                                                                                                           

    client/src/ui/components/list/EntityList.tsx

    ...

    		 +7/-3     
    ArmyViewCard.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyViewCard.tsx

    ...

    		 +1/-6     
    useGuilds.tsx
    ...                                                                                                           

    client/src/hooks/helpers/useGuilds.tsx

    ...

    		 +2/-2     
    BattlesArmyTable.tsx
    ...                                                                                                           

    client/src/ui/components/military/BattlesArmyTable.tsx

    ...

    		 +2/-2     
    StaminaResource.tsx
    ...                                                                                                           

    client/src/ui/elements/StaminaResource.tsx

    ...

    		 +1/-2     
    Structures.tsx
    ...                                                                                                           

    client/src/ui/components/models/buildings/worldmap/Structures.tsx

    ...

    		 +2/-2     
    BattleCard.tsx
    ...                                                                                                           

    client/src/ui/components/battles/BattleCard.tsx

    ...

    		 +25/-0   
    RightNavigationModule.tsx
    ...                                                                                                           

    client/src/ui/modules/navigation/RightNavigationModule.tsx

    ...

    		 +2/-2     
    Troops.tsx
    ...                                                                                                           

    client/src/ui/modules/military/battle-view/Troops.tsx

    ...

    		 +10/-2   
    LeftNavigationModule.tsx
    ...                                                                                                           

    client/src/ui/modules/navigation/LeftNavigationModule.tsx

    ...

    		 +2/-2     
    Armies.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/armies/Armies.tsx

    ...

    		 +5/-3     
    HexLayers.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/hexagon/HexLayers.tsx

    ...

    		 +1/-1     
    useBlockchainStore.tsx
    ...                                                                                                           

    client/src/hooks/store/useBlockchainStore.tsx

    ...

    		 +2/-2     
    types.ts
    ...                                                                                                           

    client/src/hooks/store/types.ts

    ...

    		 +10/-5   
    RealmListItem.tsx
    ...                                                                                                           

    client/src/ui/components/worldmap/realms/RealmListItem.tsx

    ...

    		 +1/-1     
    global.ts
    ...                                                                                                           

    sdk/packages/eternum/src/constants/global.ts

    ...

    		 +2/-2     
    EntitiesArmyTable.tsx
    ...                                                                                                           

    client/src/ui/components/military/EntitiesArmyTable.tsx

    ...

    		 +1/-1     
    ArmyPanel.tsx
    ...                                                                                                           

    client/src/ui/components/military/ArmyPanel.tsx

    ...

    		 +2/-2     
    index.ts
    ...                                                                                                           

    client/src/types/index.ts

    ...

    		 +5/-0     
    contracts.cairo
    ...                                                                                                           

    contracts/src/systems/combat/contracts.cairo

    ...

    		 +80/-69 
    system_models.json
    ...                                                                                                           

    contracts/scripts/system_models.json

    ...

    		 +26/-4   
    package.json
    ...                                                                                                           

    client/package.json

    ...

    		 +1/-0     

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

    @vercel Vercel
    Copy link

    vercel bot commented Jul 26, 2024 •
    edited
    Loading

    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 Jul 29, 2024 8:22am

    @github-actions GitHub Actions
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The method getUpdatedTroops in BattleManager class might throw an error if health.current is greater than health.lifetime. This scenario should be handled gracefully without throwing errors to avoid crashing the application.

    Code Smell
    The method raidStructure in BattleManager class directly logs to the console which is not recommended. Consider using a logging framework or handling the error more gracefully.

    UI Improvement
    The ArmyManagementCard component has a delete button that does not have a confirmation step directly in the UI flow, which might lead to accidental deletions. Consider adding a confirmation modal or an additional step to verify the user's intent.

    Refactoring Needed
    The BattleActions component has a complex conditional rendering logic that makes the component difficult to read and maintain. Consider breaking down the component into smaller subcomponents or using a more declarative approach to handle the conditional rendering.

    @github-actions GitHub Actions
    Copy link

    github-actions bot commented Jul 26, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure proper typing and usage of the battle variable within the useBattleManager function

    Refactor the useBattleManager function to ensure that the battle variable is
    properly typed and used within the useMemo callback to avoid potential runtime
    errors.

    client/src/hooks/helpers/battles/useBattles.tsx [70-79]

     export const useBattleManager = (battleId: bigint) => {
   const dojo = useDojo();
-  const battle = useComponentValue(dojo.setup.components.Battle, getEntityIdFromKeys([battleId]));
+  const battle = useComponentValue(dojo.setup.components.Battle, getEntityIdFromKeys([battleId])) as BattleInfo;
   const battleManager = useMemo(() => {
-    return new BattleManager(battleId, dojo);
+    return new BattleManager(battleId, battle, dojo);
   }, [battleId, battle]);
   return battleManager;
 };
    Suggestion importance[1-10]: 10

    Why: Ensuring proper typing and usage of the battle variable is crucial to avoid potential runtime errors, making this a high-priority improvement.

    		10
    Add null check for 'army.health.current' to prevent potential errors

    To avoid potential null reference errors, add a null check for army.health.current
    before comparing it to 0n.

    client/src/hooks/helpers/useArmies.tsx [705]

    -return army.health.current > 0n;
+return army.health?.current > 0n;
    Suggestion importance[1-10]: 9

    Why: Adding a null check for 'army.health.current' is important to prevent potential null reference errors, which could cause runtime exceptions.

    		9
    Add a null check for battle.last_updated to prevent runtime errors

    Consider checking if battle is undefined before accessing its properties in
    getElapsedTime. This will prevent potential runtime errors when battle is undefined.

    client/src/dojo/modelManager/BattleManager.ts [54-58]

     const battle = this.getBattle();
 if (!battle) return 0;
-const durationSinceLastUpdate = currentTimestamp - Number(battle.last_updated);
+const durationSinceLastUpdate = currentTimestamp - Number(battle?.last_updated ?? 0);
    Suggestion importance[1-10]: 8

    Why: This suggestion adds a null check for battle.last_updated, which is a good practice to prevent potential runtime errors when battle is undefined. It addresses a possible bug and improves code robustness.

    		8
    Prevent underflows in time calculations by adding checks

    Replace the direct subtraction in the getElapsedTime method with a safer calculation
    that checks for underflows, which can occur with large timestamp values.

    client/src/dojo/modelManager/tests/BattleManager.test.ts [104]

    -expect(battleManager.getElapsedTime(CURRENT_TIMESTAMP)).toBe(CURRENT_TIMESTAMP - Number(LAST_UPDATED));
+const elapsedTime = CURRENT_TIMESTAMP > Number(LAST_UPDATED) ? CURRENT_TIMESTAMP - Number(LAST_UPDATED) : 0;
+expect(battleManager.getElapsedTime(CURRENT_TIMESTAMP)).toBe(elapsedTime);
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential bug by adding a check to prevent underflows in time calculations, improving the reliability of the getElapsedTime method.

    		8
    Enhancement
    Correct the typo and enhance readability in the function definition

    Refactor the generateMockBatle function to correct the typo in the function name and
    improve the structure of the object returned to enhance readability.

    client/src/dojo/modelManager/tests/mock.ts [21-66]

    -export const generateMockBatle = (isOngoing: boolean, lastUpdated?: number, loser?: BattleSide) => {
+export const generateMockBattle = (isOngoing: boolean, lastUpdated?: number, loser?: BattleSide) => {
   return {
     entity_id: BATTLE_ENTITY_ID,
     ...
     last_updated: BigInt(lastUpdated ?? LAST_UPDATED),
   };
 };
    Suggestion importance[1-10]: 9

    Why: Correcting the typo in the function name is important for code clarity and maintainability. The suggestion also maintains the existing structure, ensuring no functionality is lost.

    		9
    Adjust troop calculations to handle data inconsistencies gracefully

    The function getUpdatedTroops should handle the case where currentHealth.current is
    greater than currentHealth.lifetime more gracefully by adjusting the troop count
    proportionally instead of throwing an error. This can help in scenarios where data
    inconsistencies might lead to application crashes.

    client/src/dojo/modelManager/tests/BattleManager.test.ts [259-269]

    -expect(() => battleManager["getUpdatedTroops"](currentHealth, currentTroops)).toThrow(
-  "Current health shouldn't be bigger than lifetime",
-);
+const troopReductionFactor = currentHealth.lifetime > 0 ? currentHealth.current / currentHealth.lifetime : 0;
+const updatedTroops = {
+  knight_count: currentTroops.knight_count * troopReductionFactor,
+  paladin_count: currentTroops.paladin_count * troopReductionFactor,
+  crossbowman_count: currentTroops.crossbowman_count * troopReductionFactor,
+};
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the robustness of the code by preventing application crashes due to data inconsistencies. However, it changes the logic significantly, which might not be the intended behavior in all cases.

    		7
    Robustness
    Add error handling to asynchronous mock setups

    Introduce error handling for asynchronous operations within the vi.mock setup to
    catch and handle potential rejections or exceptions, ensuring the mock setup does
    not fail silently.

    client/src/dojo/modelManager/tests/BattleManager.test.ts [19-26]

     vi.mock("@dojoengine/recs", async () => {
-  const actual = await vi.importActual("@dojoengine/recs");
-  return {
-    ...actual,
-    getComponentValue: vi.fn(),
-    runQuery: vi.fn(),
-  };
+  try {
+    const actual = await vi.importActual("@dojoengine/recs");
+    return {
+      ...actual,
+      getComponentValue: vi.fn(),
+      runQuery: vi.fn(),
+    };
+  } catch (error) {
+    console.error("Failed to mock @dojoengine/recs:", error);
+    throw error;
+  }
 });
    Suggestion importance[1-10]: 9

    Why: Adding error handling to asynchronous operations within the mock setup enhances the robustness of the tests by ensuring that any issues during the mock setup are caught and handled appropriately.

    		9
    Possible issue
    Add type assertion to ensure the 'battle' variable is of type 'BattleInfo'

    Consider using a type assertion or a type guard to ensure that the battle variable
    is indeed of type BattleInfo before using it in the armyHasLost and battleIsFinished
    functions. This will prevent potential runtime errors if the battle variable does
    not conform to the expected type.

    client/src/hooks/helpers/useArmies.tsx [696-697]

    -const battle = getBattle(getEntityIdFromKeys([army?.battle_id || 0n]), Battle);
-if (battle && armyHasLost(army, battle as BattleInfo) && battleIsFinished(Army, battle as BattleInfo)) {
+const battle = getBattle(getEntityIdFromKeys([army?.battle_id || 0n]), Battle) as BattleInfo;
+if (battle && armyHasLost(army, battle) && battleIsFinished(Army, battle)) {
     return true;
 }
    Suggestion importance[1-10]: 8

    Why: Adding a type assertion for 'battle' ensures type safety and prevents potential runtime errors, which is crucial for maintaining robust code.

    		8
    Ensure battleId is valid before using it in isRaidable

    Use a more robust type check for battleId in isRaidable to ensure it's not just
    non-zero but also a valid ID. This can prevent logical errors when battleId is
    improperly set.

    client/src/dojo/modelManager/BattleManager.ts [254-255]

    -if (this.battleId !== 0n && this.battleId === raider.battle_id) {
+if (this.battleId && this.battleId === raider.battle_id) {
   await this.dojo.setup.systemCalls.battle_leave_and_raid({
     signer: this.dojo.account.account,
     army_id: raider.entity_id,
     battle_id: this.battleId,
     structure_id: structureEntityId,
   });
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the robustness of the code by ensuring battleId is valid before using it. It prevents logical errors when battleId is improperly set, addressing a possible issue.

    		7
    Best practice
    Improve type safety by specifying more precise types for the list and get methods

    Refactor the DojoAccount interface to use a more specific type than any for the list
    and get methods to improve type safety and code clarity.

    client/src/hooks/context/DojoContext.tsx [8-17]

     interface DojoAccount {
   create: () => void;
-  list: () => any[];
-  get: (id: string) => any;
+  list: () => Account[];
+  get: (id: string) => Account | undefined;
   select: (id: string) => void;
   account: Account | AccountInterface;
   isDeploying: boolean;
   clear: () => void;
   accountDisplay: string;
 }
    Suggestion importance[1-10]: 8

    Why: Improving type safety by specifying more precise types enhances code reliability and maintainability, which is a best practice.

    		8
    Use BigInt for 'battle_id' to handle large numbers correctly

    To ensure the correct handling of large numbers, consider using BigInt for all
    operations and comparisons involving battle_id to avoid potential precision loss in
    JavaScript numbers.

    client/src/hooks/helpers/useArmies.tsx [260]

    -const armiesEntityIds = useEntityQuery([HasValue(Army, { battle_id: battle_id }), NotValue(Army, { battle_id: 0n })]);
+const armiesEntityIds = useEntityQuery([HasValue(Army, { battle_id: BigInt(battle_id) }), NotValue(Army, { battle_id: 0n })]);
    Suggestion importance[1-10]: 7

    Why: Using BigInt for 'battle_id' ensures that large numbers are handled correctly, which is a good practice to avoid precision loss in JavaScript.

    		7
    Ensure immutability by cloning the battle object before updates

    Refactor the getUpdatedBattle function to ensure that the battle object is cloned
    before modification to avoid unintended side effects on the original battle object.

    client/src/dojo/modelManager/tests/BattleManager.test.ts [317-331]

    -const updatedBattle = battleManager.getUpdatedBattle(currentTimestamp);
+const clonedBattle = JSON.parse(JSON.stringify(battle));
+const updatedBattle = battleManager.getUpdatedBattle(currentTimestamp, clonedBattle);
    Suggestion importance[1-10]: 6

    Why: Cloning the battle object before modification is a good practice to avoid unintended side effects. However, the suggestion does not provide a complete implementation, and the existing code does not show any direct modification of the battle object.

    		6
    Maintainability
    Improve the function name for clarity and maintainability

    Consider using a more descriptive function name than getTroopHeathSimplified to
    improve code readability and maintainability. A name like calculateTroopHealth would
    be clearer in indicating the function's purpose.

    client/src/dojo/modelManager/tests/mock.ts [11-13]

    -const getTroopHeathSimplified = (troopCount: bigint): bigint => {
+const calculateTroopHealth = (troopCount: bigint): bigint => {
   return troopCount * 2n;
 };
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and maintainability by using a more descriptive function name, which is a good practice but not critical.

    		7
    Refactor repeated army querying logic into a separate function

    Refactor the repeated logic for querying armies by position into a separate function
    to improve code maintainability and reduce duplication.

    client/src/hooks/helpers/useArmies.tsx [442-447]

    -const ownArmiesAtPosition = useEntityQuery([
-    Has(Army),
-    HasValue(Position, { x: position.x, y: position.y }),
-    Not(Protectee),
-    HasValue(Owner, { address: BigInt(account.address) }),
-    inBattle ? NotValue(Army, { battle_id: 0n }) : HasValue(Army, { battle_id: 0n }),
-]);
+const ownArmiesAtPosition = queryArmiesByPosition(position, account.address, inBattle);
+// Define the new function elsewhere in your codebase
+function queryArmiesByPosition(position, address, inBattle) {
+    return useEntityQuery([
+        Has(Army),
+        HasValue(Position, { x: position.x, y: position.y }),
+        Not(Protectee),
+        HasValue(Owner, { address: BigInt(address) }),
+        inBattle ? NotValue(Army, { battle_id: 0n }) : HasValue(Army, { battle_id: 0n }),
+    ]);
+}
    Suggestion importance[1-10]: 6

    Why: Refactoring repeated logic into a separate function improves code maintainability and reduces duplication, although it is not critical for functionality.

    		6
    Simplify the isClaimable method by consolidating conditions

    Refactor the isClaimable method to reduce complexity and improve readability by
    extracting conditions into well-named helper functions.

    client/src/dojo/modelManager/BattleManager.ts [162-176]

    -if (!selectedArmy) return false;
-if (this.battleIsClaimable) return this.battleIsClaimable;
-if (this.isBattleOngoing(currentTimestamp)) {
-  return false;
-}
-if (!structure) {
-  this.battleIsClaimable = false;
-  return false;
-}
-if (this.getBattleType(structure) !== BattleType.Structure) {
+if (!selectedArmy || this.battleIsClaimable || this.isBattleOngoing(currentTimestamp) || !structure || this.getBattleType(structure) !== BattleType.Structure) {
   this.battleIsClaimable = false;
   return false;
 }
 if (defender === undefined) {
   this.battleIsClaimable = true;
   return true;
 }
    Suggestion importance[1-10]: 5

    Why: This suggestion refactors the isClaimable method to reduce complexity and improve readability. While it enhances maintainability, the improvement is relatively minor.

    		5
    Performance
    Store durationSinceLastUpdate in a variable to avoid redundant calculations

    Avoid recalculating durationSinceLastUpdate multiple times within the same method by
    storing it in a variable at the beginning of the method.

    client/src/dojo/modelManager/BattleManager.ts [58-62]

     const durationSinceLastUpdate = currentTimestamp - Number(battle.last_updated);
-if (Number(battle.duration_left) >= durationSinceLastUpdate) {
-  return durationSinceLastUpdate;
-} else {
-  return Number(battle.duration_left);
-}
+return Math.min(Number(battle.duration_left), durationSinceLastUpdate);
    Suggestion importance[1-10]: 6

    Why: This suggestion improves performance by avoiding redundant calculations of durationSinceLastUpdate. It simplifies the code and makes it more efficient.

    		6

    bob0005
    bob0005 reviewed Jul 26, 2024
    View reviewed changes
    client/package.json Outdated Show resolved Hide resolved
    edisontim and others added 15 commits July 26, 2024 17:05
    @edisontim
    Heavily modify battle systems
    6e8df68
    @edisontim
    Build and prettier
    d2f3f57
    @edisontim
    Create util function to get percentage of remaining troops in battle
    c3946a6
    @edisontim
    Rename resources function
    c112a62
    @credence0x @edisontim
    Limit army (#1141)
    64bae48 
    * contract: limit armies and enable deletion

* contracts: delete army if merging kills troop

* contracts: bug fixes from v1 branch

* update test
    @edisontim
    remove tooltip library
    010c157
    @edisontim
    Add function to delete armies and limit number of armies in UI
    23b2a43
    @edisontim
    prettier + remove unused imports
    9f7f8f0
    @edisontim
    address PR comments
    7046730
    @edisontim
    Fix "ennemy" typo
    52871ee
    @edisontim
    Remove biome and add hintmodal in entity details panel
    cd891e9
    @edisontim
    Remove bigint casts
    112f665
    @edisontim
    Change sdk values back
    ff22a7d
    @edisontim
    Fix small bugs in battleview + fix deposit of resources on structure …
    1d8dc5f 
    …in battle + small optimizations
    @edisontim
    prettier
    6554179
    @edisontim edisontim force-pushed the feat/combat-revamp branch from 2274e5a to 6554179 Compare July 26, 2024 15:09
    @bob0005 bob0005 merged commit 371b02f into main Jul 29, 2024
    20 checks passed
    @bob0005 bob0005 deleted the feat/combat-revamp branch July 29, 2024 09:37
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @bob0005 bob0005 bob0005 approved these changes

    Assignees
    No one assigned
    Labels
    enhancement New feature or request Review effort [1-5]: 4 tests
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @edisontim @bob0005 @credence0x