I did a script to extract information from the Egyptian national ID, e.g. national id is 14 digit ex(29501023201952). It could divide as described below:

2 - 990115 - 01 - 0192 - 1

x - yymmdd - ss - iiig - z
  • x (2) is the birth century (2 represent 1900 to 1999, 3 represent 2000 to 2099 .. etc)

  • yymmdd (200115) is the date of birth, yy(99) year,mm(01) month, dd(15) day

  • ss(01) birth governorate coda (88 for people who born in a foreign country, 01 for who born in Cairo, ...etc )

  • iiig(0192) the sequence in the computer between births in this birthday and

  • g(2) represent the gender (2,4,6,8 for females and 1,3,5,7,9)

  • z(1) number Ministry of Interior added it to validate if the National ID fake or not (1 to 9)

I am looking for some general feedback on how I can improve the structure and efficiency of my code.

import argparse
from datetime import datetime
import textwrap

governorates = {'01': 'Cairo',
                '02': 'Alexandria',
                '03': 'Port Said',
                '04': 'Suez',
                '11': 'Damietta',
                '12': 'Dakahlia',
                '13': 'Ash Sharqia',
                '14': 'Kaliobeya',
                '15': 'Kafr El - Sheikh',
                '16': 'Gharbia',
                '17': 'Monoufia',
                '18': 'El Beheira',
                '19': 'Ismailia',
                '21': 'Giza',
                '22': 'Beni Suef',
                '23': 'Fayoum',
                '24': 'El Menia',
                '25': 'Assiut',
                '26': 'Sohag',
                '27': 'Qena',
                '28': 'Aswan',
                '29': 'Luxor',
                '31': 'Red Sea',
                '32': 'New Valley',
                '33': 'Matrouh',
                '34': 'North Sinai',
                '35': 'South Sinai',
                '88': 'Foreign'}

fake_national_id_message = 'This ID Not Valid'

def extract_birth_century(birth_century_code: int) -> int:
     extract birth century from national id it's in index 0
    :param birth_century_code: one digit
    :return: birth century
    assert type(birth_century_code) == int, 'birth century code must be int value'
    current_century = get_century_from_year(int(datetime.now().year))
    birth_century = birth_century_code + 18
    assert (birth_century >= 19) and (birth_century <= current_century), fake_national_id_message
    return birth_century

def get_century_from_year(year):
    return year // 100 + 1

def convert_birthdate(birthdate: str) -> str:
    Convert birthday in national id from  fromat yymmdd to yyyy - mm - dd format
    it's from index 0 to 6 in EG national id
    :param birthdate: str
            format cyymmdd, c represent birth century code
    :return: str
    assert len(str(birthdate)) == 7, "birthdate must be 7 digit"
    birth_century = extract_birth_century(int(birthdate[0]))
    birth_year = birthdate[1:3]
    birth_month = birthdate[3:5]
    birth_day = birthdate[5:]
    birth_full_year = (birth_century * 100) - 100 + int(birth_year)
    birthdate_str = '{0}-{1}-{2}'.format(birth_full_year, birth_month, birth_day)
    birthdate_date = datetime.strptime(birthdate_str, '%Y-%m-%d')
    assert birthdate_date <= datetime.now() and birthdate_date >= datetime.strptime('1900-01-01','%Y-%m-%d'), fake_national_id_message
    return birthdate_str

def get_birth_governorate(birth_governorate_coda: str) -> str:
    :param birth_governorate_coda:
            Index 7 and 8 in EG national id
    :return: str
             Birth governorate

    assert type(birth_governorate_coda) == str, 'Birth governorate coda must be string not integer'
    assert len(birth_governorate_coda) == 2, 'Birth governorate coda must be 2 digit'
    assert birth_governorate_coda in governorates, 'Birth governorate coda not valid'
    return governorates[birth_governorate_coda]

def get_gender(gender_code: int) -> str:
    :param gender_code:
            Index 12 in EG National ID

    :return: str
    assert type(gender_code) == int and gender_code > 0 and gender_code <= 9, 'gender code not valid'
    if gender_code % 2 == 0:
        return 'Female'
        return 'Male'

