1
\$\begingroup\$

I'm very new to programming and I don't feel confident about the readability of this program.

This program gets the text you copied then extracts phone numbers and email addresses in the text. Once those are extracted, you can paste them anywhere(Eg: notepad).

Are there any ways that I can improve this code? Any suggestions will surely be appreciated. Thanks!

import pyperclip, re
# Python 3 - 30/04/2020
# emailAndNumberExtractor.py - finds phone and email addresses on the clipboard

emailRegex = re.compile(r'''(
    [a-zA-Z0-9._%+-]+           # username
    @                           # @ symbol
    [a-zA-Z0-9.-]+              # domain name
    (\.[a-zA-Z]{2,4})           # dot something
    )''', re.VERBOSE)

phoneRegex = re.compile(r'''(
    (\d{3}|\(\d{3}\))?          # area code
    (\s|-|\.)?                  # separator
    \d{3}                       # first 3 digits
    (\s|-|\.)                   # separator
    \d{4}                       # last 4 digits
    (\s*(ext|x|ext.)\s*\d{2,5})?# extension
 )''', re.VERBOSE)

all_emails = "\nEMAILS FOUND:\n"
all_numbers = "\nNUMBERS FOUND:\n"

# get copied text and paste it to text var
text = str(pyperclip.paste())

# check if there are phone numbers/ emails in the text
if len(emailRegex.findall(text)) > 0 :
    for email in emailRegex.findall(text):
        all_emails += '\n\t' + email[0]
else:
    all_emails += "\n\tSorry, there are no emails."

if len(phoneRegex.findall(text)) > 0:
    for num in (phoneRegex.findall(text)):
        all_numbers  += '\n\t' + num[0]
else:
    all_numbers += "\n\tSorry, there are no phone numbers."

# Collects all numbers and emails found
matches = all_numbers + "\n" +  all_emails


