Skip to main content
whoops
Source Link
Carcigenicate
  • 16.3k
  • 3
  • 35
  • 80

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.

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.

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.

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.

added 6 characters in body
Source Link
Carcigenicate
  • 16.3k
  • 3
  • 35
  • 80

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.

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 aan option is available. I'll note too that I didn't test this; I eyeballed it. It looks like it should be equivalent though.

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.

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 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 a option is available. I'll note too that I didn't test this; I eyeballed it. It looks like it should be equivalent though.

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.

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.

Source Link
Carcigenicate
  • 16.3k
  • 3
  • 35
  • 80

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.

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 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 a option is available. I'll note too that I didn't test this; I eyeballed it. It looks like it should be equivalent though.