4
\$\begingroup\$

I've been working on a parser for shop receipts which extracts data about the payment. Here is the text that I'm parsing:

  * Vic107Payment 
    Text: PINNEN
    TicketData: POI: 12345678                   
KLANTTICKET
--------------------------------
Terminal:                 ABC123
Merchant:                 123456
Periode:                    1234
Transactie:             12345678


MAESTRO         
(A0000000012345)                
MAESTRO             
Kaart:       xxxxxxxxxxxxxxx1234
Kaartserienummer:              0

BETALING            
Datum:          01/01/2020 04:15
Autorisatiecode:          123ABC

Totaal:                 1,00 EUR

Leesmethode: NFC Chip           

    CardTypeId: 1234
    CardTypeText: MAESTRO
    ReceiptNumber: 
    DrawerAmount: 1,00
    Number: 1
    DrawerId: drawers/default
    DrawerNumber: 1
    Amount: 1,00
    IsCancelable: True
===

Here is the code that parses this fragment from the receipt. Now my concern is that people will find it difficult to read and/or difficult to maintain, so I was wondering whether it could be improved in any way?

def parse_card_payment(product):
    # cp stands for card payment
    cp_poi = None
    cp_terminal = None
    cp_merchant = None
    cp_period = None
    cp_transaction = None
    cp_card = None
    cp_card_serial_number = None
    cp_date = None
    cp_authorisation_code = None
    cp_total = None
    cp_card_type_id = None
    cp_card_type_text = None
    cp_drawer_id = None
    cp_drawer_amount = None
    cp_cancelable = None
    cp_card_type = None

    for line in product.strip().split('\n'):
        if 'POI' in line:
            cp_poi = line.split(':')[1].strip()
        elif 'Terminal' in line:
            cp_terminal = line.split(':')[1].strip()
        elif 'Merchant' in line:
            cp_merchant = line.split(':')[1].strip()
        elif 'Periode' in line:
            cp_period = line.split(':')[1].strip()
        elif 'Transactie' in line:
            cp_transaction = line.split(':')[1].strip()
        elif 'Kaart:' in line:
            cp_card = line.split(':')[1].strip()
        elif 'Kaartserienummer' in line:
            cp_card_serial_number = line.split(':')[1].strip()
        elif 'Datum' in line:
            cp_date = line.split(':')[1].strip()
        elif 'Autorisatiecode' in line:
            cp_authorisation_code = line.split(':')[1].strip()
        elif 'Totaal' in line:
            cp_total = line.split(':')[1].strip()
        elif 'CardTypeId' in line:
            cp_card_type_id = line.split(':')[1].strip()
        elif 'CardTypeText' in line:
            cp_card_type_text = line.split(':')[1].strip()
        elif 'DrawerAmount' in line:
            cp_drawer_amount = line.split(':')[1].strip()
        elif 'DrawerId' in line:
            cp_drawer_id = line.split(':')[1].strip()
        elif 'Cancelable' in line:
            cp_cancelable = line.split(':')[1].strip()
        elif 'Leesmethode' in line:
            cp_card_type = line.split(':')[1].strip()

    cp = {'cp_poi': cp_poi,
          'cp_terminal': cp_terminal,
          'cp_merchant': cp_merchant,
          'cp_period': cp_period,
          'cp_transaction': cp_transaction,
          'cp_card': cp_card,
          'cp_card_serial_number': cp_card_serial_number,
          'cp_date': cp_date,
          'cp_authorisation_code': cp_authorisation_code,
          'cp_total': cp_total,
          'cp_card_type_id': cp_card_type_id,
          'cp_card_type_text': cp_card_type_text,
          'cp_drawer_id': cp_drawer_id,
          'cp_drawer_amount': cp_drawer_amount,
          'cp_cancelable': cp_cancelable,
          'cp_card_type': cp_card_type}
    return cp
\$\endgroup\$
10
\$\begingroup\$

In programming there is the general principle Don't Repeat Yourself (DRY). Your code is a lot of repetition of exactly the same pattern, with only the string changing.

So, just put those strings into a dictionary, with the final variable name as keys:

