11
\$\begingroup\$

I've written a program to simulate a game of Mafia among several bots. Here's the directory structure the program lives in (to make understanding parts of the program easier):

/
 start
 README.md # Readme file
 wtfpl.txt # License
 controller/
 main.py
 messages.py # Contains a list of strings. I only put it into
 # a separate file because it's long.
 players/
 log
 player_folder/ # Every player has one of these
 to_server
 from_server
 players
 run

main.py is the actual program that I'm looking for feedback with, and it is run by running ./start in the root folder (which just executes controller/main.py, nothing fancy).

I have a few concerns, most of which come down to "good style". I tried to follow PEP8 where appropriate (with the exception of tabs vs. spaces, because TABS FOREVER) but I'm sure there are places I screwed that up. Largely, though, I'm just concerned because I'm not an incredibly experienced programmer and a not-insignificant portion of that already limited experience is code golf, so I have almost no idea of how to make an actual real program not suck. I'm just hoping people can take a look at this and suggest ways to make it "better", for whatever definition of better they choose.

I think I've commented heavily enough/chosen good enough variable and function names to explain what the program actually does, but if you have any questions I'd be happy to answer. Additionally, I know I'm not supposed to edit this question with updates as I make them, but if you want to know what my code looks like right now I'll be updating the git repo here with any updates as I make them.

Finally, one last thing: I use some try: ... except: blocks that blindly catch any exception and ignore it. I'm aware of how terrible this is, however, these bots are going to be written by other people, and I want to be catching any error at all and defaulting to "ignore the input", which is why I've done that.

#!/bin/python2
import os
import sys
import random
import textwrap
from messages import messages
class Player(object):
 """Player object for each bot."""
 vote = None
 role = 0
 # Messages to the player are stored in the player object themself until they
 # are sent, allowing more than one message to be sent with relative ease.
 messages = ''
 def __init__(self, name):
 self.name = name
 def add_message(self, s):
 self.messages += textwrap.dedent(s + '\n')
 def get_role(self):
 if self.role == 1:
 return 'a mafioso'
 if self.role == 2:
 return 'the cop'
 if self.role == 3:
 return 'the doctor'
 return 'a villager'
def m_read(p):
 """Return contents of player p's 'to_server' file, stripped of special
 characters, and clear the file.
 """
 with open(p.name + '/to_server', 'r+') as f:
 s = filter(lambda c: c.isalnum() or c == ' ', f.read())
 f.truncate(0)
 return s
def m_write(players):
 """Write messages to each player to that player's 'from_server' file"""
 # Convert Player object to a 1-element list to allow calling for a single
 # player
 if isinstance(players, Player):
 players = [players]
 for p in players:
 with open(p.name + '/from_server', 'w') as f:
 f.write(p.messages)
 p.messages = ''
def execute(p):
 """Executes the bot associated with player p"""
 os.chdir(p.name)
 os.system('./run')
 os.chdir('..')
def log(message):
 """Append message + newline to the log file. This happens a lot, so this
 function exists as shorthand.
 """
 with open('log', 'a') as f:
 f.write(textwrap.dedent(message + '\n'))
def get_players(players):
 """Return a list of player objects for each player name in the input. Also
 return the doc, cop and a list of the mafia seperately.
 """
 def assign_roles(players):
 # Shuffling the whole list ensures you aren't assigning multiple roles to one
 # player, which we would have to account for with random.choice()
 random.shuffle(players)
 # Make 1/3rd of the players mafia, one player a doctor and one player a cop
 for _ in xrange(len(players) / 3):
 players[0].role = 1
 players.append(players.pop(0))
 players[0].role = 2
 players.append(players.pop(0))
 players[0].role = 3
 # Return list of players, list of mafia, cop and doctor
 return (players,
 filter(lambda p: p.role == 1, players),
 players[-1],
 players[0])
 # At least six players are required. Ensure that we have six. Do this by 
 # testing for at least seven, because...
 if len(players) < 7:
 sys.exit('Not enough players.')
 # ...The log file is kept in the same directory as the players. Get rid of it
 players.remove('log')
 # Convert to Player object, assign roles, and return
 return assign_roles(map(lambda p: Player(p), players))
def kill(p, players, mafia, cop, doctor):
 """Return every role the player p might be filling, with them removed."""
 players.remove(p)
 if p in mafia:
 mafia.remove(p)
 if cop is p:
 cop = None
 if doctor is p:
 doctor = None
 return players, mafia, cop, doctor 
