2
\$\begingroup\$

I wrote this below script which basically replaces the one of the column in the df with another column with incremental IDs. The script works perfectly fine but I was wondering if there is a better/efficient way to write this one.

import sys

fh_in = open(sys.argv[1], 'r')
fh_out = open(sys.argv[2], 'w')

count = 1
dict = {}
final = list()

for line in fh_in:
    line = line.strip()
    line = line.split()
    if count < 10:
        dict[line[3]] = "Bra100000" + str(count)
        count = count + 1
    else:
        dict[line[3]] = "Bra10000" + str(count)
        count = count + 1
    test = "{:10s}" .format(dict[line[3]])
    fh_out.write(line[0])
    fh_out.write("\t")
    fh_out.write(line[1])
    fh_out.write("\t")
    fh_out.write(line[2])
    fh_out.write("\t")
    fh_out.write(line[1])
    fh_out.write("\t")
    fh_out.write(test)
    fh_out.write("\t")
    fh_out.write(line[4])
    fh_out.write("\t")
    fh_out.write(line[5])
    fh_out.write("\t")
    fh_out.write(line[6])
    fh_out.write("\t")
    fh_out.write(line[7])
    fh_out.write("\t")
    fh_out.write(line[8])
    fh_out.write("\t")
    fh_out.write(line[9])
    fh_out.write("\t")
    fh_out.write(line[10])
    fh_out.write("\t")
    fh_out.write(line[11])
    fh_out.write("\n")

Input:

A01     158188  158533  Contig545|m.1503        228     +       158188  158533  255,0,0 1       345     0
A01     272139  272465  comp285432_c0_seq1|m.9436       230     -       272139  272465  255,0,0 1       326     0
A01     339617  340806  TCONS_00003584|m.11226  157     -       339617  340806  255,0,0 5       95,127,68,50,432        0,190,467,619,757
A01     888838  889347  Contig1477|m.3679       92      -       888838  889347  255,0,0 1       509     0
A01     1165488 1165761 comp3043338_c0_seq1|m.9546      228     -       1165488 1165761 255,0,0 1       273     0
A01     1167009 1167386 Contig1598|m.4011       238     -       1167009 1167386 255,0,0 1       377     0
A01     1234347 1234827 comp260850_c0_seq1|m.10201      85      +       1234347 1234827 255,0,0 1       480     0

Output:

A01     158188  158533  158188  Bra1000001      228     +       158188  158533  255,0,0 1       345     0
A01     272139  272465  272139  Bra1000002      230     -       272139  272465  255,0,0 1       326     0
A01     339617  340806  339617  Bra1000003      157     -       339617  340806  255,0,0 5       95,127,68,50,432        0,190,467,619,757
A01     888838  889347  888838  Bra1000004      92      -       888838  889347  255,0,0 1       509     0
A01     1165488 1165761 1165488 Bra1000005      228     -       1165488 1165761 255,0,0 1       273     0
A01     1167009 1167386 1167009 Bra1000006      238     -       1167009 1167386 255,0,0 1       377     0
A01     1234347 1234827 1234347 Bra1000007      85      +       1234347 1234827 255,0,0 1       480     0
\$\endgroup\$
1
\$\begingroup\$

It would be a whole lot better this way:

import sys


def main(filename_in, filename_out):
    with open(filename_in) as fh_in:
        with open(filename_out, 'w') as fh_out:
            count = 1
            for line in fh_in:
                parts = line.strip().split()
                if count < 10:
                    bra = "Bra100000" + str(count)
                else:
                    bra = "Bra10000" + str(count)
                count += 1
                test = "{:10s}" .format(bra)
                fh_out.write("\t".join(parts[0:3]))
                fh_out.write("\t")
                fh_out.write(parts[1])
                fh_out.write("\t")
                fh_out.write(test)
                fh_out.write("\t")
                fh_out.write("\t".join(parts[4:12]))
                fh_out.write("\n")

if __name__ == '__main__':
    main(sys.argv[1], sys.argv[2])

