Basic python dice simulator with log function

Question

I had created a dice simulator using nothing but functions, but I wanted to attempt one using a class for the dice just to help get a better understanding of classes. I'm still a little in the dark on OOP and I feel like it was poorly implemented here, but I achieved what I was going for and I feel like that's at least something. Any tips on improvement would be appreciated.

#!/usr/bin/env python
import time
from sys import exit
from random import randint
class Die(object):
 """
 Creates a die class for use with a dice rolling utility
 """
 d_sides = None
 def __init__(self, sides = 6):
 self._sides = sides
 self._value = None
 self.roll()
 def roll(self):
 """
 Generate a random integer based off the number of sides
 """
 self._value = randint(1, self._sides)
 return self._value
 def __str__(self):
 """
 Return string containing dice sides and result
 """
 return "Die with {} sides. Result : {}".format(self._sides, self._value)
def banner():
 """
 Print out in console upon running script
 """
 print "-" * 10
 print "Snake Eyes"
 print "-" * 10
def dice_cup():
 """
 Takes user input for number of sides and number of dice then creates
 dice objects respectively
 """
 global d_sides
 # Asks for a number of dice and verifies that it's a number
 while True:
 try:
 number = int(raw_input("How many dice?\n>"))
 break
 except(TypeError, ValueError):
 print "Please enter a number\n"
 # Asks for a number of sides and verifies that it's a number
 while True:
 try:
 sides = int(raw_input("How many sides?\n>"))
 d_sides = sides
 break
 except (TypeError, ValueError):
 print "Please enter a number\n"
 # Generates dice objects
 dice_roll = [Die(sides).roll() for _ in range(int(number))]
 return dice_roll
def print_dice(d):
 """
 Converts list of dice results into string and prints results and total
 """
 # Converts list of dice results into a string
 dice_str = ','.join(map(str, d))
 # Adds total from dice roll
 total = sum(d)
 print "You rolled: {}\nTotal: {}".format(dice_str, total)
def dice_log(sides, number, roll):
 dash = '-' * 4
 f = open("dice_log.txt","a")
 f.write('\n')
 f.write(time.strftime('%I:%M:%S'))
 f.write('\n')
 f.write(dash)
 f.write('\n{}d{}\n'.format(number, sides))
 f.write(dash)
 f.write('\n{}\n'.format(','.join(map(str, roll))))
 f.write(dash)
 f.write('\nTotal\n')
 f.write(dash)
 f.write('\n{}\n'.format(sum(roll)))
 f.close
def engine():
 global d_sides
 banner()
 raw_input("Press enter to begin.")
 while True:
 roll = dice_cup()
 print_dice(roll)
 dice_log(d_sides, len(roll) , roll)
 cont = raw_input("Return to roll again or quit to exit\n>")
 if cont.lower() == "quit":
 print "\nGoodbye."
 exit(0)
def main():
 engine()
if __name__ == "__main__":
 main()

Answer 1

An assortment of comments, more or less in order from top to bottom.

Die class

Random weirdness:

• You have a static data member d_sides you don't need.
• Your constructor also rolls the die; that seems weird. I don't buy a die and it rolls itself. That being said, if we consider the current face to be an inherent property of the die then maybe it makes sense.
• Depending on your answer to the above, __str__ is weird as the die won't always have a result. Even when it does have a result, its weird to me that we call it a result; its really just the face that is facing up, and a result is however we interpret that.
• This die assumes numeric, and that it steps by 1. There are dice that aren't numeric, or dice that are numeric but the interval is greater than 1. You could make the sides a sequence, and then use random.choice.

I think your naming could use a bit of work; in this domain I'd think of it in terms of faces and the current face, instead of sides and the current value.

• Instead of self._sides, maybe say self._num_faces or something?
• Instead of self._value, I would prefer something like self._current_face.

Game driver

• In dice_cup you don't restrict the number of sides/dice to be a positive integer, and you just round their number if its a float.
• You assign the number of sides to sides, and then turn it around and assign it to d_sides.
• You validate numbers twice; write a function that takes a prompt as the input and validates a user input as a number
• If you aren't actually going to save the instances of the classes (you just save the values of the roll in dice_roll) then I don't see a reason to make it a class; just write a function that takes the number of sides.
• You turn number into an int; it already is one.
• Consider using xrange instead of range in Python 2
• You don't need to add "\n" at the end of your strings when printing; I'd prefer just a blank print statement
• Don't make d_sides a global; instead return it
• Don't use sys.exit. It is unnecessarily complicated for what you want to do here. Just end your loop and it'll exit on its own.
• Prefer context managers; they'll close themselves for you. with open("dice_log.txt", "a") as f: # do stuff with f
• You wanted to make a class; there are plenty of opportunities in the engine logic for that. For example, a Logger class to log game events, an Engine class to move the pieces, a DiceCup class to hold groups of dice and their associated properties.

