refactor: move setWorld to init method #73
Conversation
danielcdz
added
enhancement
WalkthroughThe changes in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/systems/world_setup.cairo (4)
Line range hint
9-17: LGTM! Consider updating the file name.The module renaming from
world_setuptoworldaligns well with the refactoring changes. The imports are correctly maintained.Consider renaming the file from
world_setup.cairotoworld.cairoto match the new module name, improving consistency.
21-62: LGTM! Consider refactoring for improved maintainability.The Beast initialization logic is correct and comprehensive. However, there are opportunities for improvement:
- Consider extracting the Beast initialization into a separate function for better code organization.
- The initialization of multiple Beasts follows a similar pattern, which could be abstracted into a reusable function to reduce code duplication.
Here's a suggested refactor to improve maintainability:
fn initialize_beast( world: @IWorldDispatcher, beast_id: u32, beast_name: felt252, beast_type: WorldElements, beast_description: felt252, player_id: u32, hp: u32, attack: u32, defense: u32, mt1: u32, mt2: u32, mt3: u32, mt4: u32, ) { set!( world, (Beast { beast_id, beast_name, beast_type, beast_description, player_id, hp, current_hp: hp, attack, defense, mt1, mt2, mt3, mt4, level: 5, experience_to_next_level: 1000 }) ); } // Usage in dojo_init: initialize_beast( world, 1, 'Firebeast', WorldElements::Draconic(()), 'A fiery beast.', 1, 100, 50, 40, 1, 2, 3, 4 ); initialize_beast( world, 2, 'Aqua', WorldElements::Crystal(()), 'A water beast', 2, 110, 45, 50, 5, 6, 7, 8 );This refactoring improves code readability and makes it easier to add new Beasts in the future.
64-89: LGTM! Consider refactoring Player initialization for consistency.The Player initialization logic is correct. To maintain consistency with the suggested Beast initialization refactor and improve maintainability, consider applying a similar approach to the Player initialization.
Here's a suggested refactor to improve consistency and maintainability:
fn initialize_player( world: @IWorldDispatcher, player_id: u32, player_name: felt252, beast_1: u32, beast_2: u32, beast_3: u32, beast_4: u32, potions: u32 ) { set!( world, (Player { player_id, player_name, beast_1, beast_2, beast_3, beast_4, potions }) ); } // Usage in dojo_init: initialize_player(world, 1, 'Ash', 1, 0, 0, 0, 2); initialize_player(world, 2, 'Misty', 2, 0, 0, 0, 3);This refactoring aligns the Player initialization with the Beast initialization style, improving overall code consistency and maintainability.
91-182: LGTM! Consider refactoring Potion and Mt initializations for improved maintainability.The Potion and Mt (Move Type) initializations are correct. However, there's an opportunity to improve code organization and reduce duplication, especially for the Mt initializations.
Here are suggested refactors to improve maintainability:
- For Potions:
fn initialize_potion( world: @IWorldDispatcher, potion_id: u32, potion_name: felt252, potion_effect: u32 ) { set!(world, (Potion { potion_id, potion_name, potion_effect })); } // Usage in dojo_init: initialize_potion(world, 1, 'Health Potion', 50); initialize_potion(world, 2, 'Super Potion', 100);
- For Mts:
fn initialize_mt( world: @IWorldDispatcher, mt_id: u32, mt_name: felt252, mt_type: WorldElements, mt_power: u32, mt_accuracy: u32 ) { set!( world, (Mt { mt_id, mt_name, mt_type, mt_power, mt_accuracy }) ); } // Usage in dojo_init: initialize_mt(world, 1, 'Fire Blast', WorldElements::Draconic(()), 90, 85); initialize_mt(world, 2, 'Ember', WorldElements::Crystal(()), 40, 100); initialize_mt(world, 3, 'Flame Wheel', WorldElements::Draconic(()), 60, 95); initialize_mt(world, 4, 'Fire Punch', WorldElements::Crystal(()), 75, 100); initialize_mt(world, 5, 'Water Gun', WorldElements::Crystal(()), 40, 100); initialize_mt(world, 6, 'Bubble', WorldElements::Draconic(()), 20, 100); initialize_mt(world, 7, 'Aqua Tail', WorldElements::Crystal(()), 90, 90); initialize_mt(world, 8, 'Hydro Pump', WorldElements::Crystal(()), 110, 80);These refactorings significantly reduce code duplication, improve readability, and make it easier to add new Potions or Mts in the future. They also provide a consistent initialization pattern across all entity types in the
dojo_initfunction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/systems/world_setup.cairo (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/systems/world_setup.cairo (2)
18-20: LGTM! Function declaration aligns with PR objectives.The
dojo_initfunction declaration is correct and properly annotated with#[dojo::contract]. It takes the expectedworldparameter of type@IWorldDispatcher, which aligns with the PR objectives of creating an initialization method for world entities.
Line range hint
1-183: Overall assessment: Changes meet PR objectives with room for further improvements.The refactoring successfully achieves the goal of creating a dedicated
dojo_initmethod for initializing world entities, aligning with the PR objectives. The removal of theIWorldSetuptrait andsetWorldmethod simplifies the module structure.Key points:
- The initialization logic for all entity types (Beast, Player, Potion, Mt) has been correctly moved into the
dojo_initfunction.- The changes maintain the existing functionality while improving the code structure.
Suggestions for further improvement:
- Rename the file to
world.cairoto match the new module name.- Implement the suggested refactorings for Beast, Player, Potion, and Mt initializations to reduce code duplication and improve maintainability.
These changes have significantly improved the codebase, and implementing the suggested refactors would further enhance its quality and maintainability.
To ensure that the changes haven't introduced any regressions, please run the existing test suite and confirm that all tests pass.
|
@evgongora Hello Erick could you post the following testing
great job so far |
|
@evgongora could you describe in a comment how did you fix the deploy thing? remember? It was like a versions problem with the rpc, thanks |
|
Hello @RolandoDrRobot! Important The init function wil keep being the same as Note The error that is showing when using the new |
|
This one is not gonna have any coed merged, I'll close It, thanks @evgongora for the findings and effort! |
Pull Request Overview
📝 Summary
Changes to
setWorldmethod todojo_initfunction following example: https://github.com/dojoengine/dojo/blob/d039c6d46f819edc3f2161c0520b8c8fecec0092/examples/spawn-and-move/src/others.cairo#L18🔄 Changes Made
dojo_initfunction inside the world contract module, this function directly initializes the world elementsIWorldSetupand its implementationWorldSetupImplsetWorldfunction, which was part of the interface, has been removed.🔧 Tests Results
🔜 Next Steps
Summary by CodeRabbit
dojo_init, for setting up game entities like Beast, Player, Potion, and Mt.IWorldSetuptrait, enhancing direct access to initialization logic and reducing complexity.