Calculate sales tax from tabular data

Question

This is a Python 3 solution to the problem stated in Sales Tax Problem, rejected for not being up to their standards. It also seems like the same thing as the problem stated in Sales tax calculator, rejected for being not OOP. I'm not bothering with the requirement to round sales tax to the nearest 5 cents, and I'm taking the liberty of assuming the data is text-based and tabular.

I took a Python class in college, but I have no experience writing code in Python. I'm a statistician and data analyst, and my main programming language is R. That is, I won't be interviewing for a software engineering job any time soon (or hopefully ever), but I would like to write code that is robust to user abuse/sloppiness/error, is readable and makes it easy for collaborators to work on, and is extensible in case features need to be added.

One of my big concerns is testing and data validation. I have no experience writing unit tests, and I didn't write any for this project. However catching errors is of paramount importance in the work I do, so I would like to ensure that I'm doing it right.

#!/usr/bin/env python3

# https://codereview.stackexchange.com/q/91332/47041

import argparse
from decimal import *
import sys

class ReceiptItem:
    def __init__(self, attributes):
        self.name = attributes[0]
        assert isinstance(self.name, str)
        self.imported = attributes[1]
        assert isinstance(self.imported, bool)
        self.taxable = attributes[2]
        assert isinstance(self.taxable, bool)
        self.price = attributes[3]
        assert isinstance(self.price, Decimal)

        self.tax_rate = 0
        if self.imported:
            self.tax_rate = 5
        else:
            if self.taxable:
                self.tax_rate = 10

        self.tax = self.tax_rate * self.price / 100
        self.price_with_tax = self.price + self.tax


class Receipt:
    def __init__(self, items):
        for item in items:
            assert isinstance(item, ReceiptItem)
        self.items = items
        self.subtotal = sum([item.price for item in self.items])
        self.total = sum([item.price_with_tax for item in self.items])
        self.tax = self.total - self.subtotal
        assert self.tax == sum([item.tax for item in self.items])

    def prettify(self):
        result = "\n".join(["%s: %0.2f" % (item.name, item.price_with_tax) for item in self.items])
        result = "\n".join([result, "",
            "Subtotal: %0.2f" % self.subtotal,
            "Tax: %0.2f" % self.tax,
            "Total: %0.2f" % self.total])
        return result


def validate_receipt(item_list):
    for i in range(0, len(item_list)):
        item = item_list[i]
        if len(item) != 4:
            raise ValueError("Record %i does not have four fields" % i)
        try:   
            for j in (1, 2):
                item[j] = int(item[j])
                if item[j] not in [0, 1]:
                    raise ValueError("Field %i is not 0 or 1" % j)
                item[j] = bool(item[j])
            j = 3
            try:
                item[j] = Decimal(item[j])
            except InvalidOperation:
                raise ValueError("Could not convert string to Decimal: %s" % item[j])
        except ValueError as err:
            raise ValueError("Field {0} in record {1} is invalid: {2}".format(j, i, err)) from err
        item_list[i] = ReceiptItem(item) # why can't I do item = ReceiptItem(item) and then have the change propagate to item_list[i]? did I make a copy somehow?
    return Receipt(item_list)


def main():
    parser = argparse.ArgumentParser(description="Calculate sales tax.")
    parser.add_argument("file", type=argparse.FileType("r"), nargs="?", metavar="file", help="A data file. If none provided, reads from stdin instead. Should have four columns: name (string), imported (1 or 0), taxable if not imported (1 or 0, ignored if imported=1), price (nonnegative float, will be rounded to 2 digits). Leading and trailing whitespace will be stripped. Blank lines are silently skipped.")
    parser.add_argument("-s", "--separator", default=",", dest="sep", metavar="sep", help="Field separator. Default is a comma.")
    args = parser.parse_args()
    if args.file:
        receipt = args.file.read().splitlines()
    else:
        receipt = sys.stdin.read().splitlines()
    receipt = [item.split(args.sep) for item in receipt if len(item) > 0]
    receipt = validate_receipt(receipt)
    print(receipt.prettify())

if __name__ == "__main__":
    main()

Test to make sure it works:

Save the code as tax.py, then:

tail -n +2 <<EOF | python3 tax.py
name,imported,taxable,price
Coors,0,1,3.99
Poland Spring,0,0,1.99
Orangina,1,0,2.49

EOF

Answer 1

Object-oriented modelling

The way you construct the Receipt feels clumsy:

if args.file:
    receipt = args.file.read().splitlines()
else:
    receipt = sys.stdin.read().splitlines()
receipt = [item.split(args.sep) for item in receipt if len(item) > 0]
receipt = validate_receipt(receipt)
print(receipt.prettify())

---

The first simplification is to deduplicate .read().splitlines():

input = args.file or sys.stdin
receipt = [line.split(args.sep) for line in input if line]
receipt = validate_receipt(receipt)
print(receipt.prettify())

---

The next observation I would make is that neither validate_receipt() nor receipt.prettify() does what its name suggests. validate_receipt() actually constructs a Receipt object out of a two-dimensional array of strings. receipt.prettify() actually produces a string representation of the Receipt, and it would be better as the magic Receipt.__str__() method.