---

Responding to your comments

OOP

If you do want to do this as an exercise in OOP, the first thing is to figure out what state belongs to each object, and how they should interact. Here is a potential list of "states":

• Die
  • Maintains the state of the dice, i.e. the available faces and what the current face is
• DiceCup
  • Maintains the state of a group of dice. Includes each of their current roles, and maybe some metadata (i.e. the total current value)
• Logger
  • Is responsible for logging output to a given location. Could be subclassed to be a prompt logger and a file logger, for example
• Engine
  • Is responsible for overall game state.

Let's look at the first section. I'll also try to work in my previous comments.

class Die(object):
 """A basic die object."""
 def __init__(self, faces):
 self.faces = faces
 self.current_face_index = None
 @property
 def num_faces(self):
 return len(self.faces)
 @property
 def current_face(self):
 if self.current_face_index is None:
 # You could throw an exception here, but I'll just roll for simplicity
 self.roll()
 return self.faces[self.current_face_index]
 def roll(self):
 self.current_face_index = random.randint(1, self.num_faces)
class StandardDie(Die):
 """A die object that only supports numeric 6-sided dice."""
 def __init__(self):
 super(StandardDie, self).__init__(range(1, 7))

I've introduced a few things here: properties and inheritance.

The details of how they work isn't super important, but here are the highlights:

• Properties let you access a value by calling a method, without it looking like you call a method. In this example, to get the number of faces you'd use my_die.num_faces, not my_die.num_faces(). This is useful for things that you don't want to store and are cheap to compute, or things that might not be initialized right away (like the current face).
• Inheritance lets you specialize part of the class, while keeping a lot of the other behavior. To do this I used the super function; basically all that does is hook up our subclass (StandardDie) and the superclass (Die).

Now that we have our die, we can get groups of them.

class DiceCup(object):
 """A group of die."""
 def __init__(self, dice):
 self.dice = dice
 @property
 def num_dice(self):
 return len(self.dice)
 def roll_all(self):
 for die in self.dice:
 die.roll()
class HomogeneousDiceCup(DiceCup):
 """A group of homogeneous die."""
 def __init__(self, factory, num_dice):
 super(HomogeneousDiceCup, self).__init__([factory() for _ in xrange(num_dice)])
class StandardDiceCup(HomogeneousDiceCup):
 """A group of standard dice."""
 def __init__(self, num_dice):
 super(StandardDiceCup, self).__init__(StandardDie, num_dice)
 @property
 def total(self):
 return sum(die.current_face for die in self.dice)

Now we've added a few types of dice cups. A dice cup lets you know how many dice there are and lets you roll them all, but you don't know anything about the relationship between the dice so it doesn't let you take the total, for example. Then a standard dice cup restricts you to a certain number of "standard" (6-sided, numeric 1-6) dice. Because we know what kind of dice there are, this type of dice cup also lets you get the total of all the dice.

Now lets talk about logging output

class Logger(object):
 """Logs data to a stream."""
 def __init__(self, stream):
 self.stream = stream
 def write(self, text):
 self.stream.write(text)
 def open(self):
 raise NotImplementedError()
 def close(self):
 raise NotImplementedError()
 def __enter__(self):
 return self.open()
 def __exit__(self):
 self.close()
class FileLogger(Logger):
 def __init__(self, filename, mode):
 self.filename = filename
 self.mode = mode
 def open(self):
 super(FileLogger, self).__init__(open(filename, mode))
 return self.stream
 def close(self):
 self.stream.close()

The only new concept here is of context managers; basically this lets us use a logger as

with FileLogger("myfile.txt", "w") as f:
 f.write("stuff")

and it'll make sure to close the file for us. There are a few ways we can use this; I'm going to use it as simply as possible in the Engine

def isPositive(number):
 return number > 0
class Engine(object):
 def __init__(self, logger):
 self.dice = None
 self._keep_playing = None
 self.logger = logger
 @property
 def keep_playing(self):
 if self._keep_playing is None:
 self._keep_playing = True
 else:
 self._keep_playing = self._prompt('Type "quit" to exit. ', None, lambda x: x.lower() == "quit")
 return self._keep_playing
 def play(self):
 self.banner()
 while self.keep_playing:
 num_dice = self.get_num_dice()
 num_sides = self.get_num_sides()
 self.dice = NumericDiceCup(num_dice, num_sides)
 self.dice.roll_all()
 self.print_dice()
 self.log_result()
 self.goodbye()
 def banner(self):
 print "-" * 10
 print "Snake Eyes"
 print "-" * 10
 def get_num_dice(self):
 return self._prompt('How many dice? ', int, isPositive)
 def get_num_sides(self):
 return self._prompt('How many sides? ', int, isPositive)
 def print_dice(self):
 print "You rolled: {}".format(','.join(str(die.current_face) for die in self.dice.dice))
 print "Total: {}".format(self.dice.total)
 def log_result(self):
 dash = '-' * 4
 with self.logger as logger:
 logger.write('\n')
 logger.write(time.strftime('%I:%M:%S'))
 logger.write('\n')
 logger.write(dash)
 logger.write('\n{}d{}\n'.format(number, sides))
 logger.write(dash)
 logger.write('\n{}\n'.format(','.join(map(str, roll))))
 logger.write(dash)
 logger.write('\nTotal\n')
 logger.write(dash)
 logger.write('\n{}\n'.format(sum(roll)))
 def _prompt(self, prompt, transformer, validator, error_message):
 if transformer is None: 
 transformer = lambda x: x
 while True:
 result = transformer(raw_input(prompt))
 try:
 is_valid = validator(result)
 except Exception:
 is_valid = False
 if is_valid:
 return result

