-
Notifications
You must be signed in to change notification settings - Fork 940
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.
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
Add knights fighting solution #788
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your code! 🎉 It's well-structured and follows the guidelines. There are some minor improvements that could be made, but they do not significantly impact the functionality or core requirements of your code. Keep up the good work!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
knight1.take_damage(damage_to_knight1) | ||
knight2.take_damage(damage_to_knight2) | ||
|
||
knight1.hp = max(knight1.hp, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are directly modifying the hp
attribute of the Knight
class from outside of it. This is not a good practice as it breaks the encapsulation principle of OOP. The hp
attribute should be private (i.e., _hp
) and any modifications to it should be done through methods inside the Knight
class. Consider creating a method in the Knight
class that ensures the hp
cannot fall below 0, and call this method instead of directly modifying the hp
attribute here.
knight2.take_damage(damage_to_knight2) | ||
|
||
knight1.hp = max(knight1.hp, 0) | ||
knight2.hp = max(knight2.hp, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous comment, you should not directly modify the hp
attribute of the Knight
class from outside of it. Consider creating a method in the Knight
class that ensures the hp
cannot fall below 0, and call this method instead of directly modifying the hp
attribute here.
|
||
|
||
def battle(knightsConfig): | ||
# BATTLE PREPARATIONS: | ||
def battle(knights_config: dict) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name battle
could be a bit more descriptive. Consider renaming it to something like conduct_battle
or start_battle
to better reflect its purpose.
first_battle = Battle() | ||
result1 = first_battle.fight(knights[0], knights[2]) | ||
|
||
# apply armour | ||
lancelot["protection"] = 0 | ||
for a in lancelot["armour"]: | ||
lancelot["protection"] += a["protection"] | ||
second_battle = Battle() | ||
result2 = second_battle.fight(knights[1], knights[3]) | ||
|
||
# apply weapon | ||
lancelot["power"] += lancelot["weapon"]["power"] | ||
return {**result1, **result2} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a repetition in your code when creating Battle
instances and calling the fight
method. This violates the DRY (Don't Repeat Yourself) principle. Consider using a loop to create these instances and call the fight
method. You can store the knight pairs in a list and iterate over them.
|
||
print(battle(KNIGHTS)) | ||
if __name__ == "__main__": | ||
battle(KNIGHTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of the battle
function is not being used. Consider printing the result or doing something with it.
No description provided.