A little Python roguelike with mostly using standard libraries

Question

So, I made this in order to practice my Python OOP and algorithms skills, and refactored it a bit after a few months of s tale. May anybody review it in a PR please? Want to see the flaws that could be fixed, considering I didn't improve much since the initial development of this. https://github.com/fen0s/TABSlike/ One that especially makes me worries is the implementation of getting a 3x3 area around the unit to check for enemy units... Is it fine, and can you understand it overall?

def check_surroundings(self, y_coord, x_coord):
 y_coord -= 1 #to check y level above unit
 enemies = []
 for _ in range(3):
 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[y_coord][x_coord-1:x_coord+1]:
 if entity and entity.team != self.team:
 enemies.append(entity)
 y_coord+=1
 return enemies

Answer 1

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 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.