1
\$\begingroup\$

This script takes a CSV of numbers and processes them to check to see if the numbers conform to the UK standard 11 digit numbers. Any errors are outputted to a log and the correct numbers sent to an XML for processing by a server process.

import csv
import re
import datetime
import xml.etree.cElementTree as ET
import xml.dom.minidom


now = datetime.datetime.now()
now = now.strftime("%d-%m-%Y_%H%M")

new_file = open( 'new_number_list.txt', 'w' )
log_file = open( f'log-{now}.txt', 'w' )


def strip_whitespace( row ):
    return row.replace( " ", "" )


def check_number( number ):
    regex_pattern = re.compile(r'^(?:(?:\(?(?:0(?:0|11)\)?[\s-]?\(?|\+)44\)?[\s-]?(?:\(?0\)?[\s-]?)?)|(?:\(?0))(?:(?:\d{5}\)?[\s-]?\d{4,5})|(?:\d{4}\)?[\s-]?(?:\d{5}|\d{3}[\s-]?\d{3}))|(?:\d{3}\)?[\s-]?\d{3}[\s-]?\d{3,4})|(?:\d{2}\)?[\s-]?\d{4}[\s-]?\d{4}))(?:[\s-]?(?:x|ext\.?|\#)\d{3,4})?$')

    if regex_pattern.match( number ):
        return True
    else:
        write_log_file( f'Bad number - {number}' )
        return False


def write_new_file( number ):
    new_file.write( number )
    new_file.write( '\n' )


def write_log_file( line ):
    log_file.write( line )
    log_file.write( '\n' )


def create_xml( data ):
    m_encoding = 'UTF-8'

    root = ET.Element("Numbers")

    for number in data:
        if number.startswith( '0' ):
            number = f'9{number}'
        elif number.startswith( '+44' ):
            number = number.replace( '+44', '90' )

        doc = ET.SubElement(root, "Number", number=number)

    dom = xml.dom.minidom.parseString(ET.tostring(root))
    xml_string = dom.toprettyxml()
    part1, part2 = xml_string.split('?>')

    with open( "processed_numbers.xml", 'w' ) as xfile:
        xfile.write(part1 + 'encoding=\"{}\"?>\n'.format(m_encoding) + part2)
        xfile.close()


def process_csv( input_file ):

    confirmed_numbers = []
    with open( input_file ) as csv_file:
        csv_reader = csv.reader( csv_file, delimiter=',' )
    
        for row in csv_reader:
            if not ''.join( row ).strip():
                continue
            else:
                number = strip_whitespace( row[ 1 ] )
                result = check_number( number )

                if result:
                    confirmed_numbers.append( number )

    create_xml( confirmed_numbers )




process_csv( 'inputCSV.csv' )

I am still fairly new to python and would be grateful if you could let me know how I could improve and become better at coding python.

Cheers in advance.

\$\endgroup\$
3
  • \$\begingroup\$ Please show example input and output \$\endgroup\$
    – Reinderien
    Oct 27, 2022 at 21:51
  • 1
    \$\begingroup\$ "Standard of 11-digit numbers" needs a citation. Are you certain there no longer any places remaining with 10 digits in their numbers? \$\endgroup\$ Oct 28, 2022 at 6:41
  • \$\begingroup\$ for number in data:- this can be wrapped in to list comprehension using conditional arguments ie data = [x for x in n if x <3] ....etc,. dictionaries and sets work well when your managing data \$\endgroup\$
    – Barb
    Oct 29, 2022 at 10:02

2 Answers 2

2
\$\begingroup\$

Logging

Use the logging module to log to files:

# from the logging cookbook
import logging

# create logger with the name 'my_application'
logger = logging.getLogger('my_application')
logger.setLevel(logging.DEBUG)

# create file handler
fh = logging.FileHandler(f'log-{now}.txt')
fh.setLevel(logging.DEBUG)

# create formatter and add it to the handler
formatter = logging.Formatter(
    '%(asctime)s - %(name)s - %(levelname)s - %(message)s'
)
fh.setFormatter(formatter)

# add the handler to the logger
logger.addHandler(fh)

Now, anywhere you have write_log_file you can use logger.info('line'):

def check_number( number ):
    regex_pattern = re.compile(r'^(?:(?:\(?(?:0(?:0|11)\)?[\s-]?\(?|\+)44\)?[\s-]?(?:\(?0\)?[\s-]?)?)|(?:\(?0))(?:(?:\d{5}\)?[\s-]?\d{4,5})|(?:\d{4}\)?[\s-]?(?:\d{5}|\d{3}[\s-]?\d{3}))|(?:\d{3}\)?[\s-]?\d{3}[\s-]?\d{3,4})|(?:\d{2}\)?[\s-]?\d{4}[\s-]?\d{4}))(?:[\s-]?(?:x|ext\.?|\#)\d{3,4})?$')

    if regex_pattern.match( number ):
        return True
    else:
        logger.info( f'Bad number - {number}' )
        return False