This uses some higher order functions and lambda expressions, but overall it basically uses the same concepts as described above. At this point, your game is as easy as

if __name__ == "__main__":
 logger = FileLogger("dice_log.txt", "a")
 game = Engine(logger)
 game.play()

Do note that I didn't test any of it; there are almost inevitably some bugs or bits that aren't perfect style. Also note that this is an incredibly over-engineered design for what we're accomplishing; it might makes sense if you were developing a generic dice-rolling framework, but this should give you some ideas of how to structure your object oriented programming.

• Daniel: Using sys.exit(0) is a very clean way to exit the program, why shouldn't OP use that?
• Dan Oberlam: @Coal_ there's literally no reason to use it. Just end the loop and let it exit itself.
• Daniel: I think you should expand a bit on that point. Saying 'Don't use X', without further explanation, signifies that it is in some way harmful or bad practice. Using sys.exit may be desirable for more complex programs.
• Dan Oberlam: @Coal_ done. In general though, I find that sys.exit is more dangerous than useful. For any non-trivial application there is likely some type of "clean" exit strategy; whether its cleaning up variables or closing files, etc. sys.exit is a convenient way to jump out of the middle of a program and exit blindly, which is almost never what you want. If you're finding it too hard to exit from a certain point in your program the program is likely not designed as well as it should be or your program shouldn't be exiting from that point.
• Dan Oberlam: I do in general consider sys.exit bad practice, and I don't think I've ever seen a use case at my company in code review where we didn't reject the proposed usage.

Answer 2

There's some things I really like about your code. For one, you used str.format, which is in many ways superior to '%'-formatting. You also included docstrings, which help others better understand your code.

The other answers already covered most major problems. I'll be answering mostly in terms of design choice.

Object oriented programming (and why it doesn't apply here)

Classes should be thought of as containers that capture 'state'. Methods can act on that state and properties can be used to query it. A die, however, doesn't have any sense of state. A good rule of thumb is that if a class has only two methods, one of which is the constructor, then it shouldn't be a class.

I'd go further and say that you shouldn't consider creating a class if you are not confident that there is a sense of state in your program that could be grouped together. If you want to know more, there's this well known talk on when not to use classes.

Python 3!

Python 3 rocks, and you should definitely start using it right now. Python 2 only has so much time left, and switching to Python 3 isn't that hard. The only changes that affect this code:

• Python 3 got rid of raw_input and replaced it with input.

• print is now a function, not a statement.

If you absolutely, positively feel like you need to maintain backwards compatibility with Python 2:

try:
 input = raw_input
except NameError:
 # Python 3; `raw_input` is not defined
 pass

• Noved: Thank you! After navigating through various examples and information I decided that I prefer .format as well and I'm glad that you approve. I will check out the link. I had somewhat arrived at the idea that OOP might not be ideal for this, as I had already accomplished the general idea, which I was quite happy with, without it...but I wanted to give it a shot. I can understand that it might not be the best learning experience using it in a situation where it may not apply. I'm strongly considering switching to 3 and your suggestion might have knocked me off the fence.

Answer 3

My first thought is that in your Die class, you use self.value but you could just make it so that your Roll function returns directly a number. At least that is how I'd do it. There is nothing wrong with how you did it but you don't really use this attribute so you might not use it at all.

Second thought is you use global and that's generally not ideal. You may want to avoid that, because globals tends to make code less readable and less clear long term.

You could return multiples values in your dice_cup function (see here for informations), and pass that value to your other functions.

Third thought is that overall you code is readable, you did put some documentation, good job! You could consider adding some more information in your documentation type of return value, or type of arguments if it makes any sense.

Good job!

• Noved: Thank you for your response. I had completely forgotten about returning multiple values, and I believe that would have helped me with the part that troubled me most with this, and there would be absolutely no need to have the global variable.
• Julien Rousé: No problem, you did a pretty good job already, all my remarks were a bit of nitpicking. Also read what @Coal_ said about Python3, you should just be using it ^^