print(text) 
pyperclip.copy(matches)
print("Copied to clipboard:")
print(matches)
```
\$\endgroup\$
1
\$\begingroup\$

There are a couple of pointers. I possibly got too ahead of myself with the solution, but hopefully, a step-by-step walkthrough clarifies everything. I am happy to answer any questions you have.

The overall suggestion is:

"""Email and number extractor - finds phone and email addresses on the clipboard.
Python 3 - 30/04/2020
"""
import re
from io import StringIO

import pyperclip


class PastingIO(StringIO):
    def write_items_to_string(self, items, items_name: str):
        self.write(f"{items_name} found:\n\n".upper())

        if items:
            for item in items:
                self.write(f"\t{item}\n")
        else:
            self.write(f"\n\tSorry, there are no {items_name}")

        self.write("\n")  # padding newline


def main():
    email_regex = re.compile(
        r"""(
            [a-zA-Z0-9._%+-]+           # username
            @                           # @ symbol
            [a-zA-Z0-9.-]+              # domain name
            (\.[a-zA-Z]{2,4})           # dot something
        )""",
        re.VERBOSE,
    )

    phone_regex = re.compile(
        r"""(
            (\d{3}|\(\d{3}\))?          # area code
            (\s|-|\.)?                  # separator
            \d{3}                       # first 3 digits
            (\s|-|\.)                   # separator
            \d{4}                       # last 4 digits
            (\s*(ext|x|ext.)\s*\d{2,5})?# extension
        )""",
        re.VERBOSE,
    )

    # get copied text and paste it to text var
    # text = str(pyperclip.paste())
    text = """
        This is sample text.
        hello@you.com
        There is nothing here.
        rofl@mao.org
        234-654-1234 is a telephone number.
        So is 123-456-7890!
        However, 1234-32-342 is invalid.
        Hello World!
        invalid@email.world should be invalid all-together.
        Check out this email: a@b.c
    """

    output = PastingIO()

    print("Working on:", text, sep="\n")

    matching_targets = {
        "numbers": {"regex": phone_regex,},
        "emails": {"regex": email_regex,},
    }

    for target_name, target_dict in matching_targets.items():
        regex = target_dict["regex"]
        target_dict["results"] = [match[0] for match in regex.finditer(text)]
        output.write_items_to_string(target_dict["results"], target_name)

    match_summary = output.getvalue()

    pyperclip.copy(match_summary)
    print("Copied to clipboard:", match_summary, sep="\n")


if __name__ == "__main__":
    main()

Walkthrough

"""Email and number extractor - finds phone and email addresses on the clipboard.
Python 3 - 30/04/2020
"""

is a module docstring. It basically supersedes the comments of the same content that you had in your code. The module docstring is more powerful. For example, other people can invoke it with help():

~$ python -c "import emailAndNumberExtractor; help(emailAndNumberExtractor)"
Help on module emailAndNumberExtractor:

NAME
    emailAndNumberExtractor

DESCRIPTION
    Email and number extractor - finds phone and email addresses on the clipboard.
    Python 3 - 30/04/2020

FUNCTIONS
    main()

FILE
    ~\emailandnumberextractor.py

import re
from io import StringIO

import pyperclip

I did not touch either the re import or your re.compile statements. You will have to decide for yourself if these are correct. I did however include a sample text to match against.

StringIO is used here as a sort of temporary file. When you look for and find matches, these matches should be collected, ideally in a mutable sequence like a list. This is your data. In your code, you irretrievably intertwine that data with its display, here the print of a str. You did this through string concatenation. But what if you would like to forward the list of found matches somehow, i.e. use it a second time, in another context? You cannot, because the data is mingled with the string.

As such, StringIO will be a virtual, in-memory file for us to write to. It will hold the formatting, i.e. indentations and newlines. The data will come from somewhere else and be kept separate. Since we do not need a real file, this will do. I chose this over multi-line string formatting, since that is not straightforward to do and has many caveats.


class PastingIO(StringIO):
    def write_items_to_string(self, items, items_name: str):
        self.write(f"{items_name} found:\n\n".upper())

        if items:
            for item in items:
                self.write(f"\t{item}\n")
        else:
            self.write(f"\n\tSorry, there are no {items_name}")

        self.write("\n")  # padding newline

This class definition might be the trickiest part. Don't worry if you haven't learned about classes yet. It can be understood this way:

PastingIO, our new custom class, is inheriting from StringIO. This means that PastingIO will have all the functionalities of its parent, StringIO. I explained the latter earlier. The reason I did this was to extend StringIO by a simple functionality. This functionality is the function write_items_to_string. If function definitions occur in classes, they are called methods.

Methods are much like normal functions, but since they are methods of a class, they usually do something with their class.

Now, doing something with the class itself, PastingIO, is not getting us far. See, a class is like a blueprint. It contains all the instructions of how something should look like and behave. But it is not yet something substantial. A class is like a construction plan, but we are interested in the house that will be built based on that plan. The plan itself is a (to us, useless) piece of paper.

To build the "house", instantiation is needed. This is done in the code when we call PastingIO(). The parantheses are important. It is the instruction to actually build an object from the class definition. Lastly, we give this thing a name, by just assinging it to a variable (output). More on that later.

The object we get is like StringIO, but with the added functionality. You will note that this functionality is much like the loops you defined to concatenate to the string. I created this method since those loops do identical things. Now, you won't have to repeat yourself anymore. This is in adherence to the DRY principle: don't repeat yourself.

As such, I hope write_items_to_string is self-explanatory. The self just refers to the object instance we created, output. It means that we act on output. In this case, imagine the write method to just write to a file, like you do with real files using with open("file.txt", "w") as f: f.write("Hello"), only virtually. It builds and holds our output string, including all the formatting.

Here, a class is important to hold the string. A function alone cannot (rather: should not) hold onto anything (referred to as a state).


Skipping the re.compile() statements, they are unchanged. The text statement is just a sample text to work on.


output = PastingIO()

This is the line mentioned above. output is now an instance of our custom PastingIO class. It has all the capabilities of a StringIO (much like a virtual text file), with the added write_items_to_string method, capable of modifying the content of output.


    matching_targets = {
        "numbers": {"regex": phone_regex,},
        "emails": {"regex": email_regex,},
    }

This is a nested dictionary. matching_targets is a dictionary, but each of its values is also a dictionary. This affords us a neat and organized way to store all related data and functionalities, without repeating ourselves.


    for target_name, target_dict in matching_targets.items():
        regex = target_dict["regex"]
        target_dict["results"] = [match[0] for match in regex.finditer(text)]
        output.write_items_to_string(target_dict["results"], target_name)

This is where, finally, the business happens. One key aspect I found in your code was that findall was called twice. You compiled the regex outside the loops, only once, which is great. But even findall only needs to be called once for each regex. Note that I replaced it with finditer, a simple iterator that does the same thing here. It returns matches only if you ask it to (lazy stuff), which we do in a loop.

(List) comprehensions are faster than the "manual" equivalent using for or while loops. The list at target_dict["results"] will hold all the found strings (well, only the first found capture group). Note that we iterate over matching_targets, and thus do both emails and numbers in one sweep. The results are then found in for example matching_targets["emails"]["results"], so two keys are required.

Note that this data would not strictly have to be stored in the dict, since we do nothing with it later on.

The last line calls the write_items_to_string method of output. At first, output is empty. We write to it, and it retains its contents across those loops, building up a virtual text file.

String concatenation in loops is usually a bad idea. Strings are immutable, that means even just appending a single letter will lead to the creation of an entirely new object. Lists are mutable. So a good alternative approach to what was done here would be to collect string components in a mutable sequence, like a list, and then joining them together once afterwards, using join.


match_summary = output.getvalue()

just gets the text contents of the output object. match_summary is now a string, printed to the specifications in write_items_to_string. If you want different newlines, indentations etc., look there.

Note that if you did not want each item on a newline, and would be happy with printing the "raw" output, that is suddenly much, much easier:

emails = ["hello@world.org", "a@b.c"]
result = f"Emails found: {emails}"
print(result)

is all that is needed:

Emails found: ['hello@world.org', 'a@b.c']

pyperclip.copy(match_summary)
print("Copied to clipboard:", match_summary, sep="\n")

Note that I saved a print call here by just separating the arguments with a comma. They are separated according to the sep argument.


if __name__ == "__main__":
    main()

This is a common pattern used to prevent the file from running when being imported. If a Python file, i.e. module, is run directly as a script, as is the case here, its __name__ attribute will be set to "__main__". As such, the main() function is executed, as desired.

However, notice how in the docstring explanation above, I imported your module to call help on it. In such a case, without the __name__ == "__main__" safeguard, the module would also run, which would then of course be undesired. As such, it is a good idea to keep modules importable.

\$\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.