write_new_file

You don't use this, get rid of it.

Style

You don't need spaces between parentheses and parameters:

# go from this
some_function( x )

# to this
some_function(x)

Closing context manager file handle

You don't need to do this, with will handle it for you:

with open("processed_numbers.xml", 'w') as xfile:
    xfile.write(part1 + 'encoding=\"{}\"?>\n'.format(m_encoding) + part2)
    xfile.close() # <---- Eliminate this


with open("processed_numbers.xml", 'w') as xfile:
    xfile.write(part1 + 'encoding=\"{}\"?>\n'.format(m_encoding) + part2)

re.compile

In your regex function, you are recompiling the regex even though it doesn't change across calls:

def check_number(number):
    regex_pattern = re.compile(r'^(?:(?:\(?(?:0(?:0|11)\)?[\s-]?\(?|\+)44\)?[\s-]?(?:\(?0\)?[\s-]?)?)|(?:\(?0))(?:(?:\d{5}\)?[\s-]?\d{4,5})|(?:\d{4}\)?[\s-]?(?:\d{5}|\d{3}[\s-]?\d{3}))|(?:\d{3}\)?[\s-]?\d{3}[\s-]?\d{3,4})|(?:\d{2}\)?[\s-]?\d{4}[\s-]?\d{4}))(?:[\s-]?(?:x|ext\.?|\#)\d{3,4})?$')

    if regex_pattern.match(number):
        return True
    else:
        logger.info(f'Bad number - {number}')
        return False

Instead, compile it once globally:

# I've also broken this string up because it's super long
PHONE_REGEX = re.compile(
    r'^(?:(?:\(?(?:0(?:0|11)\)?[\s-]?\(?|\+)44\)?'
    r'[\s-]?(?:\(?0\)?[\s-]?)?)|(?:\(?0))(?:(?:\d{5}\)?'
    r'[\s-]?\d{4,5})|(?:\d{4}\)?[\s-]?(?:\d{5}|\d{3}[\s-]?'
    r'\d{3}))|(?:\d{3}\)?[\s-]?\d{3}[\s-]?\d{3,4})'
    r'|(?:\d{2}\)?[\s-]?\d{4}[\s-]?\d{4}))(?:[\s-]?'
    r'(?:x|ext\.?|\#)\d{3,4})?$'
)

# now your function looks like:
def check_number(number):
    if PHONE_REGEX.match(number):
        return True
    else:
        logger.info(f'Bad number - {number}')
        return False

strip_whitespace

I don't think this needs to be a standalone function:

def process_csv( input_file ):

    confirmed_numbers = []
    with open( input_file ) as csv_file:
        csv_reader = csv.reader( csv_file, delimiter=',' )
    
        for row in csv_reader:
            if not ''.join(row).strip():
                continue
            else:
                # just do this instead
                number = row[1].replace(' ', '')
                result = check_number(number)

While we're here, you don't need the else clause. You've done the right thing and already checked the condition you want to skip:

def process_csv(input_file):
    confirmed_numbers = []
    with open(input_file) as csv_file:
        csv_reader = csv.reader(csv_file, delimiter=',')
    
        for row in csv_reader:
            if not ''.join(row).strip():
                continue
            
            number = row[1].replace(' ', '')
            result = check_number(number)

It's also important when using the csv readers and writers that your handle is opened with newline='':

def process_csv(input_file):
    confirmed_numbers = []
    with open(input_file, newline='') as csv_file:
        csv_reader = csv.reader(csv_file, delimiter=',')
    
        for row in csv_reader:
            # do things

We can also collapse the if result into if check_number(number), which I would rename is_valid(number):

def process_csv(input_file):
    confirmed_numbers = []
    with open(input_file) as csv_file:
        csv_reader = csv.reader(csv_file, delimiter=',')
    
        for row in csv_reader:
            if not ''.join(row).strip():
                continue
            
            number = row[1].replace(' ', '')

            # let's continue the pattern you used earlier
            if not is_valid(number):
                continue
         
            confirmed_numbers.append(number)

Last, I'd move the logging bit to outside of the is_valid check. It doesn't make sense to me for the test of a value to write to a file:

def check_number(number):
    if PHONE_REGEX.match(number):
        return True
    else:
        return False


