Your use of y_coord
and range
is suboptimal here. You're iterating over a range(3)
and ignoring the produced number, while also using y_coord
as an iteration variable.
Just iterate over the range of numbers that you want from the start:
def check_surroundings(self, y_coord, x_coord):
enemies = []
for cur_y in range(y_coord - 1, y_coord + 2): # Exclusive end
if self.engine.check_inbounds(cur_y, x_coord - 1) and self.engine.check_inbounds(cur_y, x_coord + 2):
for entity in self.engine.techmap[cur_y][x_coord - 1:x_coord + 1]:
if entity and entity.team != self.team:
enemies.append(entity)
return enemies
The main benefit here is the boost to readability. Ideally, it should be easy to tell what a for
loop is iterating for just by looking it at it.
for entity in self.engine.techmap[cur_y][x_coord - 1:x_coord + 1]:
It's obvious that you're getting all the matching entities from techmap
, then doing something with each.
for _ in range(3):
Here though, you're ignoring the iteration variable, so it isn't clear what is actually changing each iteration. Superficially, it looks like you're just repeating the same body three times, which is odd. It takes a second step of thinking to realize that you're using y_coord
to iterate.
This is also arguably a good place to use a list comprehension (although 2D+ list-comprehensions usually tend a little on the ugly side). The point of your nested loops is to produce a filtered list from existing collections (the iterable returned by techmap
, and range
in this case). That is exactly the use-case for list comprehensions.
def check_surroundings(self, y_coord, x_coord):
return [entity
for cur_y in range(y_coord - 1, y_coord + 2)
if self.engine.check_inbounds(y_coord, x_coord - 1) and self.engine.check_inbounds(y_coord, x_coord + 2)
for entity in self.engine.techmap[cur_y][x_coord - 1:x_coord + 1]
if entity and entity.team != self.team]
You may want to save that into a variable then return the variable. It depends on your style. I can't say that I necessarily recommend using the comprehension here, but I thought I'd show that such an option is available. I'll note too that I didn't test this; I eyeballed it. It looks like it should be equivalent though.