4
\$\begingroup\$

I wrote the following code to read out a bill from a file and then putting it into a bills-list.

The bill contains: "company", "customer", Year, Month, Day, Amount, "credit/debit"

Is there a nicer way using list comprehension to make this code look better?

def read_bills(file = "bills.csv"):
    """Read bills from the file system 'bills.csv'"""
    # Create a list in which each bill is entered.
    bills = []
    for line in open(file):
        bill = line.strip().split(',')
        for i in range(len(bill)):
            if i > 1 and i < 5:
                bill[i] = int(bill[i].strip())
            elif i == 5:
                bill[i] = float(bill[i].strip())
            else:
                bill[i] = bill[i].strip()
        bills.append(bill)
    return bills

enter image description here

\$\endgroup\$
8
  • 1
    \$\begingroup\$ And why would that be? Is this for school? \$\endgroup\$
    – Reinderien
    Nov 8 at 17:28
  • 2
    \$\begingroup\$ Also: it's good that you've told us your columns, but can you show us the first few lines of a file? \$\endgroup\$
    – Reinderien
    Nov 8 at 17:29
  • 7
    \$\begingroup\$ Screenshots are useless; please post a few lines of a file as text, including header line…. \$\endgroup\$
    – JosefZ
    Nov 8 at 18:01
  • 1
    \$\begingroup\$ Pandas may be a bit heavy for this but why not use the built-in Python CSV module ? It appears that you are parsing a simple CSV file so there is no need for a fancy line parsing routine. Plus, using a DictReader you could use column names. This would be so much more straightforward. \$\endgroup\$
    – Anonymous
    Nov 8 at 23:45
  • 1
    \$\begingroup\$ When you try to parse CSV by splitting, you are doing it wrong. Try the implementation on this CSV. \$\endgroup\$
    – Trang Oul
    Nov 9 at 15:52
6
\$\begingroup\$

The least appealing, most tedious part of the current code is the conditional logic to handle data conversion. Since you're dealing with a limited number of columns, you can use a data structure to eliminate the conditionals. (In my experience, smarter or more convenient/suitable data structures are often the most powerful devices to simplify algorithmic logic.) For example, to parse a single line, one could write something like this:

def parse_line(line):
    types = (str, str, int, int, int, float, str)
    raw_vals = [val.strip() for val in line.strip().split(',')]
    return [f(val) for val, f in zip(raw_vals, types)]

And if that line-parsing function existed, the overall bill-parsing function would be pretty trivial:

def read_bills(file_path):
    with open(file_path) as fh:
        return [parse_line(line) for line in fh]

Don't overlook the implicit suggestion here: separate the detailed logic (line parsing) from larger orchestration (opening a file and processing it line by line). The latter can usually be quite simple, hardly worth testing or worrying about too much, while the former often requires more effort to ensure correctness. Reducing the footprint of the code requiring in-depth testing is usually a good move to make.

\$\endgroup\$
2
  • \$\begingroup\$ I don't believe you need the .strip() in line.strip().split(',') as each value is stripped after the split. \$\endgroup\$
    – benh
    Nov 9 at 17:15
  • \$\begingroup\$ @benh Only the no-argument form of str.split() includes the auto-magical behavior of also removing whitespace surrounding the delimiter. For example, do some experimenting with this: 'Foo Bar, Blah, 123'.split(','). Alas, further stripping is required. One could do it all in a single split by using re.split(), but I probably wouldn't take that route -- extra complexity with little benefit. \$\endgroup\$
    – FMc
    Nov 9 at 18:23
4
\$\begingroup\$

Overlapping somewhat with @FMc:

  • Probably should avoid making a bills list; just use a generator
  • Use the built-in csv and dataclass modules to make your life easier
  • Prefer a deserialization routine that uses column names instead of column indices.
  • I see that you're using float for your billed amount but this is not appropriate. For monetary numerics use Decimal unless you have a very good reason.
  • Make an actual date object out of your date fields.

Suggested

from csv import DictReader
from dataclasses import dataclass
from datetime import date
from decimal import Decimal
from typing import Any, TextIO, Iterator


@dataclass
class BillLine:
    company: str
    customer: str
    when: date
    amount: Decimal

    @classmethod
    def from_dict(cls, data: dict[str, Any]) -> 'BillLine':
        when = date(
            int(data['Year']),
            int(data['Month']),
            int(data['Day']),
        )

        amount = Decimal(data['Amount'])
        if amount < 0:
            raise ValueError('Negative amount disallowed; use "debit"')

        credit = data['credit/debit']
        if credit == 'debit':
            amount = -amount
        elif credit != 'credit':
            raise ValueError(f'"{credit}" is an unrecognized credit state')

        return cls(
            company=data['company'],
            customer=data['customer'],
            when=when, amount=amount,
        )

    @classmethod
    def from_csv(cls, file: TextIO) -> Iterator['BillLine']:
        csv = DictReader(file)
        for record in csv:
            yield cls.from_dict(record)


def test() -> None:
    with open('bills.csv', newline='') as file:
        records = tuple(BillLine.from_csv(file))


if __name__ == '__main__':
    test()
\$\endgroup\$

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.