Your current solution sounds like a hack, for two reasons:
You are asking the agent for some information, but to do that you also have to issue commands like “turn left”. It is generally better to practice command–query separation since that makes it clearer when a method call will change the state of the object.
In itself, changing the agent's state to probe into different directions is not problematic since the agent will face into the same direction before and after the method call. However, changing object state intermittently introduces fragility: What happens when an exception is thrown before the agent has returned to their original orientation? Does the post-condition “the agent faces in the original direction” hold then as well? What happens if the number of turns is associated with some cost, or is tracked? E.g. if this is a puzzle game, the game score might consider the number of turns. Implementing the is-stuck check in terms of turning the agent might directly influence the game score.
Note that in Python, setting an instance field may very well have unknown side effects if that instance field is implemented as a property. In contrast, setting a member field is always “safe” in a language like Java.
The operations provided by the agent and the operations required in your program are mismatched. You want to know whether an agent is stuck, so it should have a method agent.is_stuck() -> bool
. Since that method is internal to the agent, it can directly use the same logic as agent.can_move_in_facing_direction()
with no need to modify the object state. You would probably extract the common logic into a helper method like self.__can_move_in_direction(d)
.
As a general design issue, using anything except function parameters to pass function parameters is not a good idea. Using instance fields is not fundamentally different from using global variables to communicate parameters. It's the moral equivalent of:
# params for add
x = None
y = None
result = None
def add():
result = x + y
# invoke add
x = 40
y = 2
add()
print(result) #=> 42
Does this have its uses? Yes, in space-constrained assembly programming where you have to prevent stack allocations. But in high-level languages? Not really.
The problem with global variables (or really, any variable with a large scope) is that it's increasingly difficult to keep track of how that variable is used. If I use a global-ish variable and call a function, I need to know whether the called function directly or indirectly modifies that variable. Keeping track of the exact behaviour of all those functions ranges from difficult (if there are many functions that could affect those variables) to impossible (if infinitely many functions could be added by inheriting your class). It's difficult to understand the behaviour of these functions since they don't just depend on their explicit parameters, but implicitly also on the state of the world around them.
Mutating state as little as possible and keeping any mutable state nicely encapsulated and divided into small cohesive chunks is not just a matter of “clean code”, it's fundamental to keep a large code base comprehensible.
can_move
? Are you immediately picking a possible direction and move that way? Or are you showing the user the possibilities he has? Something else? – Bart van Ingen Schenau Jan 25 '17 at 8:29can_move()
(for each direction), I return True if the agent cannot move anywhere (False otherwise). This information is then used to modify the agent's future behavior. So there's no direct connection between a call tocan_move()
and a movement. – max Jan 25 '17 at 8:39