4
\$\begingroup\$

Python program adds a value to the first number in a fixed width file of variable length. Normally one line header. Before user deletes original file and renames new file they need to check it.

How can the error handling be improved?

# Add number to first number on each line of file then write to another file
#
# Initialise
infile = 'egtxt.prn'
hdrln = 1
llen = 0
fw = 7  # Field width of first number on line
add = 80.0  # Add value to first number on line

print ('Processing {}, first number is {} characters wide, adding {} ...'.format(infile, fw, add))

ifile = open(infile, 'r')
tfile = open(infile+'.out', 'w')
# Skip header
line = ifile.readline()
tfile.write (line)

while True:
    line = ifile.readline()
    if not line:
        break
    llen = len(line) - fw
    tfile.write ('{:{sfw}.{sprec}f}{}'.format(float(line[:fw])+add, line[-llen:], sfw=str(fw), sprec='2'))

# Finalise
ifile.close()
tfile.close()
print (' done.')

Sample egtxt.prn follows:

ID      Name               Address                         Comment
   2.88 Barnett, Irene A.  P.O. Box 829, 4484 Aliquet Ave  viverra. Maecenas iaculis
  30.58 Irwin, Honorato X. Ap #446-1219 Nisi. Road         dui
3428.21 Talley, Bree S.    862-3586 Duis Ave               leo. Morbi
 176.12 Hensley, Kai T.    Ap #842-3806 Class Street       eu
\$\endgroup\$
  • \$\begingroup\$ When you say "The wrong format is written in the second last line in the .out file." it sounds like this question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$ – Simon Forsberg May 14 '17 at 10:36
  • \$\begingroup\$ @Simon Forsberg I'll let the code stand as it is without the runtime error that I put in the data. \$\endgroup\$ – flywire May 14 '17 at 10:59
  • \$\begingroup\$ I've reopened your question now \$\endgroup\$ – Simon Forsberg May 14 '17 at 12:20
  • \$\begingroup\$ Note topic is ERROR HANDLING. Say there is a blank line in data or output is read-only. \$\endgroup\$ – flywire May 14 '17 at 13:08
3
\$\begingroup\$

Constants uppercase

A very widespread convention is that constants should be uppercase, so please conform to the standard for readability.

Unused variables

hdrln = 1
llen = 0

The first one is never used, the second one is never used with the original value, so I suggest removing that declaration.

with and iterable interface for files

with is a commonly used to keyword to simplify working with files to take care of automatic closing, also files are iterable so you can just do: for line in file that should simplify your code.

Isolate logic in functions

tfile.write ('{:{sfw}.{sprec}f}{}'.format(float(line[:fw])+add, line[-llen:], sfw=str(fw), sprec='2'))

That should be in its function along with documentation because it is the main part of the program, the logic should be highlighted and explained not hidden inside the boilerplate.

\$\endgroup\$
  • \$\begingroup\$ Constants as uppercase: My intention is that these constants will become variables. Unused variables: hdrln will be used, thanks for pointing out llen does not need to be initialised. \$\endgroup\$ – flywire May 14 '17 at 13:01
  • 1
    \$\begingroup\$ Agree with using for statement but I used with statement to try to trap for file handling errors. Isolate logic: good point. Trying to get comfortable with python. \$\endgroup\$ – flywire May 14 '17 at 13:04
2
\$\begingroup\$

As a first rewrite, I would come up with the following:

import contextlib


ID_WIDTH = 7


def process_file(input_filename, value_to_add):
    """Add number to first number on each line then write to another file"""
    with contextlib.ExitStack as stack:
        in_file = stack.enter_context(open(input_filename))
        out_file = stack.enter_context(open(input_filename + '.out', 'w'))

        out_file.write(next(in_file))  # Skip header
        for line in in_file:
            line_id = float(line[:ID_WIDTH]) + value_to_add
            out_file.write('{:{width}.2f}{}'.format(line_id, line[ID_WIDTH:], width=ID_WIDTH))