Improvements:

  • Use with open(...) as ...:. This way the file handles will be correctly closed when exiting the block
  • Drop the unused variable final
  • Also drop the pointless variable dict
  • Since both branches of the if-else increment count, you can move it outside
  • Instead of count = count + 1, it's simpler as count += 1
  • Instead of the repetitive write, simplify with join
  • Do not just dump the code in the global namespace: put it inside a function. This way the command line arguments will have meaningful names
  • Instead of stripping and splitting line in 2 steps, do it in one: line.strip().split()
  • It's not a good practice to overwrite loop variables and function parameters. line is a good name for a line of input, splitting that becomes something else and "line" is no longer an appropriate name, so I called it "parts" instead
\$\endgroup\$
  • \$\begingroup\$ Very nice suggestions and improvements. Needs to learn a lot it seems. I have one question though. Is there a particular advantage of making the whole code as function? \$\endgroup\$ – upendra Oct 28 '14 at 0:03
  • \$\begingroup\$ In addition to what I already mentioned about the parameters having meaningful names, this makes it possible to use the script both as standalone script, and as a package that you can import from other packages or scripts, most notably unit tests. You might not need that now, but the if __name__ == '__main__': stuff is a good practice to get used to early on. \$\endgroup\$ – janos Oct 28 '14 at 0:05
  • \$\begingroup\$ Great....thanks. Will try to use your guidelines to write readable and beautiful code..Modifying my script now using git.. \$\endgroup\$ – upendra Oct 28 '14 at 0:12
  • \$\begingroup\$ OP could replace values in the existing list and use a single ','.join(line) to write the whole thing. \$\endgroup\$ – tdelaney Oct 28 '14 at 0:28
  • \$\begingroup\$ Also, if count < 10 and the following format could be elimiated with a simple format "Bra1%06d" % count. \$\endgroup\$ – tdelaney Oct 28 '14 at 0:29
1
\$\begingroup\$

The final result:

#!/usr/bin/env python

import sys
from argparse import ArgumentParser


def main(argv=None):
    args = parse_argv(argv=argv)
    with open(args.input_path) as input_file, \
            open(args.output_path, 'w') as output_file:
        process_file(input_file, output_file)
    return 0


def parse_argv(argv=None):
    if argv is None:
        argv = sys.argv
    parser = ArgumentParser()
    parser.add_argument('input_path', metavar='INPUT')
    parser.add_argument('output_path', metavar='OUTPUT')
    return parser.parse_args(args=argv[1:])


def process_file(input_file, output_file):
    for line_count, input_line in enumerate(input_file, start=1):
        output_file.write(process_line(line_count, input_line))


def process_line(line_count, line, prefix='Bra', offset=10000000, sep='\t',
                 end='\n'):
    row = line.strip().split()
    row[3] = row[1]
    row.insert(4, '{:10s}'.format('{}{:d}'.format(prefix,
                                                  offset + line_count)))
    return '{}{}'.format(sep.join(row), end)


if __name__ == '__main__':
    sys.exit(main())

General notes

  • Instead of nesting, make a new function. It's more readable, maintainable, reusable, and testable.

  • Use defaults arguments of built-ins e.g. don't pass 'r' to open.

  • Try to keep business login out of the module level and inside functions and classes. It gives you more control over how the code is executed, making it easier to reuse and test.


Include a Hashbang for *nix users

#!/usr/bin/env python

Use argparse to process command line arguments. It validates what's passed and provides help and usage automatically.

from argparse import ArgumentParser

Allow your code to be executed by other code by accepting argv from anywhere not just sys.argv.

def main(argv=None):
    args = parse_argv(argv=argv)

Use context managers to ensure files are closed.

    with open(args.input_path) as input_file, \
            open(args.output_path, 'w') as output_file:

Use enumerate to count lines, it's easier than doing it yourself.

def process_file(input_file, output_file):
    for line_count, input_line in enumerate(input_file, start=1):
        output_file.write(process_line(line_count, input_line))

Use format to do all the formatting and join instead of writing your own separators.

    row.insert(4, '{:10s}'.format('{}{:d}'.format(prefix,
                                                  offset + line_count)))
    return '{}{}'.format(sep.join(row), end)
\$\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.