1
\$\begingroup\$

I am working on a script for an automated workflow. It expects a CSV and a disk image to be present in the directory that is supplied as args.input.

I want to check for and handle every possible scenario: no CSV, no disk image, too many CSVs, too many disk images, and any possible combination thereto.

I've written the below, which works, and seems to be human readable, but it just seems incredibly excessive and verbose – is there any way I can make this more compact, yet still retain its legibility?

# CONFORMANCE CHECKS
def check_conformance():
    csv = glob.glob(args.input+'*.csv')
    disk_image = glob.glob(args.input+'*.E01')

    if len(csv) == 1:
        does_csv_exist = os.path.isfile(csv[0])
    elif len(csv) < 1:
        does_csv_exist = False
    elif len(csv) > 1:
        does_csv_exist = "too many CSVs!"
    if len(disk_image) == 1:
        does_E01_exist = os.path.isfile(disk_image[0])
    elif len(disk_image) < 1:
        does_E01_exist = False
    elif len(disk_image) > 1:
        does_E01_exist = "too many Disk Images!"
    if len(disk_image) > 1 and len(csv) > 1:
        does_csv_exist = "too many CSVs!"
        does_E01_exist = "too many disk images!"
        return (False, does_csv_exist, does_E01_exist,)
    if does_E01_exist is True and does_csv_exist is True:
        return True
    elif does_E01_exist is True and does_csv_exist is False:
        return (False, "CSV is missing")
    elif does_E01_exist is False and does_csv_exist is True:
        return (False, "E01 disk image is missing")
    elif does_E01_exist is False and does_csv_exist is False:
        return (False, "E01 disk image AND csv are missing")
    elif does_csv_exist is not True and does_csv_exist is not False:
        return (False, does_csv_exist)
    elif does_E01_exist is not True and does_E01_exist is not False:
        return (False, does_E01_exist)
\$\endgroup\$
  • 1
    \$\begingroup\$ Please add more context. What is args? Who calls this function, and what does it do with the return value? \$\endgroup\$ – 200_success Feb 17 '16 at 21:30
3
\$\begingroup\$

I would recommend decomposing this function into smaller functions. For example, you would end up with something like this:

def check_conformance():
   csv_conformance  = check_csv_conformance()
   disk_conformance = check_disk_conformance()

Then you can return based on these checks as follows:

return csv_conformance && disk_conformance

Moreover, as a general rule, it is a bad habit to have different amount of return values based on branching. You should have a specific input-output length (again, in general. there are cases where you want a variable length array to be return)

\$\endgroup\$
1
\$\begingroup\$

First of all, instead of doing a is True and b is True just do a and b - True is implied, and it makes it a lot easier to read.

The names could be better - instead of does_EO1_exist, just call it EO1_exists - it reads better.

In general, though, it's better to beg forgiveness than ask permission in Python. I've found this to be especially true with file systems - things can change while your program is running. A cleaner way of doing this, and a more pythonic one, is

try:
    do_something_with_file()
except FileNotFoundException:
    print "The file doesn't exist"
except ImageNotFoundException:
    print "Disk image doesn't exist"

# these may not be real errors, you'll probably not want to do this verbatim

Generally, you'd expect that both things are true (right?), so doing a bunch of checks before seems unnecessary.

\$\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.