def main():
 # Get player objects for all players, the doc, the cop, and a list of mafia
 os.chdir('players')
 players, mafia, cop, doctor = get_players(os.listdir('.'))
 # Give everyone a list of players
 for p in players:
 with open(p.name + '/players', 'w') as f:
 # Sort it so that it isn't ordered by role
 f.write('\n'.join(sorted([l.name for l in players])))
 # Clear the log file, so it's fresh for the new game
 with open('log', 'w') as f:
 f.truncate(0)
 # Create a dictionary allowing you to look up player objects by their name
 name_to_player = dict(map(lambda p: (p.name, p), players))
 day = 0
 # Day 0 doesn't have a suspect or victim, every subsequent day does
 suspect, victim = None, None
 # Game loop, exits when mafia is dead or mafia outnumbers village
 while mafia and (len(players) - len(mafia)) > len(mafia):
 log('Day {} begins.'.format(day))
 # Randomize turn order every day. Bots /shouldn't/ be able to figure
 # this out, but who knows what you crazy kids will come up with. :P
 random.shuffle(players)
 # Print a message at the beginning of each day. On the first day, power
 # roles need to be old their role and mafia members need to know their
 # allies. On every other day, the cop needs to know the result of their
 # investigation and all players need to know who died.
 if day == 0:
 log("""\
 Cop: {}
 Doctor: {}
 Mafia: {}""".format(
 cop.name, doctor.name, ', '.join(m.name for m in mafia)))
 for p in players:
 p.add_message("""\
 Rise and shine! Today is day 0.
 No voting will occur today.
 Be warned: Tonight the mafia will strike.""")
 for m in mafia:
 m.add_message("""\
 You are a member of the mafia.
 Your allies are:""")
 m.add_message('\n'.join(p.name for p in mafia if p is not m))
 cop.add_message('You are the cop.')
 doctor.add_message('You are the doctor.')
 else:
 for p in players:
 p.add_message('Dawn of day {}.'.format(day))
 if victim is not None:
 p.add_message('Last night, {} was killed.'.format(victim.name))
 if victim is not None:
 players, mafia, cop, doctor = kill(victim, players, mafia, cop, doctor)
 log('{}, {}, was killed.'.format(victim.name, victim.get_role))
 if suspect is not None:
 cop.add_message('Investigations showed that {} is {}-aligned.'.format(
 suspect.name, 'mafia' if suspect.role == 1 else 'village'))
 log('These players are still alive: {}'.format(
 ', '.join(p.name for p in players)))
 m_write(players)
 # During a day, players may perform up to 50 actions (Action= vote or talk)
 for r in xrange(50):
 for p in players:
 try:
 execute(p)
 command = m_read(p).split()
 if command[0] == 'vote':
 # Set the player's vote
 if day != 0:
 if command[1] == 'no':
 if command[2] == 'one':
 p.vote = None
 log('{} has voted to lynch no one.'.format(p.name))
 for l in players:
 l.add_message('{} has voted to lynch no one.'.format(p.name))
 else:
 p.vote = name_to_player[command[1]]
 log('{} has voted to lynch {}.'.format(p.name, command[1]))
 for l in players:
 l.add_message(
 '{} has voted to lynch {}.'.format(p.name, command[1]))
 elif command[0] == 'say':
 # Send a message to all players
 message = '{} says "'.format(p.name)
 # Messages with an id higher than 4 have the name of a bot attached
 # This screws with parsing a bit so we handle them seperately
 if int(command[1]) > 4:
 if len(command) == 4:
 # Convert from a name to a player object and back to ensure
 # that it's a correct name
 message += '{}, '.format(name_to_player[command[3]].name)
 message += messages[int(command[1])]
 message += '{}"'.format(name_to_player[command[2]].name)
 else:
 if len(command) == 3:
 message += '{}, '.format(name_to_player[command[2]].name)
 message += '{}"'.format(messages[int(command[1])])
 log(message)
 for l in players:
 l.add_message(message)
 except:
 # Do nothing on invalid input
 pass
 m_write(players)
 # Tally up the votes for each player
 votes = [p.vote for p in players]
 # Shuffle to eliminate max() bias
 random.shuffle(votes)
 # The most voted player is lynched, with ties broken randomly
 lynched = max(votes, key=votes.count)
 if lynched is not None:
 log('The town has killed {}!'.format(lynched.name))
 log('They were {}.'.format(lynched.get_role()))
 for p in players:
 p.add_message("""\
 The town has killed {}!
 They were {}.""".format(lynched.name, lynched.get_role))
 players, mafia, cop, doctor = kill(lynched, players, mafia, cop, doctor)
 else:
 log('The town opted to lynch no one today.')
 for p in players:
 p.add_message('The town opted to lynch no one today.')
 m_write(players)
 for p in players:
 execute(p)
 p.vote = None
 # Don't go to night if a win condition's been met.
 if not mafia or (len(players) - len(mafia)) <= len(mafia):
 break
 # Day ends, night begins
 # MAFIA NIGHT ACTION
 # Each mafioso votes for a victim. The most voted player is then killed,
 # unless saved that night by the doctor.
 for m in mafia:
 m.add_message('It is night. Vote for a victim.')
 m_write(mafia)
 victim_votes = [] 
 for m in mafia:
 try:
 execute(m)
 victim_votes.append(name_to_player[m_read(m)])
 log('{} votes to kill {}.'.format(m.name, victim_votes[-1].name))
 except:
 # Vote to kill no one on invalid input
 victim_votes.append(None)
 log(m.name + ' votes to kill no one.')
 # Shuffle to eliminate max() bias
 random.shuffle(victim_votes)
 # The victim is the player most voted for by the mafia, with ties broken
 # randomly.
 victim = max(victim_votes, key=victim_votes.count)
 log('The mafia collectively decides to kill {}.'.format(
 victim.name if victim is not None else 'no one'))
 # COP NIGHT ACTION
 # The cop chooses a player to investigate. At the dawn of the next day,
 # they are told whether that player is village- or mafia-aligned.
 if cop is not None:
 cop.add_message('It is night. Who would you like to investigate?')
 m_write(cop)
 try:
 execute(cop)
 suspect = name_to_player[m_read(cop)]
 log('{} spends the night investigating {}.'.format(
 cop.name, suspect.name))
 except:
 # Investigate no one on invalid input
 suspect = None
 log('{} chooses not to investigate anyone.'.format(cop.name))
 # DOCTOR NIGHT ACTION
 # The doctor chooses a player they expect the mafia to try to kill. If they
 # are right, the mafia gets no kills that night.
 if doctor is not None:
 doctor.add_message('It is night. Who would you like to save?')
 m_write(doctor)
 try:
 execute(doctor)
 patient = name_to_player[m_read(doctor)]
 if patient == victim:
 victim = None
 log('{} was able to save {} from near-certain death.'.format(
 doctor.name, patient.name))
 else:
 log('{} tried to save {}, but they were not the target.'.format(
 doctor.name, patient.name))
 except:
 # Save no one on invalid input
 log('{} took tonight off.'.format(doctor.name))
 log('')
 day += 1
 if mafia:
 print 'MAFIA VICTORY'
 log('MAFIA VICTORY')
 else:
 print 'VILLAGE VICTORY'
 log('VILLAGE VICTORY')
