Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/

Source Link

edited Apr 13, 2017 at 12:40

Community Bot

edited Apr 13, 2017 at 12:40

Community Bot

The overall body reads like an if tree, yet all scenarios are mutually exclusive. You can probably squeak a little better performance out by changing the second and later ifs to elifs. Make sure they're ordered in terms of descending frequency. If the number of possibilities will grow larger, consider other approaches that don't require a series of if tests. (At some point the overhead of testing each alternative will likely be more expensive than that of using alternate lookups such as draw method on various tile classes.)
The distance check loop in a res tile seems flawed. It executes a for loop, but only saves the result from the last element of othertiles. And for each element, it checks if othertiles is empty (aside: you should spell that if othertiles: rather than creating and comparing against an empty list every time). If you want the first result, you can fix both of these by using break and a for/else loop:
```
 for x in othertiles:
 # distance calculation
 break
 else:
 t.haspower = 0
```
(Alternately you could just set t.haspower = 0—or False—before the for loop, and let the for loop set it to 1—or True—if applicable.)
The distance calculation is computationally heavy. Like my comment on your previous post previous post, this is typically more relevant to C level code, but it's still good to remember that sqrt takes a while to calculate, so you can look for opportunities to avoid calling it. Consider that you compare Dist(...) to a constant 2500. If instead of calling sqrt you just returned the square of the distance, you could compare to 6250000 and have the same effect.
Calling the distance function, you tear t.pos and x.pos apart and piece it back together for a function that just uses their first two elements. You might save some effort, readability, and possibly gain a hint of performance by calling your distance function with just Dist(t.pos, x.pos).

The overall body reads like an if tree, yet all scenarios are mutually exclusive. You can probably squeak a little better performance out by changing the second and later ifs to elifs. Make sure they're ordered in terms of descending frequency. If the number of possibilities will grow larger, consider other approaches that don't require a series of if tests. (At some point the overhead of testing each alternative will likely be more expensive than that of using alternate lookups such as draw method on various tile classes.)
The distance check loop in a res tile seems flawed. It executes a for loop, but only saves the result from the last element of othertiles. And for each element, it checks if othertiles is empty (aside: you should spell that if othertiles: rather than creating and comparing against an empty list every time). If you want the first result, you can fix both of these by using break and a for/else loop:
```
 for x in othertiles:
 # distance calculation
 break
 else:
 t.haspower = 0
```
(Alternately you could just set t.haspower = 0—or False—before the for loop, and let the for loop set it to 1—or True—if applicable.)
The distance calculation is computationally heavy. Like my comment on your previous post, this is typically more relevant to C level code, but it's still good to remember that sqrt takes a while to calculate, so you can look for opportunities to avoid calling it. Consider that you compare Dist(...) to a constant 2500. If instead of calling sqrt you just returned the square of the distance, you could compare to 6250000 and have the same effect.
Calling the distance function, you tear t.pos and x.pos apart and piece it back together for a function that just uses their first two elements. You might save some effort, readability, and possibly gain a hint of performance by calling your distance function with just Dist(t.pos, x.pos).

The overall body reads like an if tree, yet all scenarios are mutually exclusive. You can probably squeak a little better performance out by changing the second and later ifs to elifs. Make sure they're ordered in terms of descending frequency. If the number of possibilities will grow larger, consider other approaches that don't require a series of if tests. (At some point the overhead of testing each alternative will likely be more expensive than that of using alternate lookups such as draw method on various tile classes.)
The distance check loop in a res tile seems flawed. It executes a for loop, but only saves the result from the last element of othertiles. And for each element, it checks if othertiles is empty (aside: you should spell that if othertiles: rather than creating and comparing against an empty list every time). If you want the first result, you can fix both of these by using break and a for/else loop:
```
 for x in othertiles:
 # distance calculation
 break
 else:
 t.haspower = 0
```
(Alternately you could just set t.haspower = 0—or False—before the for loop, and let the for loop set it to 1—or True—if applicable.)
The distance calculation is computationally heavy. Like my comment on your previous post, this is typically more relevant to C level code, but it's still good to remember that sqrt takes a while to calculate, so you can look for opportunities to avoid calling it. Consider that you compare Dist(...) to a constant 2500. If instead of calling sqrt you just returned the square of the distance, you could compare to 6250000 and have the same effect.
Calling the distance function, you tear t.pos and x.pos apart and piece it back together for a function that just uses their first two elements. You might save some effort, readability, and possibly gain a hint of performance by calling your distance function with just Dist(t.pos, x.pos).

Source Link

answered Nov 24, 2013 at 23:45

Michael Urman

answered Nov 24, 2013 at 23:45

Michael Urman

3.9k
1
12
23

Humans are bad at figuring out where the slow spots are. You will need to ask the computer by profiling your code. As rolfl said, you're likely better off saving this for when you have more of your code in place and it does enough work that it's actually slowing down.

It's quite possible most of your slowdown related to extra tiles comes from othertiles=[x for x in tiles if x.spr==power1 or x.spr==power2]; perhaps you should update othertiles incrementally as the board changes instead of recalculating it every pygame.event. Other than that, I'm going to concentrate on the inner for t in tiles loop. There are a couple things in there that stand out to me. They may or may not make a real difference to your code's performance.

The overall body reads like an if tree, yet all scenarios are mutually exclusive. You can probably squeak a little better performance out by changing the second and later ifs to elifs. Make sure they're ordered in terms of descending frequency. If the number of possibilities will grow larger, consider other approaches that don't require a series of if tests. (At some point the overhead of testing each alternative will likely be more expensive than that of using alternate lookups such as draw method on various tile classes.)
The distance check loop in a res tile seems flawed. It executes a for loop, but only saves the result from the last element of othertiles. And for each element, it checks if othertiles is empty (aside: you should spell that if othertiles: rather than creating and comparing against an empty list every time). If you want the first result, you can fix both of these by using break and a for/else loop:
```
 for x in othertiles:
 # distance calculation
 break
 else:
 t.haspower = 0
```
(Alternately you could just set t.haspower = 0—or False—before the for loop, and let the for loop set it to 1—or True—if applicable.)
The distance calculation is computationally heavy. Like my comment on your previous post, this is typically more relevant to C level code, but it's still good to remember that sqrt takes a while to calculate, so you can look for opportunities to avoid calling it. Consider that you compare Dist(...) to a constant 2500. If instead of calling sqrt you just returned the square of the distance, you could compare to 6250000 and have the same effect.
Calling the distance function, you tear t.pos and x.pos apart and piece it back together for a function that just uses their first two elements. You might save some effort, readability, and possibly gain a hint of performance by calling your distance function with just Dist(t.pos, x.pos).

Two more notes:

You probably don't want just the first or last distance to a power tile. Instead you likely want the closest. This might be easier to handle with an approach like distance = min(Dist(t.pos, x.pos) for x in othertiles)) if othertiles else 2501 or even better:
```
 t.haspower = False
 if othertiles:
 t.haspower = min(Dist(t.pos, x.pos) for x in othertiles)) < 2500
 # or distance_squared and compare to 6250000 per third bullet
```
It's unusual in python to name your functions with initial capital letters. When I read the line distance=Dist((t.pos[0],t.pos[1]),(x.pos[0],x.pos[1])) I assumed it was instantiating a class (which would probably be even heavier than calculating a square root). I would suggest renaming this use all lowercase letters.

lang-py