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.

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 #781

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Solution #781

wants to merge 7 commits into from

Conversation

sashasyrota
Copy link

No description provided.

Copy link

@viktoria-rybenchuk viktoria-rybenchuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add all changed files to the PR

Copy link

@Dimosphen1 Dimosphen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several changes were requested



def battle(knights: dict) -> dict:
# apply weapon

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these comments, the code is self-explicit enough

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use lower case in directory names here and in the other places as well

Comment on lines 16 to 29
first_knight = knights_list[stats]
second_knight = knights_list[stats + 2]
first_knight["hp"] -= (second_knight["power"]
- first_knight["protection"])
second_knight["hp"] -= (first_knight["power"]
- second_knight["protection"])
if first_knight["hp"] <= 0:
first_knight["hp"] = 0
if second_knight["hp"] <= 0:
second_knight["hp"] = 0
result.update({
first_knight["name"]: first_knight["hp"],
second_knight["name"]: second_knight["hp"]
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could place part of the logic that happens in the loop to a separate function for a better readability

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to create separate file for each function, create a single file utils.py and place armour, potion and weapon functions there

app/main.py Outdated


print(battle(KNIGHTS))
# battle

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove redundant comments

app/main.py Outdated
from app.Battle.battle import battle


knights = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place knights config to a separate file for better logic separation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

зображення_2024-10-08_142647058
you meant place config like this?

app/main.py Outdated

print(battle(KNIGHTS))
# battle
print(battle(knights))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a good idea to use if __name__ == "__main__" here

Copy link

@pavlopro pavlopro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants