How do I organize this code?

Question

I'm trying to sort functions into their appropriate classes by turning them into methods (it is possible to leave a function outside of the class if it seems appropriate). But I'm not sure what to do with this particular function (shown below).

First, here is my class:

class OreBlobAction:
   def __init__(self, entity, image_store):
      self.entity = entity
      self.image_store = image_store

   def ore_blob_action(self, world, action, ticks):
      entity = self.entity
      entity_pt = entities.get_position(self.entity)
      vein = find_nearest(world, entity_pt, entities.Vein)
      (tiles, found) = blob_to_vein(world, self.entity, vein)

      next_time = ticks + entities.get_rate(entity)
      if found:
         quake = create_quake(world, tiles[0], ticks, self.image_store)
         worldmodel.add_entity(world, quake)
         next_time = ticks + entities.get_rate(entity) * 2

      schedule_action(world, self.entity,
         OreBlobAction(self.entity, self.image_store), next_time)

      return tiles

And now, here is the function:

def take_action(world, action, ticks):
   entities.remove_pending_action(action.entity, action)
   if isinstance(action, VeinAction):
      return vein_action(world, action, ticks)
   elif isinstance(action, MinerNotFullAction):
      return miner_not_full_action(world, action, ticks)
   elif isinstance(action, MinerFullAction):
      return miner_full_action(world, action, ticks)
   elif isinstance(action, OreBlobAction):
      return ore_blob_action(world, action, ticks)
   elif isinstance(action, OreTransformAction):
      return ore_transform_action(world, action, ticks)
   elif isinstance(action, EntityDeathAction):
      return entity_death_action(world, action, ticks)
   elif isinstance(action, WyvernSpawnAction):
      return wyvern_spawn_action(world, action, ticks)
   elif isinstance(action, WyvernAction):
      return wyvern_action(world, action, ticks)
   elif isinstance(action, VeinSpawnAction):
      return vein_spawn_action(world, action, ticks)
   elif isinstance(action, AnimationAction):
      return animation_action(world, action, ticks)

   return []

As you can see, this function accounts for the actions of not only the OreBlobAction class, but also multiple others. Would it be better to leave this function outside of the OreBlobAction class? Or is there a better way to do this?

NOTE: If I leave this function out of the OreBlobAction class, and I try to run the program, I get this error:

NameError: global name 'ore_blob_action' is not defined

What's the point of that function? Why do you have all those separate functions, rather than calling the method of the Action subclass directly? — Daniel Roseman, Apr 09 '14 at 08:24
It was given to us like that, our job is to categorize the functions into appropriate classes by turning them into methods. Or, you can leave the functions out of the class if it seems more appropriate. I'm just really confused as to whether I should include this inside the OreBlobAction class or to leave it out. :( — , Apr 09 '14 at 08:26
Just turning them into methods isn't the point. The API is supposed to be [polymorphic](http://stackoverflow.com/questions/1031273/what-is-polymorphism); you should be able to take any sort of Action object, call the method named, say, `action`, and have it execute the appropriate code depending on the type of Action it is without having to test that it's a `VeinSpawnAction` and call `vein_spawn_action`. — user2357112, Apr 09 '14 at 08:29
Right, but if I leave it like this, I get the error I listed above. :( So I'm not sure what to do. — , Apr 09 '14 at 08:41
But no-one says to "leave it like this"! You should remove that function completely and put the actions into methods on the subclasses, as Brett says. — Daniel Roseman, Apr 09 '14 at 08:46
Ah, so I should separate out each case into their respective classes? — , Apr 09 '14 at 08:53

score 1 · Answer 1 · answered Apr 09 '14 at 08:25

Your big switch statement on the type of the action is a red flag for refactoring. Is there anything to stop you moving the "take action" method into the action class itself? For instance

class Action(object):
    """ An action that can be performed on the game world. """

    def perform(self, world, ticks):
        """ Perform the action. """
        raise NotImplementedError()

This would be your base action class, then within each type of action, you would override the perform(...) method, for example

class WyvernSpawnAction(Action):
    """ An action that spawns a Wyvern. """

    [... Some action specific initialisation code here ...]

    def perform(self, world, ticks):
        """ Spawn a Wyvern. """
        world.spawn(Wyvern(...))

Your boilerplate to remove the action from the world would remain, and you can now freely add new types of action without eventually adding hundreds more comparisons within your function. In addition, you can now handle cases where actions can inherit the behaviour of other actions without having to be extremely careful about the ordering of your comparisons.

How do I organize this code?

1 Answers1