Refactored mafia and mafia_param #18
Conversation
… not_found_page()
.using player var instead of direct access to array . moved request filtering to a function
. edited player class
converted to a data class
added functions die, is_alive, ban, unban, switch_ban
. added max_comments function
added functions to allow backward compatibility. ready to pull request.
converted request mapper to a dict, handled invalid requests better
working on the role module. (gets teams from the json)
and their teams are hardcored into code (and removed from json)
If you are not going to solve |
needless to say this issue links directly to #6 |
There was a problem hiding this comment.
Hi @aeirya
Thanks for your PR, I reviewed it and there was some points you should consider.
After resolving them I'll check performance (respected to the time of server actions).
Remember to keep it as simple as possible so that anyone can easily read the code and contribute to it.
| def get_repitition(self) -> int: | ||
| return int(mafia_params.nRoles[self.name]) | ||
|
|
||
| # get desription(self) -> str here |
There was a problem hiding this comment.
Remove these comments and issue them.
| ) | ||
|
|
||
| class Roles(enum.Enum): | ||
| #could've used enum.auto() |
There was a problem hiding this comment.
Remove this too, you can talk about your strategies and decisions in PR comments so that one who want to read about it can find his/her answer easily and keep code simple too.
There was a problem hiding this comment.
ok .. it was actually a note for myself which I forgot to remove from code
| return Roles["_".join([word.upper() for word in string.split()])] | ||
|
|
||
| roles = dict([(r, r.to_role()) for r in Roles]) | ||
| ordered_roles = [roles.get(get(key)) for key in mafia_params.ordered_roles] |
There was a problem hiding this comment.
Last new line is common for code files.
Please consider it in your files.
| @@ -1,110 +1,171 @@ | |||
| from sys import argv | |||
| from random import randrange, shuffle | |||
| from typing import Callable, Dict, List, Text, Tuple | |||
There was a problem hiding this comment.
I don't believe typing can help us.
Most of these variables are in-app variables and have controlled values so we don't need typing.
Instead of that you can make proper docstring for your functions.
There was a problem hiding this comment.
I personally don't see any reason not to use it.
Sure we could use docstrings too but for more rewarding info imo.
Using too much doc is just gonna get our code cluttered and slow to read by others.
But still, I could remove them after after I'm done with those functions, if u insist.
There was a problem hiding this comment.
This kind of variable assignment is reminding Rust for me and I don't want the code be that much frightening to look at it, for instance it may be obvious for reader to understand that is_... may return True or False.
| self.n_players = n_players | ||
| self.roles = roles | ||
|
|
||
| def is_ip_valid(self, ip : str) -> bool: |
| def kill(player : Player) -> None: | ||
| # notice banned players cannot be killed | ||
| if player.is_alive(): | ||
| player.die() |
There was a problem hiding this comment.
add another if which makes player alive if kill is pressed again after player's death.
There was a problem hiding this comment.
done.
also checks for is_banned now
| def get_requests() -> List[Tuple[str, Player]]: | ||
| requests = [] | ||
| for key in request_mapper.keys(): | ||
| ip = request.args.get(key) | ||
|
|
||
| if ip is None: | ||
| continue | ||
| if not player_manager.is_ip_valid(ip): | ||
| print("invalid ip / request: ", ip, f"({key})") | ||
| return -1 | ||
|
|
||
| player = player_manager.get_player(ip) | ||
| requests.append((key, player)) | ||
| return requests |
There was a problem hiding this comment.
I don't get the point of this part please explain it for me and it's use in application
There was a problem hiding this comment.
it's supposed to check for every possible request type in our request object, and check if the ip address is valid (it was previously done by duplicated lines of ip checking).
what you may find strange at first is that this function returns a list of request types and their arguments.
it isn't uncommon to split a big function to smaller ones with different responsibilities, which I did for the god function.
tho the main reason i made get_requests was that fact i was unsure if it is possible for a request object to have multiple args or not (and i actually guess the answer is no, which means i'll probably clean this part a little bit soon)
| """A player in mafia game""" | ||
| def __init__(self, ip, username, role, image_name): | ||
| self.ip = ip | ||
| self.username = username | ||
| self.state = "alive" | ||
| self.role = role | ||
| self.image_name = image_name | ||
| self.comment = False | ||
| ip : str | ||
| username : str | ||
| role : str | ||
| image_name : str | ||
| state : PlayerState = PlayerState.ALIVE | ||
| comment : bool = False |
There was a problem hiding this comment.
Change it back.
No need for typing.
There was a problem hiding this comment.
data classes require you to do typing. (it just doesn't work if we don't)
needless to say, it is a much more intuitive way of making classes which are data holder.
did some stuff to increase readability and code quality:
Also added some functionality:
(todo:
. use these to refer to roles instead of strings
. add all roles)