Basic terrain generator

Question

I've decided to try and start programming using Object-Oriented-Programming, so I built a small terrain generator that could be done using something as simple as this:

import random
for _ in range(1000):
    print random.choice('*#~o'),

Instead, I decided to have a go at OOP and made it like this instead:

# Terrain generator
from random import choice
from time import sleep


# Class containg world attr.
class Attr(object):
    def __init__(self):
        self.TILES = '#*~o'
        self.WORLD_SIZE = 1000
        self.Y_LENGTH = 1000
        self.X_LENGTH = 1000


# Generator class
class Generator(object):
    def __init__(self):
        self.world = []
        self.attr = Attr()

    # Create an empty row
    def create_row(self):
        for _ in range(self.attr.X_LENGTH):
            self.world.append(choice(self.attr.TILES))


# Main generator class
class MainGen(object):
    def __init__(self):
        self.gen = Generator()
        self.attr = Attr()

    # Create the world
    def create_world(self):
        for _ in range(self.attr.WORLD_SIZE):
            self.gen.create_row()

    # Render the world
    def render_world(self):
        for tile in self.gen.world:
            print tile,
            sleep(0.05)



# Main game class
class Game(object):
    def __init__(self):
        self.main_gen = MainGen()

    # Run the functions
    def run(self):
        self.main_gen.create_world()
        self.main_gen.render_world()


# Start the program
if __name__ == "__main__":
    game = Game()
    game.run()

All I'm really looking for are these things:

• What OOP mistakes did I make?
• What general mistakes did I make?
• How can I improve in general?

Answer 1

Separation of responsibilities

Consider the features you need and how you can group them into classes, and try to separate responsibilities as cleanly as possible. For example you need:

• Generate a terrain of given dimensions and set of tiles
• Render a generated terrain
• Glue code: configure the available classes/functions and do something with them

Here's one possible class design to meet these requirements:

• WorldGenerator: generates a world from given parameters

  • Constructor parameters: x length, y length, set of tiles
  • Public methods: generate (world)

• WorldRenderer: renders a given world

  • Constructor parameters: a world generator
  • Public methods: render (world)

• main: the glue code. This could be just a method, there's no point making it a class.

There's no place for an Attr class here. The attributes belong to the world generator, they can be encapsulated in it. It also makes sense to create different world generators with different attributes, instead of using constant attributes for all worlds.

In your original MainGen class, you were creating a world and rendering it. Creating a world should be the responsibility of the generator class, and it makes sense to separate from rendering anyway.

Suggested implementation

from random import choice


class WorldGenerator(object):
    def __init__(self, tiles='#*~o', x_length=1000, y_length=1000):
        self.world = []
        self.tiles = tiles
        self.x_length = x_length
        self.y_length = y_length

    def create_row(self):
        for _ in range(self.x_length):
            self.world.append(choice(self.tiles))

    def create(self):
        self.world = []
        for _ in range(self.y_length):
            self.create_row()


class WorldRenderer(object):
    def __init__(self, generator):
        self.gen = generator

    def render(self):
        for tile in self.gen.world:
            print tile,
            # sleep(0.05)


def main():
    gen = WorldGenerator(x_length=30, y_length=10)
    gen.create()
    renderer = WorldRenderer(gen)
    renderer.render()


if __name__ == "__main__":
    main()

Answer 2

You've gone from two easy-to-understand lines to six methods across three classes; hardly an efficient use of OOP. There is really no need to have the three classes Attr, Generator and MainGen, and they're so heavily interconnected as to suggest single class.

If you do want to use classes, I think you should have a World class, in which generate (covering Generator and MainGen) is a class method for creating a new world and the attributes (covering Attr) are a mix of class attributes and parameters to the class method. This would look like:

import random

class World(object):
    """The world in which the game takes place.

    Args:
      map (list of list of string): a representation
        of the world, a 2-D structure of single-
        character tiles. 

    Attributes:
      map (list of list of string): the map supplied
        in the argument to __init__
      TILES (string): the characters to use for tiles. 

    """

    TILES = '#*~o'

    def __init__(self, map):
        self.map = map

    def __str__(self):
        """Display whole map as 2-D grid."""
        return "\n".join("".join(row) for row in self.map)

    def render(self):
        """Generator to provide one tile at a time."""
        for row in self.map:
            for tile in row:
                yield tile

    @classmethod
    def generate(cls, shape):
        """Generate a new, random map.

        Args:
          shape (int or tuple of int): Either a two-tuple
            (x, y) or a single integer to use for both x
            and y defining the shape of the map to create. 

        """
        try:
            x, y = shape
        except TypeError:
            x = y = shape
        return cls([[random.choice(cls.TILES)
                     for _ in range(x)]
                    for _ in range(y)])

Then when you play the game:

world = World.generate(100)
for tile in world.render():
    ...

This separates the world itself out from how the game needs it displayed. As a simple example:

>>> world = World.generate(5)
>>> print(str(world))
oo#~#
*oo~#
#o#o~
*o*#o
**#*o

Answer 3

No need to have instances of Attr - it's just a class of static constants essentially. I'd change it to be more like:

class Attr(object):
    TILES = '#*~o'
    WORLD_SIZE = 1000
    Y_LENGTH = 1000
    X_LENGTH = 1000

# Generator class
class Generator(object):
    def __init__(self):
        self.world = []

    # Create an empty row
    def create_row(self):
        for _ in range(Attr.X_LENGTH):
            self.world.append(choice(Attr.TILES))


# Main generator class
class MainGen(object):
    def __init__(self):
        self.gen = Generator()

    # Create the world
    def create_world(self):
        for _ in range(Attr.WORLD_SIZE):
            self.gen.create_row()

    # Render the world
    def render_world(self):
        for tile in self.gen.world:
            print tile,
            sleep(0.05)