-
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
Solution #794
base: master
Are you sure you want to change the base?
Solution #794
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 so far! 😊 However, there are a few things that need to be addressed. The hardcoding in the battle
function needs to be replaced with a more dynamic approach. Also, the battle logic in the battle_between
function needs to be revised. The usage of the subtraction operator in the context of the battle is not intuitive, consider using more descriptive method names. Lastly, the __init__
method should adhere to the Single Responsibility Principle. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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 so far! However, there are a few areas where the code could be improved. In particular, there are several instances of code repetition that could be resolved by creating helper functions. Also, it would be beneficial to avoid mutating instances in your methods. Keep up the good work, and with a few adjustments, your code will be even better! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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 this pull request! You've shown a good understanding of the code structure and applied the DRY principle effectively. However, there are some areas that could be improved. Please review the comments regarding the use of global variables, function responsibilities, and method naming. Keep up the good work and continue to improve your code quality! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
class Knight: | ||
def __init__(self, knight: dict) -> None: | ||
self.knight_property = knight |
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.
Code Style Issue: The variable name knight_property
is not very descriptive. Consider changing it to something more descriptive, such as knight_attributes
or knight_stats
.
return True | ||
return False | ||
|
||
def set_power(self) -> int: |
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.
Code Style Issue: The method name set_power
is misleading because it does not actually set the power attribute of the knight. It simply returns a calculated value. Consider renaming it to something like calculate_power
.
+ (self.knight_property["potion"]["effect"].get("power", 0) | ||
if self.is_item("potion") else 0)) | ||
|
||
def set_hp(self) -> int: |
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.
Code Style Issue: Similar to the set_power
method, the set_hp
method does not actually set the hp attribute of the knight. Consider renaming it to calculate_hp
.
+ (self.knight_property["potion"]["effect"].get("hp", 0) | ||
if self.is_item("potion") else 0)) | ||
|
||
def set_protection(self) -> int: |
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.
Code Style Issue: Similar to the previous methods, the set_protection
method does not set the protection attribute of the knight. Consider renaming it to calculate_protection
.
0) | ||
if self.is_item("potion") else 0)) | ||
|
||
def preparation(self) -> None: |
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.
Clean Code Issue: The preparation
method is where the power, hp, and protection attributes of the knight are actually set. This method could be renamed to prepare_for_battle
to make its purpose more clear. Additionally, consider adding a docstring to this method to explain what it does.
def preparation(self) -> None: | ||
self.power = self.set_power() | ||
self.hp = self.set_hp() | ||
self.protection = self.set_protection() |
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.
Don`t Repeat Yourself Issue: You could use a loop to avoid repeating the same logic for setting the power, hp, and protection attributes. Create a dictionary with the attribute names as keys and the calculation methods as values, then iterate over the dictionary to set the attributes. Here is an example of how you could do this:
python
attributes = {'power': self.calculate_power, 'hp': self.calculate_hp, 'protection': self.calculate_protection}
for attr, calculation in attributes.items():
setattr(self, attr, calculation())
This way, if you need to add more attributes in the future, you only need to update the dictionary.
knight.power = self.power - other.protection | ||
return knight | ||
|
||
def fight(self, other: Knight) -> Knight: |
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.
Clean Code Issue: The fight
method could be renamed to attack
to make its purpose more clear. Additionally, consider adding a docstring to this method to explain what it does.
def battle_between(knight1: Knight, knight2: Knight) -> None: | ||
knight1.fight(knight2.reduce_power(knight1)) | ||
knight2.fight(knight1.reduce_power(knight2)) |
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.
This is a code quality issue. You have a battle_between
function that accepts two Knight
objects and makes them fight. But the way you've implemented this function, it's not clear who is attacking whom. In the current implementation, both knights attack each other at the same time, which doesn't make much sense in the context of a battle. Consider revising this function to clarify the sequence of actions.
|
||
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.
This is a code quality issue. The function battle
is doing too many things: it prepares knights for battle, conducts the battle, and returns the results. Consider breaking it down into smaller functions, each responsible for a single task. This would make your code more readable and easier to maintain.
for knight1, knight2 in battle_table: | ||
battle_between(knights[knight1], knights[knight2]) |
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.
This is a code style issue. You're using a global variable battle_table
inside the battle
function. It's generally a bad practice to use global variables, as it can lead to unexpected behavior and make your code harder to understand. Consider passing the battle_table
as a parameter to the battle
function instead.
No description provided.