if __name__ == '__main__':
    filename = 'egtxt.prn'
    value_to_add = 80.0
    print('Processing', filename, '/ First number is', ID_WIDTH, 'characters wide / Adding', value_to_add, '...')
    process_file(filename, value_to_add)
    print('Done')

Important key-points being:

  • usage of contextlib.ExitStack to properly close every files opened whatever happens
  • usage of a function to encapsulate, test and reuse behaviour
  • usage of docstrings instead of comments for added value
  • usage of the iterator protocol on files to iterate line by line
  • simplification of constants usage by using slicing or directly integrating them in other constants ('.{sprec}f' became '.2f' for instance).

Now, specifying the width of the first field for parsing purposes feels terribly error-prone. You may cut an overly long ID and end up with weird/broken results. Instead, I would rely on the .split() method of strings to extract out this first field. This is easy using the optional maxsplit parameter:

>>> "   spam \t eggs bacon foo bar  baz spam  ".split(maxsplit=1)
['spam', 'eggs bacon foo bar  baz spam  ']

You would just need to use the length of the field for formatting purposes but should add back the separator (a TAB I guess) between this field and the rest of the line.

As you also mentionned empty lines in the comments, you can test the length of the list returned by split and decide whether you continue processing it or not. All in all, this may yield something like:

import contextlib


def process_file(input_filename, value_to_add, id_width=7, sep='\t'):
    """Add number to first number on each line then write to another file"""
    with contextlib.ExitStack as stack:
        in_file = stack.enter_context(open(input_filename))
        out_file = stack.enter_context(open(input_filename + '.out', 'w'))

        out_file.write(next(in_file))  # Skip header
        for line in in_file:
            try:
                line_id, line = line.split(maxsplit=1)
                line_id = float(line_id) + value_to_add
            except ValueError:
                continue
            out_file.write('{:{width}.2f}{}{}'.format(line_id, sep, line, width=id_width))


if __name__ == '__main__':
    filename = 'egtxt.prn'
    value_to_add = 80.0
    print('Processing', filename, 'Adding', value_to_add, '...')
    process_file(filename, value_to_add)
    print('Done')
\$\endgroup\$
  • \$\begingroup\$ I like it! (Sorry I can't format comment) Note: Multiple header lines, variable precision, fixed width not tab delimited but assume space. Unfortunately I can't seem to import contextlib to Win7 Python 3.6.0. runtime error: File "me.py", line 6, in process_file with contextlib.ExitStack as stack: AttributeError: enter \$\endgroup\$ – flywire May 17 '17 at 11:16
  • \$\begingroup\$ This generates an ExitStack error, Otherwise it would be good. \$\endgroup\$ – flywire Jun 3 '17 at 11:00
1
\$\begingroup\$

Happy for any more comments but I think this is getting better:

# Copy input file adding number to first number of each line from input file
# Inputs: in_file, header_lines, float_width, float_precision, add_number
# eg: addnumber.py egtxt.prn 2 7 2 80.0
#
import sys
if len(sys.argv) != 6:
    format_string = '  Usage {} input_file header_lines number_width number_precision add_number'
    print(format_string.format(sys.argv[0]))
    exit()

# Initialise
in_file = sys.argv[1]
header_lines = int(sys.argv[2])
float_width = int(sys.argv[3])
float_precision = int(sys.argv[4])
add_number = float(sys.argv[5])

format_string = '\nProcessing {}, {} header lines, first number is {}.{} format, adding {} ...'
print(format_string.format(in_file, header_lines, float_width, float_precision, add_number))

with open(in_file, 'r') as fin, open(in_file+'.out', 'w') as fout:
    temp_filename = fout.name
    itogo = header_lines
    for line in fin:
        if itogo:
            # Header lines
            fout.write(line)
            itogo -= 1
        else:
            # Add number
            llen = len(line) - float_width
            format_string = '{:{sfw}.{sprec}f}{}'
            fout.write(format_string.format(float(line[:float_width])+add_number, line[-llen:], sfw=str(float_width), sprec=str(float_precision)))

print(' done.')
\$\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.