Good Python style?

[Andreas Beyer]

Hi,

I found the following quite cryptic code, which basically reads the
first column of some_file into a set.
In Python I am used to seeing much more verbose/explicit code. However,
the example below _may_ actually be faster than the usual "for line in ..."
Do you consider this code good Python style? Or would you recommend to
refrain from such complex single-line code??

Thanks!
Andreas

inp = resource(some_file)
# read first entries of all non-empty lines into a set
some_set = frozenset([line.split()[0] for line in \
filter(None, [ln.strip() for ln in inp])])

 

[Steven D'Aprano]

Hi,

I found the following quite cryptic code, which basically reads the
first column of some_file into a set.
In Python I am used to seeing much more verbose/explicit code. However,
the example below _may_ actually be faster than the usual "for line in ..."
Do you consider this code good Python style? Or would you recommend to
refrain from such complex single-line code??

Thanks!
Andreas

inp = resource(some_file)
# read first entries of all non-empty lines into a set
some_set = frozenset([line.split()[0] for line in \
filter(None, [ln.strip() for ln in inp])])

It's a little complex, but not excessively so. Any more, and it would
probably be too complex.

It would probably be easier to read with more readable names and a few
comments:

some_set = frozenset(
# ... from a list comp of the first word in each line
[line.split()[0] for line in
# ... each line has leading and trailing white space
# filtered out, and blanks are skipped
filter(None, # strip whitespace and filter out blank lines
[line.strip() for line in input_lines]
)])

Splitting it into multiple lines is self-documenting:

blankless_lines = filter(None, [line.strip() for line in input_lines])
first_words = [line.split()[0] for line in blankless_words]
some_set = frozenset(first_words)

As for which is faster, I doubt that there will be much difference, but I
expect the first version will be a smidgen fastest. However, I'm too lazy
to time it myself, so I'll just point you at the timeit module.

 

[Virgil Dupras]

Hi,

I found the following quite cryptic code, which basically reads the
first column of some_file into a set.
In Python I am used to seeing much more verbose/explicit code. However,
the example below _may_ actually be faster than the usual "for line in ..."
Do you consider this code good Python style? Or would you recommend to
refrain from such complex single-line code??

Thanks!
Andreas

inp = resource(some_file)
# read first entries of all non-empty lines into a set
some_set = frozenset([line.split()[0] for line in \
filter(None, [ln.strip() for ln in inp])])

I think it would be more readable if you would take the filter out of
the list comprehension, and the list comprehension out of the set.

inp = resource(some_file)
stripped_lines = (ln.strip() for ln in inp)
splitted_lines = (line.split()[0] for line in stripped_lines if line)
some_set = frozenset(splitted_lines)

 

[Michael Hoffman]

It would probably be easier to read with more readable names and a few
comments:
[...]

Splitting it into multiple lines is self-documenting:

blankless_lines = filter(None, [line.strip() for line in input_lines])
first_words = [line.split()[0] for line in blankless_words]
some_set = frozenset(first_words)

Re-writing code so that it is self-documenting is almost always a better
approach. Premature optimization is the root of all evil.

 

[Alex Martelli]

Hi,

I found the following quite cryptic code, which basically reads the
first column of some_file into a set.
In Python I am used to seeing much more verbose/explicit code. However,
the example below _may_ actually be faster than the usual "for line in ..."
Do you consider this code good Python style? Or would you recommend to
refrain from such complex single-line code??

Thanks!
Andreas

inp = resource(some_file)
# read first entries of all non-empty lines into a set
some_set = frozenset([line.split()[0] for line in \
filter(None, [ln.strip() for ln in inp])])

Sparse is better than dense, and this code is far from the fastest one
could write -- it builds useless lists comprehensions where genexps
would do, it splits off all whitespace-separated words (rather than just
the first one) then tosses all but the first away, etc.

frozenset(first_word_of_line
for line in input_file
for first_word_of_line in line.split(None, 1)[:1]
)

does not sacrifice any speed (on the contrary), yet achieves somewhat
better clarity by clearer variable names, reasonable use of whitespace
for formatting, and avoidance of redundant processing.

If this set didn't have to be frozen, building it in a normal for loop
(with a .add call in the nested loop) would no doubt be just as good in
terms of both clarity and performance; however, frozen sets (like
tuples) don't lend themselves to such incremental building "in place"
(you'd have to build the mutable version, then "cast" it at the end; the
alternative of making a new frozenset each time through the loop is
clearly unpleasant), so I can see why one would want to avoid that.

A good, readable, and only slightly slower alternative is to write a
named generator:

def first_words(input_file):
for line in input_file:
first_word_if_any = line.split(None, 1)
if first_word_if_any:
yield first_word_if_any[0]

ff = frozenset(first_words(input_file))

A full-fledged generator gives you more formatting choices, an extra
name to help, and the possibility of naming intermediate variables.
Whether it's worth it in this case is moot -- personally I find the
"slice then for-loop" idiom (which I used in the genexp) just as clear
as the "test then index" one (which I used in first_words) to express
the underlying notion "give me the first item if any, but just proceed
without giving anything if the list is empty"; but I can imagine
somebody else deciding that the test/index idiom is a more direct
expression of that notion. You could use it in a genexp, too, of
course:

frozenset(first_word_if_any[0]
for line in input_file
for first_word_if_any in [line.split(None, 1)]
if first_word_if_any
)

but this requires the "for x in [y]" trick to simulate the "x=y"
assigment of an intermediate variable in a genexp or LC, which is never
an elegant approach, so I wouldn't recommend it.


Alex