Hacker News Reader

Comment by kingkongjaffa | Hacker News Reader

Something I hate in my own code is this pattern of instantiating an empty list and then iterating on it when reading files. Is there a better way than starting lst= [] and then later doing lst.append()

This is an example from the linked course https://github.com/dabeaz-course/python-mastery/blob/main/Ex...:

``` # readport.py

import csv

# A function that reads a file into a list of dicts

def read_portfolio(filename):

    portfolio = []

    with open(filename) as f:

        rows = csv.reader(f)

        headers = next(rows)

        for row in rows:

            record = {

                'name' : row[0],

                'shares' : int(row[1]),

                'price' : float(row[2])

            }

            portfolio.append(record)

    return portfolio

```

View on HN

Whenever you see this pattern, think of using a generator instead:

    def read_portfolio(filename):
        with open(filename) as f:
            rows = csv.reader(f)
            headers = next(rows)
            for row in rows:
                yield {
                    'name' : row[0],
                    'shares' : int(row[1]),
                    'price' : float(row[2]),
                }

Now you can call read_portfolio() to get an iterable that lazily reads the file and yields dicts:

    portfolio = read_portfolio()
    for record in portfolio:
        print '{shares} shares of {name} at ${price}'.format_map(record)

or use the built-in csv.DictReader :)

It's not better than a generator, but I'm surprised nobody has mentioned the very terse and still mostly readable

    header, *records = [row.strip().split(',') for row in open(filename).readlines()]

but then you need a way to parse the records, which could be Template() from the string library or something like...

    type_record = lambda r : (r[0], int(r[1]), float(r[2]))

