4
\$\begingroup\$

I was tasked with writing a python script to check if a passed ID number exists within a file. The file I was give contains roughly 25,000 unique ID numbers. An ID number is a unique 9 digit number.

I have a couple specific things for you to comment on if you decide to review this:

  1. As it stands, I convert the user_id passed in the id_exists to a str. I do this because I need the ID number to be the same type when I read the ID's from the file. Is there an easier way to accomplish this?

  2. This is my first time using a PEP 484 type hint, Generator. Am I using it correctly?

  3. This is also my first time utilizing a generator in this fashion. I used a generator here because instead of loading the entire file into memory, storing in a list, then iterating over the list, I can read each line and can exit early if an ID is found. Is it good that I'm using it in this way, or is there an easier way to do this?

  4. Using a helper function is a new concept to me. Am I utilizing this helper function in the most pythonic way?

Of course, any constructive criticism beyond these points is welcome. Thanks in advance.

"""
Ensures id number matches any within the ID file.
An ID number is a unique 9 digit number.
"""
import random
import time
from typing import Union, Generator
def id_exists(user_id: Union[int, str]) -> bool:
 """
 Determines if the passed ID exists within the ID file. The
 ID passed must be of length 9 exactly, and only contain numbers.
 :param Union[int, str] user_id: User ID to check for existence
 :return bool: True if ID exists in file, False otherwise
 """
 # Generator[yield_type, send_type, return_type] #
 def get_ids() -> Generator[str, None, None]:
 """
 Helper function for `id_exists`.
 Yields each line in the ID file.
 :return Generator[str, None, None]: Lines in ID file.
 """
 with open("ids.txt", "r") as file:
 for line in file:
 yield line.strip()
 # Convert user_id to <class 'str'> if not already #
 if type(user_id) == int:
 user_id = str(user_id)
 # Ensure user_id matches specifications #
 if len(user_id) != 9 or not user_id.isdigit():
 raise ValueError("ID should be 9 characters long and all digits.")
 # Check ID against file and return result #
 return any(user_id == stored_id for stored_id in get_ids())
asked Jan 29, 2020 at 0:31
\$\endgroup\$
1
  • \$\begingroup\$ Can the user_id have leading 0's? If it can, the using str() for the conversion could cause problems. An f-string f"{user_id:0>9}" could be used to convert the integer and pad it with leading 0's. \$\endgroup\$ Commented Jan 31, 2020 at 19:59

1 Answer 1

3
\$\begingroup\$
  1. One of:

    • Only take a string or a number.
    • Use str either way.
    • Work against a bespoke class.
  2. Yes. But I find it better to just use Iterator[str], unless you're using the coroutine aspects of it.

  3. Yeah that's fine. But:

    • You can just use in rather than any(...).
    • You don't need the function get_ids you can just use any, like you are now.
  • Don't use type(...) == int, use isinstance.
  • Validating user_id should probably happen somewhere else.

    I'll make a class as an example to show this.

class UserId:
 __slots__ = ('id',)
 id: int
 def __init__(self, id: int) -> None:
 if not self.valid_id(id):
 raise ValueError("ID should be 9 characters long and all digits.")
 self.id = id
 @staticmethod
 def valid_id(id: int) -> bool:
 if not isinstance(id, int):
 return False
 str_id = str(id)
 return (
 len(str_id) == 9
 and str_id.isdigit()
 )
 def exists(self) -> bool:
 user_id = str(self.id)
 with open('ids.txt') as f:
 return any(user_id == line.strip() for line in f)
print(UserId(123456789).exists())
answered Jan 29, 2020 at 2:14
\$\endgroup\$
4
  • \$\begingroup\$ Hmm, I wonder why did you write the return from valid_id() method like this: return (and len(str_id) == 9 and str_id.isdigit())? Is it just a style thing or...? \$\endgroup\$ Commented Jan 29, 2020 at 6:59
  • \$\begingroup\$ @GrajdeanuAlex. Are you talking about the first and? If so, it's not meant to be there. Forgot to remove it when I moved isinstance out of the check \$\endgroup\$ Commented Jan 29, 2020 at 7:01
  • \$\begingroup\$ yep. Thought that's a hidden feature that I haven't heard about \$\endgroup\$ Commented Jan 29, 2020 at 7:02
  • \$\begingroup\$ @GrajdeanuAlex. That would be one weird feature. But tbh, it did look nicer... \$\endgroup\$ Commented Jan 29, 2020 at 7:03

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.