def extract_info_from_national_id(national_id: int):
    Extract information from national EG national ID
    :param national_id: int
            EG national id must be number of 14 digit
    :return: dict
    assert type(national_id) == int, "National ID must be Numbers not string"
    assert len(str(national_id)) == 14, "National ID must be 14 Number "
    national_id_str = str(national_id)
    info ={}
    info['birth_century'] = extract_birth_century(int(national_id_str[0]))
    info['date_of_birth'] = convert_birthdate(national_id_str[0:7])
    info['birth_governorate'] = get_birth_governorate(national_id_str[7:9])
    info['sequence_in_computer'] = national_id_str[9:13]
    info['gender'] = get_gender(int(national_id_str[12]))
    # last_number = national_id_str[13]
    return info

def print_info(information):
    print('Birth Century :', information['birth_century'])
    print('Date Of Birth :', information['date_of_birth'])
    print('Birth Governorate :', information['birth_governorate'])
    print('Gender :', information['gender'])

if __name__ == '__main__':
    parser = argparse.ArgumentParser(formatter_class=argparse.RawDescriptionHelpFormatter,
                                     Extract Information from Egyptian National ID
                                                Birth century
                                                Date Of Birth
                                                Birth Governorate

    parser.add_argument('national_id', type=int,
                        help="Add The Egyption National ID 14 Digit")
    args = parser.parse_args()
    info = extract_info_from_national_id(args.national_id)

This seems like exactly the sort of thing you would use a class for. Not just any class, but an immutable one!

I'd suggest you create a class EgyptianNationalId and initialize it from a string. You could then parse out all the fields at the time of creation, and store them using read-only attributes (such as with a namedtuple).

Something like this:

import collections
from datetime import datetime as dt

_ENID = collections.namedtuple('_ENID', 'list of fields')

class EgyptianNationalId(_ENID):
    ''' Model an Egyptian National ID string.

    >>> id = EgyptianNationalId('29501023201952')
    >>> 1999 in id.century
    >>> id.birth_date.year == 1995
    >>> id.birth_date.month
    >>> id.birth_date.day
    >>> id.birth_date.governorate
    'New Valley'
    century = {
       '1': range(1800, 1900),
       '2': range(1900, 2000),
       '3': range(2000, 2100),
       '4': range(2100, 2200),

    def from_str(cls, s):
        fields = cls.parse_str(s)
        return cls(*fields)

    def parse_str(cls, s):
        scent = s[0]
        syymmdd = s[1:7]
        ... other fields ...

        birth_date = dt.strptime(syymmdd, '%y%m%d')
        # TODO: Validate birth_date, maybe against century?

        fields = (
            ... other fields ...
        return fields
  • \$\begingroup\$ Perhaps a dataclass then? \$\endgroup\$
    – user134596
    Jun 12 '19 at 3:27
  • \$\begingroup\$ A dataclass could work, if the python version is high enough. \$\endgroup\$
    – aghast
    Jun 12 '19 at 17:54

At first: well done, your code quality is already at a high level! Here are some remarks that you could integrate besides the class approach mentioned before.

The assert statement works only if python is in debug mode

It's not running when you interpret it with optimization flags, see officialdocs. It is meant as a debug tool that helps tracking down bugs. For your purpose of validation I'd suggest implementing a custom function that raises an exception manually.

The assert statements are scattered in every function

This makes it unnecessarily harder to understand what the function actually does. I recommend either factoring out the asserts, or, and that's what I'd prefer, do in two steps: First do syntax check of the id, meaning a check if the format is correct. Then do all your processing. And second, after the processing, you do a logical check of the data, meaning if the syntactically correct string contained actually meaningful data.

The naming of the functions appears inconsistent

In extract_info_from_national_id() it reads:

info['birth_century'] = extract_birth_century(int(national_id_str[0]))
info['date_of_birth'] = convert_birthdate(national_id_str[0:7])
info['birth_governorate'] = get_birth_governorate(national_id_str[7:9])
info['sequence_in_computer'] = national_id_str[9:13]
info['gender'] = get_gender(int(national_id_str[12]))

So every function is meant to convert a string into something more meaningful, which makes me expect a standardized naming pattern. I'd probably stick to get_something().

You can build a string splitter

In extract_info_from_national_id() you do the string splitting. That mixes different levels of abstraction. One could go for a simple function split_enid that returns a dictionary or a named tuple, which holds the different substrings, but nicely accessible by name. Once done, you can use it like this:

enid_strs = splid_enid(notional_id)
info['birth_century'] = get_birth_century(enid_strs.birth_century)
info['date_of_birth'] = convert_birthdate(enid_strs.birthdate)
info['birth_governorate'] = get_birth_governorate(splitted_enid.governorate)

Keep going, I like your code!


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.