Which class should store the lookup table? [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 6 years ago.

Improve this question 开发者_StackOverflow

The world contains agents at different locations, with only a single agent at any location. Each agent knows where he's at, but I also need to quickly check if there's an agent at a given location. Hence, I also maintain a map from locations to agents. I have a problem deciding where this map belongs to: class World, class Agent (as a class attribute) or elsewhere.

In the following I put the lookup table, agent_locations, in class World. But now agents have to call world.update_agent_location every time they move. This is very annoying; what if I decide later to track other things about the agents, apart from their locations - would I need to add calls back to the world object all across the Agent code?

class World:
  def __init__(self, n_agents):
    # ...
    self.agents = []
    self.agent_locations = {}
    for id in range(n_agents):
      x, y = self.find_location()
      agent = Agent(self,x,y)
      self.agents.append(agent)
      self.agent_locations[x,y] = agent
  def update_agent_location(self, agent, x, y):
    del self.agent_locations[agent.x, agent.y]
    self.agent_locations[x, y] = agent
  def update(self): # next step in the simulation
    for agent in self.agents:
      agent.update() # next step for this agent
  # ...

class Agent:
  def __init__(self, world, x, y):
    self.world = world
    self.x, self.y = x, y
  def move(self, x1, y1):
    self.world.update_agent_location(self, x1, y1)
    self.x, self.y = x1, y1
  def update():
    # find a good location that is not occupied and move there
    for x, y in self.valid_locations():
      if not self.location_is_good(x, y):
        continue
      if self.world.agent_locations[x, y]: # location occupied
        continue
      self.move(x, y)

I can instead put agent_locations in class Agent as a class attribute. But that only works when I have a single World object. If I later decide to instantiate multiple World objects, the lookup tables would need to be world-specific.

I am sure there's a better solution...

EDIT: I added a few lines to the code to show how agent_locations is used. Note that it's only used from inside Agent objects, but I don't know if that would remain the case forever.

---

Okay, I think I can provide my answer, which is perhaps more an opinion than a definitive "do this" (I don't have any formal training in programming).

I think you agent_locations should be a member of each World instance.

I try to think in terms of interface primarily. In my view, the World class should be responsible for managing the resources of your world, in this case space. Since World is the manager of space, Agents should ask their world if space is available (i.e. unoccupied), not each other. Therefore, I think your self.location_is_good call more appropriately would be self.world.is_location_available(x, y) [1]

This makes it natural for the world to be responsible for looking up the availability of the given space. The World class might furthermore have other variables deciding whether a space is available. What if there's a shrubbery there? Or something. You probably already have some kind of table for your (x, y) coordinates on each world. Being "occupied" can be a property of those objects.

Furthermore: Your World already knows the state of every agent (by [(agent.x, agent.y) for agent in self.agents] [2]). The agent_locations dict is essentially an index or cache for these properties, which therefore belongs to World.

Regarding the pain of sending state back to the World... well, you're not going to solve that by making Agent do it instead. But doing update_agent_location(self, agent, x, y) is completely superfluous, since x == agent.x; y == agent.y (if you reverse the lines where you call it). You could simply have one method in World, update_agent_state(self, agent), which World can use to update its indices. You might even submit an extra param to describe the type of state change (if you don't want to update all the properties eveytime).

class World(object):
  # ...
  def update_agent_state(self, agent, state_change=None):
    # Update properties based on what changed, or
    # drop state_change param and update everything everytime
    if state_change == Agent.LOCATION_CHANGE:
      self.agent_locations[agent.x, agent.y] = agent
    elif state_change == Agent.WHATEVER:
      pass

class Agent(object):
  LOCATION_CHANGE = 1

  def update(self):
    for x, y in self.valid_locations():
      if not self.can_move_to(x, y)
        continue

      self.move(x, y)

  def can_move_to(self, x, y):
    """Determines if x, y is a location where we can move."""
    if not self.world.is_location_available(x, y):
      return False
    if not self.has_money_to_travel_to(x, y):
      return False

    return True

  def move(self, x, y):
    """Moves to x, y and notifies world of state change."""
    self.x = x
    self.y = y

    self.world.update_agent_state(self, Agent.LOCATION_CHANGE)

Something like that (read my footnotes).

[1] Unless, of course, "goodness" of a location depends upon other variables than if the space is free. E.g. if you should only move to (x, y) if 1) the location is available and 2) the agent has 1000 $ to pay for a ticket, then you should have a Agent.can_move_to(x, y) which in turn calls the world's method, and checks its wallet.

[2] I'm assuming your self.agents = {} is a typo, since you can't append on a dict. You mean a list ([]) right?

---

It helps with OOP to talk about the objects in terms of is a and has a. A World has a list of Agents and a list of Locations. A Location has an Agent. An Agent has a Location and a World.

class Agent:
    def __init__(self, world):
        self.location = None
        self.world = world

    def move(self, new_location):
        if self.location is not None:
            self.location.agent = None
        new_location.agent = self
        self.location = new_location

    def update(self):
        for new_location in self.world.locations:
            if self.location_is_good(new_location):
                self.move(new_location)

    def location_is_good(self, location):
        if location.agent is not None:
            return False

class Location:
    def __init__(self, x, y):
        self.x = x
        self.y = y
        self.agent = None

Go through the mental exercise of adding a new attribute to a Location, such as terrain, and it's easy to see the benefits of such encapsulation. Likewise, adding new things to Agent, such as a weapon, just requires a weapon-specific function similar to move(). Note that the World doesn't need to get involved in the move() at all. The move is handled strictly between the Agent and the Location.

---

Sorry, I don't understand the problem. """When the agent decides where to move, he checks to make sure he doesn't run into other agents""". Evidently a location can have 0 or 1 agent. Surely the Location class has an agent attribute.

locn = self.where_to_move()
if locn.agent is None:
    self.move(locn)
elif locn.agent is self:
    raise ConfusedAgentError()
else:
    self.execute_plan_B()