I'm not a fan of receipt = … followed by another receipt = …, since you're using the same variable to refer to two different types.

But why should the Receipt be constructed all at once? That OOP design is not much better than print(format_as_receipt(all_the_data_as_a_2d_array)). I suggest a loop more like this:

input = args.file or sys.stdin
receipt = Receipt()
for line in input:
    receipt.add(ReceiptItem(*line.split(args.sep)))
print(receipt)

---

Based on the ideas above, and omitting data validation, here's how I would rewrite it:

import argparse
from decimal import Decimal
import sys

class ReceiptItem:
    def __init__(self, name, imported, taxable, price):
        self.name = name
        self.imported = bool(int(imported))
        self.taxable = bool(int(taxable))
        self.price = float(price)

        self.tax_rate = Decimal('0.05') if self.imported else \
                        Decimal('0.10') if self.taxable else \
                        Decimal()
        self.tax = self.price * self.tax_rate
        self.price_with_tax = self.price + self.tax

    def __str__(self):
        return "%s: %0.2f" % (self.name, self.price_with_tax)

class Receipt:
    def __init__(self):
        self.items = []
        self.subtotal = self.tax = Decimal()

    def add(self, item):
        self.items.append(item)
        self.subtotal += item.price
        self.tax += item.tax

    @property
    def total(self):
        return self.subtotal + self.tax

    def __str__(self):
        return "\n".join([str(item) for item in self.items] + [
            '',
            "Subtotal: %0.2f" % self.subtotal,
            "Tax: %0.2f" % self.tax,
            "Total: %0.2f" % self.total])

def main():
    parser = argparse.ArgumentParser(description="Calculate sales tax.")
    parser.add_argument("file", type=argparse.FileType("r"), nargs="?", metavar="file", help="A data file. If none provided, reads from stdin instead. Should have four columns: name (string), imported (1 or 0), taxable if not imported (1 or 0, ignored if imported=1), price (nonnegative float, will be rounded to 2 digits). Leading and trailing whitespace will be stripped. Blank lines are silently skipped.")
    parser.add_argument("-s", "--separator", default=",", dest="sep", metavar="sep", help="Field separator. Default is a comma.")
    args = parser.parse_args()
    input = args.file or sys.stdin
    receipt = Receipt()
    for line_number, line in enumerate(input):
        receipt.add(ReceiptItem(*line.rstrip().split(args.sep)))
    print(receipt)

if __name__ == "__main__":
    sys.exit(main() or 0)

Validation

Validation should be done in the constructor, so that it cannot be easily bypassed. Doing the validation in a separate function and then asserting the expectations is both redundant and less safe.

The error messages should be more human-friendly. For example, "Could not convert string to Decimal: %s" would be slightly better as "%s is not a decimal value". Counting fields and records starting from 0 is also counterintuitive to most users.

You do plenty of exception throwing, but in the end the exceptions remain uncaught. Users should see error messages, not stack traces for data validation errors.

I suggest doing the validation using meta-programming:

from collections import namedtuple

class ReceiptItem:
    def __init__(self, *fields):
        if len(fields) < len(ReceiptItem.COLUMNS):
            raise ValueError('Field(s) {} missing'.format(','.join(f.name for f in ReceiptItem.COLUMNS[len(fields):])))
        if len(fields) > len(ReceiptItem.COLUMNS):
            raise ValueError('{} fields instead of {}'.format(len(fields), len(ReceiptItem.COLUMNS)))
        for i, column in enumerate(ReceiptItem.COLUMNS):
            try:
                if not column.validate(fields[i]):
                    raise ValueError('Field {} ({}) {}'.format(i + 1, column.name, column.message))
            except Exception:
                raise ValueError('Field {} ({}) {}'.format(i + 1, column.name, column.message))
            setattr(self, column.name, column.convert(fields[i]))

    @property
    def tax_rate(self):
        return Decimal('0.05') if self.imported else \
               Decimal('0.10') if self.taxable else \
               Decimal()

    @property
    def tax(self):
        return self.price * self.tax_rate

    @property
    def price_with_tax(self):
        return self.price + self.tax

    def __str__(self):
        return "%s: %0.2f" % (self.name, self.price_with_tax)

Field = namedtuple('Field', 'name message validate convert')
ReceiptItem.COLUMNS = [
    Field('name', "is missing",
          lambda s: s.strip(),
          lambda s: s.strip()),
    Field('imported', "must be 0 or 1",
          lambda s: s in (False, True, '0', '1'),
          lambda s: bool(int(s))),
    Field('taxable', "must be 0 or 1",
          lambda s: s in (False, True, '0', '1'),
          lambda s: bool(int(s))),
    Field('price', "must be a decimal",
          lambda s: Decimal(s),
          lambda s: Decimal(s)),
]

… and the corresponding code in main():

for line_number, line in enumerate(input, 1):
    try:
        receipt.add(ReceiptItem(*line.rstrip().split(args.sep)))
    except ValueError as e:
        print('Error on line {}: "{}"\n{}'.format(line_number, line.rstrip(), e), file=sys.stderr)
        return 1