def process_csv(input_file):
    confirmed_numbers = []
    with open(input_file) as csv_file:
        csv_reader = csv.reader(csv_file, delimiter=',')
    
        for row in csv_reader:
            if not ''.join(row).strip():
                continue
            
            number = row[1].replace(' ', '')

            if not is_valid(number):
                logger.info(f'Bad number - {number}')
                continue
         
            confirmed_numbers.append(number)

Taking in arguments from the user

You have already taken the time and made the csv file a parameter. This is good! Now to finish things off, you can allow yourself or any other user to pass in a filename when calling your python script:

import sys

~skipping all of your other code~

if __name__ == "__main__":
    # argv[1] is the first command line arg passed to your script
    # while argv[0] is the path of your script
    file = sys.argv[1]
    print(f'Processing file {file}')
    main(file)

Two things, an if __name__ guard will ensure your main function only runs if you invoke the script. If you want to import the module, the if block will prevent that from happening. This is good practice for when you start to write code designed to be more reusable by other modules.

Another way to take in command-line arguments is with the argparse module:

from argparse import ArgumentParser


if __name__ == "__main__":
    parser = ArgumentParser()
    parser.add_argument('file', type=str, help='Path to a csv file you want to read')
    args = parser.parse_args()

    file = args.file

    main(file)

For a simple program like this, sys.argv might get the job done, but for larger programs with more command line arguments, argparse can be very powerful.

\$\endgroup\$
1
\$\begingroup\$

Overlapping somewhat with the good advice from C.Nivs,

Use the actual logging module.

Since log_file already uses string interpolation, do not call strftime - just embed the timestamp format in the interpolated string.

strip_whitespace doesn't IMO meet the threshold for needing a function.

Add PEP484 typehints to your function signatures.

Your regex is a nightmare. If you can't simplify it, then at least use x (verbose) mode and break it up into multiple lines.

Don't mix minidom and non-minidom code, and don't manually generate and parse XML. Just use minidom on its own.

The phone number prefix parsing does not belong in create_xml.

Consider converting process_csv into a generator.

Suggested

import csv
import logging
import re
from datetime import datetime
from pathlib import Path
from typing import Iterator, Iterable
from xml.dom import getDOMImplementation


def make_logger() -> logging.Logger:
    now = datetime.now()
    handler = logging.FileHandler(f'log-{now:%d-%m-%Y_%H%M}.txt')
    handler.setLevel(logging.INFO)

    logger = logging.getLogger('phonenumbers')
    logger.setLevel(logging.INFO)
    logger.addHandler(handler)
    return logger


logger = make_logger()


def check_number(phone_number: str) -> bool:
    regex_pattern = re.compile(
        r'''(?x)
        ^(?:
            (?:
                \(?
                (?:0(?:0|11)\)?[\s-]?\(?|\+)
                44\)?[\s-]?
                (?:\(?0\)?[\s-]?)?
            )
            |(?:\(?0)
        )
        (?:
            (?:\d{5}\)?[\s-]?\d{4,5})
            |(?:\d{4}\)?[\s-]?(?:\d{5}|\d{3}[\s-]?\d{3}))
            |(?:\d{3}\)?[\s-]?\d{3}[\s-]?\d{3,4})
            |(?:\d{2}\)?[\s-]?\d{4}[\s-]?\d{4})
        )
        (?:[\s-]?(?:x|ext\.?|\#)\d{3,4})?
        $'''
    )

    if regex_pattern.match(phone_number):
        return True
    
    logger.error(f'Bad number - {phone_number}')
    return False


def create_xml(data: Iterable[str]) -> None:
    doc = getDOMImplementation().createDocument(None, 'Numbers', None)

    for number in data:
        node = doc.createElement('Number')
        node.setAttribute('number', number)
        doc.documentElement.appendChild(node)

    encoding = 'UTF-8'
    with open('processed_numbers.xml', 'w', encoding=encoding) as xfile:
        doc.writexml(xfile, encoding=encoding)


def process_csv(input_file: Path) -> Iterator[str]:
    with input_file.open(newline='') as csv_file:
        csv_reader = csv.reader(csv_file)

        for some_other_col, phone_str in csv_reader:
            number = phone_str.replace(' ', '')
            if number and check_number(number):
                if number.startswith('0'):
                    yield '9' + number
                elif number.startswith('+44'):
                    yield '90' + number.removeprefix('+44')


def main() -> None:
    confirmed_numbers = process_csv(Path('inputCSV.csv'))
    create_xml(confirmed_numbers)


if __name__ == '__main__':
    main()
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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