if __name__ == '__main__':
 main()
syb0rg
21.9k10 gold badges113 silver badges192 bronze badges
asked Apr 21, 2014 at 21:32
\$\endgroup\$
1
  • \$\begingroup\$ Love the code looks awesome. You going to compile to exe so we can all play it? \$\endgroup\$ Commented Apr 30, 2014 at 15:53

1 Answer 1

8
\$\begingroup\$

Overall, it looks pretty good to me. I agree that you have good commenting and variable and function naming. I see just a few things to comment on.

  • You have assign_roles() nested inside get_players(). Is there a reason for doing this? Unless there's a good one, I'd recommend putting assign_roles() at the top level along with everything else.

  • Could you put the log file someplace like '/tmp/log' so it doesn't mess up your list of players? Besides, what if you have a player named 'log'? Also, you might consider looking into python's logging module. It can be a bit complex, but provides a lot of nice features for managing log files.

  • I see an opportunity for further modularizing your main() routine -- maybe routines like day_action(), maybe_lynch_somebody(), mafia_night_action(), cop_night_action(), and doctor_night_action(). It might involve passing your lists around some, but you're already doing that up above, so it shouldn't be an issue.

Looks like an interesting game.

answered Apr 22, 2014 at 23:59
\$\endgroup\$
7
  • \$\begingroup\$ 1. I was under the impression that are only ever called from inside another function are generally kept inside that other function for clarity reasons (with the exception of main(), of course). If I'm mistaken, I can fix that easily. 2. That's true, I could. Having log under players/ is an artifact of before I made the log() function. I was writing to it with the whole with open(... syntax each time and didn't to navigate to a different folder each time. Now that I almost always do it from the function, that wouldn't be an issue at all. I didn't know python had a logging module... \$\endgroup\$ Commented Apr 23, 2014 at 8:14
  • \$\begingroup\$ ...but I'll check it out! 3. That's true, I should do that. If I'm being honest with you, I was starting to near the end of the project when I got there and having it all dumped into main() is probably an artifact of me growing lazy. :P Thank you for your help! \$\endgroup\$ Commented Apr 23, 2014 at 8:16
  • \$\begingroup\$ You're welcome. 1. With regard to nested functions, it may just be a matter of preference on my part. I think it's easier to find things when they're all at the top level. PEP20, The Zen of Python (legacy.python.org/dev/peps/pep-0020) does say, "Flat is better than nested." However, if nesting works well for you to indicate that the subordinate function is only called by its parent, I wouldn't stop doing that just because some guy on the internet says to. Now, if twenty guys on the internet say so, that's something else again. :) Let's see if anybody else has an opinion. \$\endgroup\$ Commented Apr 23, 2014 at 10:37
  • \$\begingroup\$ 2. For putting the log file in /tmp, I didn't mean to suggest that you should os.chdir("/tmp") before opening the file. Just use the full path in the open statement: open('/tmp/log', 'a'). That will work from wherever you are. \$\endgroup\$ Commented Apr 23, 2014 at 10:40
  • \$\begingroup\$ re: nested functions: I'm going to be honest and say that the coding style opinions of a guy who actively posts answers on codereview.SE probably trumps those of a guy who did a CodeAcademy course and uses reddit. :P \$\endgroup\$ Commented Apr 23, 2014 at 12:03

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.