8 ms·

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

• BeetleB 3y ago
  You could try to squeeze it all into a list comprehension.
• thrdbndndn 3y ago
  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))]`)
  • roywiggins 3y ago
    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 ] or return list(map(get_record, rows))
    • thrdbndndn 3y ago
      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".
      • gen220 3y ago
        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. :)
        • proamdev123 3y ago
          > 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.
          • sanderjd 3y ago
            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.
          • gen220 3y ago
            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-validate/ 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.
  • nurbl 3y ago
    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.
• ayhanfuat 3y ago
  Why don't you like it? I am asking because you are asking for a better way. Better in what way?
• Sentack 3y ago
  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.
• senex 3y ago
  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.
• rjh29 3y ago
  You could make read_portfolio a generator ( https://wiki.python.org/moin/Generators 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.
• nicwolff 3y ago
  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)
  • drexlspivey 3y ago
    or use the built-in csv.DictReader :)
• felixhummel 3y ago
  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))
• WesolyKubeczek 3y ago
  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 ]
• deleted 3y ago
  [deleted]
• tomn 3y ago
  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.
  • CogitoCogito 3y ago
    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.
    • tomn 3y ago
      > 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.
• drcongo 3y ago
  I actually kinda like that pattern in terms of readability, though I think a generator would outperform it.
  • travisjungroth 3y ago
    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.
• rami3l 3y ago
  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.
• mekoka 3y ago
  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]
  • KMnO4 3y ago
    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
    • tomn 3y ago
      If you do that, it will try to read from a closed file.
  • kstrauser 3y ago
    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 ]
• PennRobotics 3y ago
  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.
  • CogitoCogito 3y ago
    > 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.
    • philsnow 3y ago
• deleted 3y ago
  [deleted]
• twism 3y ago
  Sigh (re: sibling comments) whatever happened to PEP 20 in particular: ``` There should be one-- and preferably only one --obvious way to do it. ```
  • BerislavLopac 3y ago
    This is absolutely the most misinterpreted line in the Zen of Python. The key word there is obvious, not one.