At this point, the two no longer mesh well, unless you would be able to unpack into a function/generator/lambda rather than into a variable. (I don't know but my naive attempts and quick SO search were unfruitful.) Also, you're potentially giving up benefits of the CSV reader. Plus, as others have clarified, brevity does not equal readability or relative lack of bugs:

In the course example, it's reasonably easy to add some try blocks/error handling/default values while assigning records, giving you the chance to salvage valid rows without affecting speed or readability. In fact, error handling would be a necessity if that CSV file is externally accessible. Contrast that with my two lines, where there's not an elegant way to handle a bad row or escaped comma or missing file or virtually any other surprise.

Anything else I can think of off-hand (defaultdict, UserList, "if not portfolio:") has the same initialization step, endures some performance degradation, is more fragile, and/or is needlessly unreadable, like this lump of coal:

    portfolio = [record] if 'portfolio' not in globals() else portfolio + [record]

So... your technique and generators. Those are safe-ish, readable, relatively concise, etc.

> It's not better than a generator, but I'm surprised nobody has mentioned the very terse and still mostly readable

> header, *records = [row.strip().split(',') for row in open(filename).readlines()]

Better would be:

    header, *records = [row.strip().split(',') for row in open(filename)]

No need to read the lines all into memory first.

Edit: Also if you want to be explicit with the file closing, you could do something like:

    with open(filename) as infile:
        header, *records = [row.strip().split(',') for row in infile]

That is if we wanted to protect against future changes to semantics for garbage collection/reference counting. I always do this, but I kind of doubt it will ever really matter in any code I write.

> No need to read the lines all into memory first.

It looks like that code does read the whole file:

(with a foo.csv that is 350955 bytes long:)

  % python -V
  Python 3.11.4
  % python
  >>> f = open("foo.csv")
  >>> f.tell()
  0
  >>> header, *records = [row.strip().split(',') for row in f]
  >>> f.tell()
  350955

I thought that using a list comprehension to bind header and records was eagerly consuming the file, so I changed it to a generator comprehension with

  >>> f.close()
  >>> f.open("foo.csv")
  >>> header, *records = (row.strip().split(',') for row in f)
  >>> f.tell()
  350955

nope, I guess the destructuring bind does it?

  >>> f.close()
  >>> f.open("foo.csv")
  >>> headers, records = f.readline().strip().split(','), (row.strip().split(',') for row in f)
  >>> f.tell()
  125

not as neat, though. Is there a golf-ier way to do it?*

The parent poster was pointing out that this requires having two in-memory complete copies of the file:

    [... for row in open(filename).readlines()]

The readlines return value is one copy, and the list comprehension is another copy. However, that first copy can be avoided with:

    [... for row in open(filename)]

The entire file must still be read to evaluate the list comprehension.

Additionally, this doesn't do what you think it does:

    >>> header, *records = (row.strip().split(',') for row in f)

Compare to this, using a variable for clarity:

    >>> gen = (row.strip().split(',') for row in f)
    >>> header, *records = next(gen)

    def read_portfolio(filename):
        record = lambda r: {
            'name': r[0],
            'shares': int(r[1]),
            'price': float(r[2]),
        }
        with open(filename) as f:
            rows = csv.reader(f)
            headers = next(rows)
            return [record(r) for r in rows]

Swap the square brackets for parentheses in the return statement and it will return a generator expression.

That will read the file as needed (ie as you iterate over it) instead of loading the entire thing in memory.

    for record in read_portfolio(fn):
        # do stuff

If you do that, it will try to read from a closed file.

Or even:

  def read_portfolio(filename):
      with open(filename) as f:
          rows = csv.reader(f)
          headers = next(rows)
          return [
              {
                  "name": r[0],
                  "shares": int(r[1]),
                  "price": float(r[2]),
              }
              for r in rows
          ]

I don't think you can really improve on this.

You could use a list comprehension, but that can be unclear and hard to extend, depending on the situation. It can be a nice option if most of the parts in the generator can be broken out into functions with their own name, though.

You could turn it into a generator, which can cause some fun bugs (e.g. everything works fine when you first iterate over it, but not afterwards), so IMO that's best used when it needs to be a generator, for semantics or performance.

You could turn it into a generator, then add a wrapper that turns it into a list (keeping the inner function private), or use a decorator that does the same, but it's less clear than this pattern.

So, i'd just learn to live with it.

Yeah I think using a list comprehension is overkill. The main reason I like list comprehensions is because I don't introduce variables (even temporarily) that I don't really need. I think that clarifies the code. But putting the code in a separate function also avoids introducing those variables to the current scope only at a cost of putting the code somewhere else (which I personally think has a cost). In this case I would just use a function or (probably) just inline it as you don't like.

> which I personally think has a cost

Yeah, so many people don't get this, but too many small functions can be hard to understand -- that's why I qualified that option.

In this case i agree that inlining it is fine, i was talking about the general pattern.

There is always list comprehension.

So `portfolio = [{'name': row[0], 'shares': int(row[1]), 'price': float(row[2]) for row in rows]`

But if it's more complicated than this (like if there is conditional(s) inside the loop), I'd recommend just stick with the current approach. It's possible to have even multiple conditionals in list comprehension, but it's not really very readable. If you do want to, walrus operator can make things better

(something like `numbers = [m[1] for s in array if (m := re.search(r'^.*(\d+).*$', s))]`)

You can alternately stick the logic into a function, which maintains the readability.

    def get_record(row):
        return {
                'name': row[0],
                'shares': int(row[1]),
                'price': float(row[2])
        }
    return [ get_record(r) for r in rows ]

    return list(map(get_record, rows))

I do like to use ad hoc functions to make things cleaner (mainly for the handy "early" return behavior), but in this case I don't find it's much better than "just create an empty list first and do a for loop".

Personally, I enjoy the pattern of making the return type a dataclass and make this function a static method on the dataclass, something like `def from_data(self, data: Dict) -> PortfolioRow`.

In 2023, it's rude to return dictionaries. :)

> In 2023, it's rude to return dictionaries. :)

Why do you say that?

I think you were partially kidding, but also half serious. What’s the issue with returning dictionaries, and why should we be returning dataclasses instead?

Asking for my own learning.

Data classes are "self-documenting" with respect to the "keys" you can expect to be present. Relatedly, they enable meaningful type hints:

  def some_method(input: Dict[str, int]):
    ...

You can just as easily call `some_method({"foo": 1})` as `some_method({"bar": 2})`.

vs.

  def some_method(input: MyDataClassWithFoo):
    ...

Now you can't pass a dict where the key is "bar" and presumably get a KeyError when it tries to look up a "foo" key, you can only pass a MyDataClassWithFoo.

Regarding the 2023 part, it's because dataclasses in Python are pretty fully-featured and part of the stdlib, so building rich data structures is quick, ergonomic, and helps remove/centralize boilerplate and parsing (aka validating, see https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-va...).

Regarding rude, yea it was a bit tongue-in-cheek (I would never hold a grudge against somebody for returning a dict).

You should generally define an interface for your function that's as precise as its logic allows for. `Dict` is as good as `Any`: it doesn't tell you very much about what the function internally expects. The sibling comment to mine does a good job going through this.

They can be more readable than that at least, e.g.:

    keys = "name", "shares", "price"
    portfolio = [
        dict(zip(keys, row))
        for row in rows
    ]

If I had to do more complex stuff than building a dict like this I'd move it into a function. That tends to make the purpose more clear anyway.

That said, it's fine to append to a list too, I just prefer comprehensions when they fit the job. In particular, if you're just going to iterate once over this list anyway, you can turn it into a iterator comprehension by replacing [] by () and save some memory.

Sigh (re: sibling comments) whatever happened to PEP 20 in particular:

``` There should be one-- and preferably only one --obvious way to do it. ```

This is absolutely the most misinterpreted line in the Zen of Python. The key word there is obvious, not one.

    with open(filename) as f:
        rows = csv.reader(f)
        next(rows)
        return [
            {
                'name': row[0],
                'shares': int(row[1]),
                'price': float(row[2]),
            } for row in rows
        ]

Why don't you like it? I am asking because you are asking for a better way. Better in what way?

You could “yield” the record instead of constructing the list. This makes “read_portfolio” into an iterator instead of returning a list. Use a list comprehension or list constructor to convert the iterator to a list if needed.

You could make read_portfolio a generator ( https://wiki.python.org/moin/Generators ). But that might confuse inexperienced Python programmers.

Personally the way you've done it is the most Pythonic IMO. List comprehensions are great but would be less readable in this case.

If there is no library for your case like pandas (or even csv.DictReader), you could always use an iterator:

    def iter_portfolio(rows):
         for row in rows:
             yield {'name': row[0]}
    
    rows = ...
    portfolio = list(iter_portfolio(rows))

I too have run into this situation, and while list comprehension makes it possible, it's never clean looking.

Honestly, this is the approach I've been using even though I hate it. Specially if your code is going to be read by anyone other than you.

I actually kinda like that pattern in terms of readability, though I think a generator would outperform it.

If it’s a lot of data and part of a pipeline you’ll get memory saving. 1,000 lines and reading from a CSV it won’t matter really.

You could try to squeeze it all into a list comprehension.

I have also encountered this quite often. I'll say the ideal solution would be "postfix streaming methods" like `.filter` and `.map`. Unfortunately, Python doesn't have those (prefix `filter`s and `map`s are not even close), and you have comprehension expressions at best. To make things worse, complex comprehensions can also create confusion, so for your particular example I'll probably say it's acceptable. It could be better if you use unpacking instead of indexing though, as others have pointed out.

Reply on news.ycombinator.com