3
\$\begingroup\$

I want to read only 1 column from file and save it in the list column. I have this code and I don't know how to optimize it. It takes lots of time because I need to call it a lot of times.

column = (sum(1 for line in open(file) if line.strip("\n")))*[0]
counter_row = 0
counter_column = 0
with open(file, 'r') as f:
    for row in f:
        row = row.strip("\n")
        if row:
            for el_column in row.split(","):
                if counter_column==n_column:
                     column[counter_row]=el_column
                counter_column = counter_column + 1
        counter_row = counter_row + 1
        counter_column = 0
\$\endgroup\$
  • \$\begingroup\$ Are you also reading each file multiple times to grab different columns? For example, do you read files A,B and C to get column 1, and then read them all again to get column 2, and so on? \$\endgroup\$ – CAB Sep 28 '16 at 20:28
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Sep 28 '16 at 21:51
11
\$\begingroup\$

Your biggest problem is the first line:

column = (sum(1 for line in open(file) if line.strip("\n")))*[0]

Opening the file twice is a bad idea for performance, probably much worse than not knowing how large of a list to allocate. Furthermore, that statement only allocates one element per non-empty line, whereas the counter_row counts every line in the file. So, the code would crash with an IndexError if there are any empty lines.

The inner for loop is just a complicated way to do indexing.

Assuming that you want empty lines to be represented by 0, you could write:

column = []
with open(file) as f:
    for line in f:
        row = line.strip("\n")
        column.append(row.split(",")[n_column] if row else 0)

If you don't need to test for empty lines, then you could just write:

with open(file) as f:
    column = [line.strip("\n").split(",")[n_column] for line in f]

You should consider using the csv module, though, which can handle CSV quoting properly.

\$\endgroup\$
7
\$\begingroup\$

It's hard to follow the code when it has no comments. I'll take a stab at it, though.

It looks like you read through each file twice; once to count the number of lines and once to process each line. That is very wasteful.

The inner for loop works through each column to find one, you could simply index.

column = []
with open(file, 'r') as f:
    for row in f:
        row = row.strip("\n")
        if ',' in row:
            column.append( row.split(",")[n_column] )
        else:
            column.append( 0 );

If you are reading files multiple time to read successive columns, you could read all columns in one pass, instead.

# I'm going to assume each file has a fixed number of columns
N_COL = 5

columns = [[]] * N_COL               # N_COL empty columns
with open(file, 'r') as f:
    for row in f:
        row = row.strip("\n")
        if ',' in row:
            values = row.split(",")  # break each row into N_COL values
        else:
            values = [0]*N_COL       # blank row treated as N_COL zeros

        # append the i'th value to the i'th column
        for i in range(N_COL)):
            columns[i].append( values[i] )

After this columns[0] is the first column of values, columns[1] the second, and so forth.

This might not work if there a a huge number of files, or each file is huge. You might run out of memory to hold all the data.

\$\endgroup\$
  • \$\begingroup\$ If n_column is the last column, then the newlines will get copied. \$\endgroup\$ – 200_success Sep 28 '16 at 20:15

Not the answer you're looking for? Browse other questions tagged or ask your own question.