Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to improve some code in order to get a better perfomance, I have to do a lot of pattern matching for little tags on medium large strings, for example:

import re
STR = "0001x10x11716506872xdc23654&xd01371600031832xx10xID=000110011001010\n"

def getID(string):
    result = re.search('.*ID=(\d+)', string)
    if result:
        print result.group(1)
    else:
        print "No Results"

Taking in mind the perfomance I rewrite it:

def getID2(string):
    result = string.find('ID=')
    if result:
        result += 3
        print string[result:result+15]
    else:
        print 'No Results'

Are there a better way to improve these approaches? Any comments are welcome...

share|improve this question
5  
Is this the actual code that you are trying to improve the performance of? As it stands, print will be more expensive then anything else and relative to that no other change will really matter. –  Winston Ewert Jan 17 '13 at 15:17
    
This a getter from a class that get the string at the constructor –  cespinoza Jan 17 '13 at 15:54
1  
If its a getter you should be referencing self and returning the answer. If you want help you need to show your actual code! As it stands this is totally useless. –  Winston Ewert Jan 17 '13 at 16:44
    
Also, what performance improvement do you need? What are possible use cases? There's not much more you can do since find("ID=") is O(n), n being the length of the string. You can get performance only by not traversing the whole string, which is not possible unless we know more about your date (ID always at the end?). –  Quentin Pradet May 3 '13 at 8:41

2 Answers 2

If you name your function getSomething(), I expect it to return a value with no side effects. I do not expect it to print anything. Therefore, your function should either return the found string, or None if it is not not found.

Your original and revised code look for different things: the former wants as many digits as possible, and the latter wants 15 characters. Combining the two requirements, I take it that you want 15 digits.

re.search() lets the regular expression match any part of the string, rather than the entire string. Therefore, you can omit .* from the beginning of your regular expression.

Here's how I would write it:

import re
STR = "0001x10x11716506872xdc23654&xd01371600031832xx10xID=000110011001010\n"

def getID(string):
    result = re.search('ID=(\d{15})', string)
    return result and result.group(1)

print getID(STR) or "No Results"
share|improve this answer
    
Out of interest, even though I prefer the look of your suggestion, do you think it will perform better than the find(...) option? My instinct is that RE's will be slower in general and the \d{15} will do character-checking for digits which the find solution does not do... –  rolfl Jan 10 at 12:43

You are trying to get what's after ID=?
I'm not sure about performance (you'll have to benchmark on your own data) but I would do this:

def get_id(string):
    try:
        id = str.split('ID=')[1].strip()
        return id
    except IndexError:
        return None

string = "0001x10x11716506872xdc23654&xd01371600031832xx10xID=000110011001010\n"
id = get_id(string)

This only works if there are 0 or 1 ID= parts. If there are 2 or more only the first one gets returned.

Edit:

This is how I would do benchmarks (I might be wrong, I don't have much experience in this):

>>> #string with escaped '\n' at the end:
>>> string = '0001x10x11716506872xdc23654&xd01371600031832xx10xID=000110011001010\\n'
>>> #original regex + getting the match object:
>>> timeit.timeit(stmt="re.search(r'.*ID=(\d+)', '{}').group(1)".format(string), setup="import re")
3.4012476679999963
>>> #original regex only:
>>> timeit.timeit(stmt="re.search(r'.*ID=(\d+)', '{}')".format(string), setup="import re")
2.560870873996464
>>> #alternative regex + match object (had to use c-style string formatting):
>>> timeit.timeit(stmt="re.search(r'ID=(\d{15})', '%s').group(1)" % string, setup="import re")
3.0494329900029697
>>> #alternative regex only:
>>> timeit.timeit(stmt="re.search(r'ID=(\d{15})', '%s')" % string, setup="import re")
2.2219171980032115
>>> #str.split:
>>> timeit.timeit(stmt="'{}'.split('ID=')[1].strip()".format(string))
1.127366863998759

I think these are the benchmarks for the relevant parts. This is on a CoreDuo MacBook Pro and your results may vary. It's been my experience that using methods from the standard library usually yields the best results. On the other hand, this is important only if you have a ton of data to process. Otherwise a more flexible but "slower" way of doing things might be better.

share|improve this answer
1  
Since the actual asker of the question is using non-existant value checking in his solution, it seems fair that the assumptions of your suggestion are very restrictive too.... but I can't believe this solution will be anything near as quick as any other alternatives so far. The str.split has to check every character in the string inclusing all of those chars after the first match.... it simply does not add up. +1 though for suggesting benchmarking.... which is the obvious review question I would ask... "What are the benchmarks?" –  rolfl Jan 10 at 12:48
1  
@rolfl I added some benchmarks... I think they are ok but please correct me if I'm mistaken. –  otonvm Jan 10 at 17:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.