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

Dev #17

Merged
merged 9 commits into from
Sep 14, 2020
Merged

Dev #17

merged 9 commits into from
Sep 14, 2020

Conversation

omidrostamabadi
Copy link

Dear Sadra,
due to the issue #6, I've implemented the basic Player class in player.py which handles mafia players by instantiating this class each time a new user is added to the game. As a result, I had to change mafia.py, GOD.html and Player.html as well to conform to the new structure.

Copy link
Owner

@sadrasabouri sadrasabouri left a comment

Choose a reason for hiding this comment

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

That was really nice.
Thanks for your time and efforts 👍
I'll checkout to your dev branch and test it practically and then issue my last review.
I left minor comments, it may attract your attention.
If you have time, I think it would be great if we make a class for Role too.

mafia.py Outdated
@@ -11,7 +12,8 @@
id = 0
nPlayers = 0
roles = []
ip2role_index_name = {}
#ip2role_index_name = {}
Copy link
Owner

Choose a reason for hiding this comment

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

You can remove them, don't need to be commented.

mafia.py Outdated
else:
if id > nPlayers:
return render_template("404.html", is_farsi=True)
role = roles[id]
ip2role_index_name[ip] = [role,
"""ip2role_index_name[ip] = [role,
Copy link
Owner

Choose a reason for hiding this comment

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

Remove it too.

self.state = state
def set_comment(self, comment):
self.comment = comment

Copy link
Owner

Choose a reason for hiding this comment

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

Check this line it looks like it needs another new line (code style).

image_name = role + "_" + str(ip2role_index_name[ip][1])
False]"""
image_name = role + "_" + str(randrange(1, nRoles[role] + 1))
ip2players[ip] = Player(ip, username, role, image_name)
print("*" * 20, "New Player","*" * 20)
toGod = ip + " : " + str(id) + " : " + username + " --> " + role
toGod += "/" + role2fa[role] #TODO: Just in Farsi Mode
Copy link
Owner

Choose a reason for hiding this comment

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

It will be nice if we have a proper text for player informing to God which can be use like this:

toGod = print(ip2players[ip])

Which can be done using __str__ function in Player class.

mafia.py Outdated
@@ -11,7 +12,8 @@
id = 0
nPlayers = 0
roles = []
ip2role_index_name = {}
#ip2role_index_name = {}
ip2players = {}
Copy link
Owner

Choose a reason for hiding this comment

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

How about ip2player?
I mean by the way it's dictionary which maps each ip to a player.

@aeirya
Copy link

aeirya commented Sep 13, 2020

If you have time, I think it would be great if we make a class for Role too.

I couldn't agree more. This is actually what I am currently working on.

@sadrasabouri
Copy link
Owner

If you have time, I think it would be great if we make a class for Role too.

I couldn't agree more. This is actually what I am currently working on.

That's nice you can work on it too it won't cause any conflict but just in the case you can:

git remote add omid_repo https://github.com/Thepunisher79/mafia
git fetch omid_repo

and merge omid_repo/dev to your dev if it's necessary.

@sadrasabouri
Copy link
Owner

@Thepunisher79 It seems right practically.
After considering these minor issues I'll merge it.

@sadrasabouri sadrasabouri merged commit ca72324 into sadrasabouri:dev Sep 14, 2020
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.

3 participants