There's really nothing wrong with your code at all. It works, it's clear why it works, it's hard to get wrong or misunderstand.
Yes, you could make it faster, but only by a constant factor. (Your algorithm does a two reverse
s, for N
steps each, and one remove
, which is N-1
steps, so O(N)
. And since your data aren't sorted or anything that would help us find a value faster, it's obvious that the ideal algorithm would also be O(N)
.) And at the cost of making it more complicated.
The obvious probably-faster way to do it is to just manually iterate from the end until we find a value, then delete that value. That also avoids having to deal with the ValueError
. Using enumerate
might help… but getting it right (without copying the whole thing) may be tricky.
So, let's compare these to your existing code, both wrapped it in a try
/except
, and in an if
:
def f_rev_ex(xs, s):
xs.reverse()
try:
xs.remove(s)
except ValueError:
pass
xs.reverse()
def f_rev_if(xs, s):
if s in xs:
xs.reverse()
xs.remove(s)
xs.reverse()
def f_for(xs, s):
for i in range(len(xs)-1, -1, -1):
if s == xs[i]:
del xs[i]
break
def f_enum(xs, s):
for i, x in reversed(list(enumerate(xs))):
if x == s:
del xs[i]
break
For a list as tiny as yours, the test isn't even worth running, so I invented my own random data (in real life you have to know your data, of course):
In [58]: xs = [random.choice(string.ascii_lowercase) for _ in range(10000)]
In [59]: %timeit y = x[:]; f_rev_ex(y, 'a')
10000 loops, best of 3: 34.7 µs per loop
In [60]: %timeit y = x[:]; f_rev_if(y, 'a')
10000 loops, best of 3: 35.1 µs per loop
In [61]: %timeit y = x[:]; f_for(y, 'a')
10000 loops, best of 3: 26.6 µs per loop
In [62]: %timeit y = x[:]; f_enum(y, 'a')
1000 loops, best of 3: 604 µs per loop
Well, that last one wasn't a very good idea… but the other one is about 25% faster than what we started with. So we've saved a whole 9 microseconds, on data 4 orders of magnitude larger than your actual data. It's up to you whether that's worth the less-readable, easier-to-screw up code. (And I'm not going to show you my enumerate
-based implementation without copying, because I got it wrong. :P)