RECEIPT_ITEMS = {"cp_poi": "POI", "cp_terminal": "Terminal",
                 "cp_merchant": "Merchant", "cp_period": "Periode",
                 "cp_transaction": "Transactie", "cp_card": "Kaart",
                 "cp_card_serial_number": "Kaartserienummer", "cp_date": "Datum",
                 "cp_authorisation_code": "Autorisatiecode",
                 "cp_total": "Totaal", "cp_card_type_id": "CardTypeId", 
                 "cp_card_type_text": "CardTypeText",
                 "cp_drawer_amount": "DrawerAmount", "cp_drawer_id": "DrawerId",
                 "cp_cancelable": "Cancelable", "cp_card_type": "Leesmethode"}

def parse_card_payment(product):
    cp = dict.fromkeys(RECEIPT_ITEMS.keys())
    for line in product.splitlines():
        for key, value in RECEIPT_ITEMS.items():
            if value in line:
                cp[key] = line.split(":")[1].strip()
                break
    return cp

This has the advantage that if you ever have receipts in another language than Dutch (but with the same structure), you only need to localize the values of this dictionary and not change your whole code.

Note that I used str.splitlines, which automatically ignores trailing newlines.

A different approach might be to use a multi-line RegEx to perform the search directly, but that will probably be more complicated.

\$\endgroup\$
2
  • \$\begingroup\$ Loved this approach, looks succinct and easily readable. \$\endgroup\$
    – Cornul11
    Mar 13 '20 at 14:56
  • \$\begingroup\$ Keep in mind that if more than one variable name appears in the same line, only the "first" (where "first" is determined by what order Python decides to retrieve the key/value pairs in) will be seen. If any variable names are a subset of another, that could be a problem. \$\endgroup\$ Mar 14 '20 at 5:39
5
\$\begingroup\$

bugs

Due to the extra : in the lines, POI and Datum are parsed incorrectly

{'cp_poi': 'POI',
 'cp_terminal': 'ABC123',
 'cp_merchant': '123456',
 'cp_period': '1234',
 'cp_transaction': '12345678',
 'cp_card': 'xxxxxxxxxxxxxxx1234',
 'cp_card_serial_number': '0',
 'cp_date': '01/01/2020 04',
 'cp_authorisation_code': '123ABC',
 'cp_total': '1,00 EUR',
 'cp_card_type_id': '1234',
 'cp_card_type_text': 'MAESTRO',
 'cp_drawer_id': 'drawers/default',
 'cp_drawer_amount': '1,00',
 'cp_cancelable': 'True',
 'cp_card_type': 'NFC Chip'}

alternate approach

Instead of a giant if-elseif-if tree, I would as a function that parses a line, and returns the type of line with the value.

def parse_line(line):
    """
    parses a line on a receipt. 

    Returns the datafield and value as a tuple 
    or tuple with the original text if there is no data on the line
    """
    return tuple(part.strip() for part in line.split(': ')[-2:])

Note that I split on ": ". The space makes the parsing of the date correct. The [-2:] selects the last 2 items, making the POI parse correctly.

parsed_results = {
    result[0]: result[1]
    for result in (parse_line(line) for line in text.split("\n"))
    if len(result) > 1
}
{'Text': 'PINNEN',
 'POI': '12345678',
 'Terminal': 'ABC123',
 'Merchant': '123456',
 'Periode': '1234',
 'Transactie': '12345678',
 'Kaart': 'xxxxxxxxxxxxxxx1234',
 'Kaartserienummer': '0',
 'Datum': '01/01/2020 04:15',
 'Autorisatiecode': '123ABC',
 'Totaal': '1,00 EUR',
 'Leesmethode': 'NFC Chip',
 'CardTypeId': '1234',
 'CardTypeText': 'MAESTRO',
 'ReceiptNumber': '',
 'DrawerAmount': '1,00',
 'Number': '1',
 'DrawerId': 'drawers/default',
 'DrawerNumber': '1',
 'Amount': '1,00',
 'IsCancelable': 'True'}

Or you can use regular expressions

import re

PATTERN = re.compile(r"(?:.*:\s*)?(\w+?):\s+(.*?)\s*$")

def parse_line2(line):
    return PATTERN.findall(line)

