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())
\$\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\$
    – RootTwo
    Jan 31 '20 at 19:59
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())
\$\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\$ Jan 29 '20 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\$
    – Peilonrayz
    Jan 29 '20 at 7:01
  • \$\begingroup\$ yep. Thought that's a hidden feature that I haven't heard about \$\endgroup\$ Jan 29 '20 at 7:02
  • \$\begingroup\$ @GrajdeanuAlex. That would be one weird feature. But tbh, it did look nicer... \$\endgroup\$
    – Peilonrayz
    Jan 29 '20 at 7:03

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.