8
\$\begingroup\$

I am writing a python code to recursively traverse a directory containing only text files and then print the 10 digit Indian phone number and all its variants. I am very new to python and I have written the following code to print the numbers and this code is working fine. I want to know if there is any other better way to do this program so that the code looks better. Right now, I have used 3 nested for loops, I was wondering if I could reduce time complexity a bit. I am a beginner in python.

Contact_finder.py

import os, re

class ContactFinder:

    pattern = re.compile('^(?:(?:\+|0{0,2})91(\s*[\ -]\s*)?|[0]?)?[789]\d{9}|(\d[ -]?){10}\d$')

    def traverse(self,dir_path):
        for root, dirs, files in os.walk(dir_path):
            for file in files:
                filename = os.path.join(root, file)
                with open(filename) as fn:
                    for contact in fn:
                        if self.pattern.match(contact):
                            print contact


contact = ContactFinder()
contact.traverse('/home/sinscary/Documents/niki')

Please suggest me a better solution than this.

\$\endgroup\$
10
\$\begingroup\$

You don't need a class here at all. You only seem to use it as a namespace and for that just importing it from another file would be sufficient.

I would make pattern a global constant and give it a better name.

I would put the extracting of the phone numbers into a separate function for readability.

I would make both functions generators and make it the responsibility of the caller to actually print the contact (or do something else with it).

Lastly, I would use a if __name__ == "__main__": guard to allow importing these functions without executing the traversal.

import os, re class

PHONE_NUNBER = re.compile('^(?:(?:\+|0{0,2})91(\s*[\ -]\s*)?|[0]?)?[789]\d{9}|(\d[ -]?){10}\d$')

def get_contacts(filename, pattern):
    with open(filename) as fn:
        for contact in fn:
            if pattern.match(contact):
               yield contact

def get_contacts_recursive(dir_path, pattern):
    for root, dirs, files in os.walk(dir_path):
        for file in files:
            filename = os.path.join(root, file)
            for contact in get_contacts(filename, pattern):
                yield contact

def main():
    for contact in get_contacts_recursive('/home/sinscary/Documents/niki', PHONE_NUMBER):
        print contact

if __name__ == "__main__":
    main()
\$\endgroup\$
10
\$\begingroup\$

Explain your regex. Most of the actual logic of your code is buried in one long string. Use raw, multiline strings and the re.VERBOSE mode to allow you to include comments on what each part of the pattern matches. You could also simplify it; it seems odd to have both choices in an optional non-capturing group be optional themselves, for example.

Here's one way to rewrite it.

CONTACT_NUMBER = re.compile(r'''
    ^  # start of line
    (?:
      (?:\+|0{0,2})91(\s*[\ -]\s*)?  # country code
      |[0]?  # or leading zero
    )?
    [789]\d{9}  # ten digits starting 7, 8 or 9
    |(\d[ -]?){10}\d  # or eleven digits with a separator
    $  # end of line
''', re.VERBOSE)

To complement the other answer, Stop Writing Classes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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