parsed_results2 = {
    result[0][0]: result[0][1]
    for result in (parse_line2(line) for line in text.split("\n"))
    if result
}

In this simple case I would use the first parser method. If the patterns get a little more complicated, You can change to the re.

translation:

Here I would use a dictionary that links all keywords in your return dictionary to the keys in the parsed lines:

data_translation = {
    "cp_poi": "POI",
    "cp_terminal": "Terminal",
    "cp_merchant": "Merchant",
    "cp_period": "Periode",
    "cp_total": "Totaal",
    "cp_date": "Datum"
    # ...
}

result = {
    keyword: parsed_results.get(key_value, None)
    for keyword, key_value in data_translation.items()
}
{'cp_poi': '12345678',
 'cp_terminal': 'ABC123',
 'cp_merchant': '123456',
 'cp_period': '1234',
 'cp_total': '1,00 EUR',
 'cp_date': '01/01/2020 04:15'}

further parsing.

Since functions can be values in a dictionary, you can add functions to further process the values. For example convert thetotal to a tuple of value, currency, transform the date from a string to a datetime object,...

import decimal

def parse_amount(amount):
    """converts an amount to a tuple of amount, currency"""
    value, currency = amount.split(" ")
    value = value.replace(",", ".")
    context = decimal.Context(prec=2, rounding=decimal.ROUND_HALF_UP)
    value_decimal = decimal.Decimal(value, context=context).quantize(
        decimal.Decimal("0.01")
    )
    return value_decimal, currency

def parse_date(date_str):
    return datetime.datetime.strptime(date_str, "%d/%m/%Y %H:%M")

converters = {
    "cp_date": parse_date,
    "cp_total": parse_amount
}
converted_result = {
    key: converters.get(key, lambda x: x)(value)
    for key, value in results.items()
}
{'cp_poi': '12345678',
 'cp_terminal': 'ABC123',
 'cp_merchant': '123456',
 'cp_period': '1234',
 'cp_total': (Decimal('1.00'), 'EUR'),
 'cp_date': datetime.datetime(2020, 1, 1, 4, 15)}

other remarks:

docstring

Use a docstring to describe what the method does

formatting

I don't like this style of dict literal

cp = {'cp_poi': cp_poi,
      'cp_terminal': cp_terminal,
      'cp_merchant': cp_merchant,
      # ...
      'cp_cancelable': cp_cancelable,
      'cp_card_type': cp_card_type}

I use black with a line length of 79 as automatic formatter

Which turns this into

cp = {
    "cp_poi": cp_poi,
    "cp_terminal": cp_terminal,
    "cp_merchant": cp_merchant,
    # ...
    "cp_cancelable": cp_cancelable,
    "cp_card_type": cp_card_type,
}

This minimizes the hassle if I want to remove or add a line, also in the git diffs.

Data structures

In general, if you needa lot of variables, each only differing in a slight amount, you can use a better data structure. In this case, this is with dicts, instead of the dozen variables and lone if-else tree. Get to know the python data structures, and the different looping arrangements in Python. Almost never is a dozen variables the best solution.

\$\endgroup\$
5
  • \$\begingroup\$ Ah, I just noticed the bug as well, but since you already pointed it out, I don't have to anymore :) \$\endgroup\$
    – Graipher
    Mar 13 '20 at 10:59
  • \$\begingroup\$ I wouldn't even split on the colon or colon-space. Find the first colon, grab everything after that. Splitting on colons, or colon-space, is the wrong thing. \$\endgroup\$ Mar 13 '20 at 13:56
  • \$\begingroup\$ for POI the part before the first colon is not needed, and the second colon separates key and value. For date, the second colon is part of the value, so I don't see how your approach would work. \$\endgroup\$ Mar 13 '20 at 13:58
  • \$\begingroup\$ Thank you for pointing out the bug with parsing the lines with double colons. \$\endgroup\$
    – Cornul11
    Mar 13 '20 at 14:54
  • 1
    \$\begingroup\$ I combined your approach and the one provided by @Graipher into something that looks more readable. I've just defined the keywords that the parser should be looking for then I translate them to elements in the dictionary. \$\endgroup\$
    – Cornul11
    Mar 13 '20 at 14:55

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.