completed task #868
completed task #868
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request:
-
Potion Application: The
apply_potionmethod is called without checking if the potion isNonefor bothred_knightandmordred. This could lead to errors if the potion is not provided. Please add a check to ensure the potion exists before applying it. -
Weapon Parameter: The
Knightclass constructor expects aweapondictionary, but currently, only thepowervalue is being passed. Ensure that the entireweapondictionary is passed to the constructor to prevent errors. -
Attribute Initialization: The
apply_potionmethod assumesself.armouris already defined. Make sureself.armouris initialized in the__init__method or elsewhere before calling this method to avoid attribute errors. -
Armour Application: The
apply_armourmethod expects a list of dictionaries with a 'protection' key. Ensure the input matches this expectation to prevent runtime errors. -
Weapon Handling: In line 9, you are adding
self.weapondirectly toself.power, which will cause an error since a dictionary cannot be added to an integer. Extract the specific value, such asweapon['power'], to add toself.power.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you need further assistance. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
app/Preparation/Knights.py
Outdated
| class Knight: | ||
| def __init__(self, name: str, power: int, | ||
| hp: int, | ||
| weapon: dict) -> None: |
There was a problem hiding this comment.
The weapon parameter is expected to be a dictionary, but in line 9, you are directly adding self.weapon to self.power. This will raise an error since you cannot add a dictionary to an integer. You might want to extract a specific value from the weapon dictionary, such as weapon['power'], to add to self.power.
| self.weapon = weapon | ||
| self.power += self.weapon | ||
|
|
||
| def apply_armour(self, protection: list[dict]) -> None: |
There was a problem hiding this comment.
The apply_armour method expects a list of dictionaries, each containing a 'protection' key. Ensure that the input to this method matches this expectation to avoid runtime errors.
| elif key == "power": | ||
| self.power += value | ||
| elif key == "protection": | ||
| self.armour += value |
There was a problem hiding this comment.
The apply_potion method assumes that self.armour is already defined. Ensure that self.armour is initialized in the __init__ method or elsewhere before this method is called to avoid attribute errors.
| red_knight["hp"] += red_knight["potion"]["effect"]["hp"] | ||
| lancelot_value = knights_config["lancelot"] | ||
| lancelot = Knight(lancelot_value["name"], lancelot_value["power"], | ||
| lancelot_value["hp"], lancelot_value["weapon"]["power"]) |
There was a problem hiding this comment.
The Knight class constructor expects a weapon dictionary, but you are passing lancelot_value["weapon"]["power"], which is an integer. You should pass the entire weapon dictionary instead.
|
|
||
| arthur_value = knights_config["arthur"] | ||
| arthur = Knight(arthur_value["name"], arthur_value["power"], | ||
| arthur_value["hp"], arthur_value["weapon"]["power"]) |
There was a problem hiding this comment.
Similar to the previous comment, ensure you pass the entire weapon dictionary to the Knight constructor, not just the power value.
|
|
||
| mordred_value = knights_config["mordred"] | ||
| mordred = Knight(mordred_value["name"], mordred_value["power"], | ||
| mordred_value["hp"], mordred_value["weapon"]["power"]) |
There was a problem hiding this comment.
Ensure you pass the entire weapon dictionary to the Knight constructor, not just the power value.
| red_knight_value = knights_config["red_knight"] | ||
| red_knight = Knight(red_knight_value["name"], red_knight_value["power"], | ||
| red_knight_value["hp"], | ||
| red_knight_value["weapon"]["power"]) |
There was a problem hiding this comment.
Ensure you pass the entire weapon dictionary to the Knight constructor, not just the power value.
app/main.py
Outdated
| mordred = Knight(mordred_value["name"], mordred_value["power"], | ||
| mordred_value["hp"], mordred_value["weapon"]["power"]) | ||
| mordred.apply_armour(mordred_value["armour"]) | ||
| mordred.apply_potion(mordred_value["potion"]["effect"]) |
There was a problem hiding this comment.
The apply_potion method is called without checking if mordred_value["potion"] is None. This could lead to an error if potion is not provided. Consider adding a check similar to the other knights.
app/main.py
Outdated
| red_knight_value["hp"], | ||
| red_knight_value["weapon"]["power"]) | ||
| red_knight.apply_armour(red_knight_value["armour"]) | ||
| red_knight.apply_potion(red_knight_value["potion"]["effect"]) |
There was a problem hiding this comment.
The apply_potion method is called without checking if red_knight_value["potion"] is None. This could lead to an error if potion is not provided. Consider adding a check similar to the other knights.
There was a problem hiding this comment.
Review Summary
Great job on addressing the issues previously mentioned! Your code now meets the task requirements and constraints. Here are some key points and suggestions for further improvement:
-
Armour Initialization: Ensure that
fighter_1.armourandfighter_2.armourare initialized before use to avoid attribute errors. Consider initializingself.armourto0instead ofNoneto prevent errors during arithmetic operations. -
Weapon Handling: The
weaponparameter is currently handled as an integer, which aligns with your current implementation. However, ensure consistency across your codebase, especially if previous feedback suggested using a dictionary. -
Code Consistency: The
apply_potionmethod andKnightclass constructor are correctly implemented, ensuring that potions and weapons are applied as intended. This consistency is crucial for the functionality of your game mechanics. -
Final Conclusion: Your code is well-structured, organized into modules and packages, and passes
flake8andpytest. The changes made have improved the functionality and reliability of your code.
Keep up the good work! Remember to review the comments to further enhance your code and learn from any mistakes. Addressing critical issues in the future will help you avoid similar problems and improve your coding skills. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| fighter_1.hp -= (fighter_2.power - fighter_1.armour) | ||
| fighter_2.hp -= (fighter_1.power - fighter_2.armour) |
There was a problem hiding this comment.
Ensure that fighter_1.armour and fighter_2.armour are initialized before they are used in these calculations. This is crucial to avoid attribute errors if the armour is not set.
| class Knight: | ||
| def __init__(self, name: str, power: int, | ||
| hp: int, | ||
| weapon: int) -> None: |
There was a problem hiding this comment.
The weapon parameter is currently an integer, but previous feedback suggested passing a dictionary. Ensure consistency in how weapons are handled across your codebase.
| self.hp = hp | ||
| self.weapon = weapon | ||
| self.power += self.weapon | ||
| self.armour = None |
There was a problem hiding this comment.
Consider initializing self.armour to 0 instead of None to avoid errors when performing arithmetic operations.
No description provided.