Overview
What you have done is a good start. Is there room for improvement? I think so. In the following comments I will try not to repeat what has already been mentioned before in the previous reviews.
Correct Errors
You have VALID_CHARS = letters + digits + "\. -". But this generates a warning:
SyntaxWarning: invalid escape sequence '\.'
VALID_CHARS is used within a regular expression character class as in f"[^{VALID_CHARS}]" and in that context the '.' character does not need to be escaped since it will not be interpreted as the special regex character representing any character except newline.
Then when I call your function passing as follows:
unoformatted_phonebook = '''/+1-541-754-3010 156 Alphand_St. <J Steeve>
133, Green, Rd. <E Kustur> NY-56423 ;+1-541-914-3010!
'''
print(phone(unoformatted_phonebook, 'E Kustur'))
The output is:
Phone => E Kustur, Name => , Address => 133 Green Rd. NY-56423 1-541-914-3010
That is clearly not correct.
What Should Be Returned?
On a lookup you are returning either an error message or a formatted entry as a string. Wouldn't it be more flexible to return an object representing the entry or None if not found and allow the user to format the data or error message as they see fit?
Process the Unformatted Phonebook Once
Every time your function is called, it will scan through the entire unformatted phone book on average half the entries and then parse the correct entry to extract the name, phone and address. This is okay when you have two entries in the phone book, but what if you have a million entries?
The unformatted phonebook should be processed once and a dictionary created that maps a phone number to an entry consisting of a name and address. Once the unformatted phonebook had been processed phone numbers can then be looked up in \$O(1)\$ time.
What is the Abstraction?
What you are doing is creating a "reverse phonebook" that allows one to provide a phone number and return the name and address associated with that phone. Since we should be creating a dictionary once that will be subsequently used for all lookups, why not take advantage of Python's object-oriented capabilities and create a ReversePhonebook class that processes the unformatted phone book in its initializer and provides a lookup_by_phone method?
Better Choice of Names
It's already been mentioned that phone is not the best name for your function. But what about the argument names strng and num. What does strng tell me about this other than it is probably a string being passed? What does it actually represent? And what type of num are we expecting? Actually we are expecting a string value representing a phone number to look up.
When you split up the passed unformatted phone book on newline characters, you get a list of strings. What does each string represent? In your code you are referring to these as people (for the list) and person (for an individual element). But what you have is better thought of as a list of phone book entries rather than a list of people and these varibles should be named accordingly.
Perform Validation
You have name = re.search("<.*>", no_num).group(). But if no match is found this will raise an exception. When it comes to user-supplied input, take nothing for granted: you should check to see if the call to re.search actually returns a non-None value.
A Possible Implementation
The following incorporates the previous suggestions I and others have made.
"""
This module parses a poorly formatted phonebook into a well-formatted
one permitting efficient look-ups by name.
"""
from collections import namedtuple
import re
from string import ascii_letters as letters, digits
VALID_CHARS = letters + digits + ". -"
# A phonebook entry looked up by name:
PhoneBookEntry = namedtuple('PhoneBookEntry', ['name', 'address'])
class ReversePhoneBook:
"""This class implements the reverse phonebook."""
def __init__(self, unformatted_phonebook: str):
"""See https://codereview.stackexchange.com/questions/291252/extract-data-from-poorly-formatted-phonebook
for the format of the unformatted phonebook."""
self._entries: dict[str, PhoneBookEntry] = {}
phonebook_entries = unformatted_phonebook.strip().split('\n')
name_regex = re.compile(r'<(.+)>')
phone_regex = re.compile(r'\+([0-9]{1,2}-[0-9]{3}-[0-9]{3}-[0-9]{4})')
invalid_chars_regex = re.compile(fr'[^{VALID_CHARS}]+')
multiple_spaces_regex = re.compile(fr' +')
for entry in phonebook_entries:
name_match = name_regex.search(entry)
phone_match = phone_regex.search(entry)
if name_match is None or phone_match is None:
raise RuntimeError(f'Invalid entry: {entry}')
name = name_match[1]
phone = phone_match[1]
if phone in self._entries:
raise RuntimeError(f'Duplicate phone {phone} for name {name}')
# Remove invalid characters:
cleaned_entry = invalid_chars_regex.sub(' ', entry)
# Remove name and phone from entry:
cleaned_entry = cleaned_entry.replace(name, '').replace(phone, '')
# What's left is the address:
address = cleaned_entry.strip()
# Replace multiple spaces with a single space:
address = multiple_spaces_regex.sub(' ', address)
self._entries[phone] = PhoneBookEntry(name, address)
def lookup_by_phone(self, phone: str) -> PhoneBookEntry | None:
"""Look up phonebook entry by name."""
return self._entries.get(phone)
if __name__ == '__main__':
unoformatted_phonebook = '''/+1-541-754-3010 156 Alphand_St. <J Steeve>
133, Green, Rd. <E Kustur> NY-56423 ;+1-541-914-3010!
'''
phonebook = ReversePhoneBook(unoformatted_phonebook)
for phone in ('1-541-754-3010', '1-541-914-3010'):
entry = phonebook.lookup_by_phone(phone)
if entry is None:
print('No entry found for', phone)
else:
print(f'Phone => {phone}, Name => {entry.name}, Address => {entry.address}')
Prints:
Phone => 1-541-754-3010, Name => J Steeve, Address => 156 Alphand St.
Phone => 1-541-914-3010, Name => E Kustur, Address => 133 Green Rd. NY-5642
Future Extensions
Right now you are requiring phone numbers to be unique. But my spouse and I share the same phone number. Can't we each have an entry in the unformatted phonebook so that when a lookup is done by phone number two entries are returned?
What if you also wanted to provide a facility for the user to be able to look up an entry by name? Names are not unique, so this facility in general returns a list of matches.
How difficult given the above implementation would it be to implement these ideas compared to your original implementation?
