8
\$\begingroup\$

I have written the following function to process images. Now I am not sure if it's pythonic or considered as spaghetti code. I know being flat is better than nested, but I don't see a way around it.

I could easily apply list comprehensions if the "if" statements are only resolving true/false, but each step depends on the output of the previous steps, so I don't think I could use itertools. 

def get_image(width, height, collection_sku, order_id, name, handle):
    """image processing pipeline"""

    # Check-1 image exception should be checked before proceeding further
    # covers both images in exception(currently bookshelves) list and also personal true doors
    #   1. bookshelves need to be cropped
    #   2. personal images needs to be added in the required size
    if helpers.image_exception(collection_sku):

        # Step-1 Find image folder
        directory_sizes = helpers.find_directory(width, height)

        if directory_sizes is not None:
            # Step-2 Find the image from the given folder.
            key = helpers.find_image(directory_sizes, collection_sku)

            if key is not None:

                requires_resize = helpers.should_resize(width, height)
                requires_flip = helpers.should_flip(handle) and not helpers.flip_exception(collection_sku)

                # Check-2 check whether image requires resizing or flipping
                if requires_resize or requires_flip:

                    # Step-3 Create local directory to store temporary images
                    location, is_created = helpers.local_directory(
                        order_id)

                    if is_created:
                        # Step-4 Download image
                        image = helpers.download_image(key, location, name)

                        if image is not None:
                            width_px = helpers.cm_to_pixel(width)
                            height_px = helpers.cm_to_pixel(height)
                            image_modified = False

                            # Step-5.1 Resize image
                            if requires_resize:
                                helpers.resize_image(image, width_px, height_px)
                                image_modified = True

                            # Step-5.2 Flip image
                            if requires_flip:
                                helpers.flip_image(image)
                                image_modified = True

                            # Step-6 Upload image
                            if image_modified:
                                helpers.upload_image(image, key)

                    # Step-3 No local dir created
                    # Step-4 Can't download image
                    # Step-5.1 Can't resize image
                    # Step-5.2 Can't flip image
                    # Step-6 Can't upload image
                    return None

                # Check-2 if no image processing required then copy image over
                helpers.copy_image(key, target=None) # TODO
                return True

    # raise exception in sub job
    #   Check-1 if image door image cannot be processed
    #   Step-1 if directory not available
    #   Step-2 if image not available

    return None
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers. \$\endgroup\$ Commented May 31, 2018 at 23:03

1 Answer 1

Reset to default
6
\$\begingroup\$

As Reinderien mentions, you can decrease some of the complexity of your code by inverting your if-statements. While I cannot test this because I do not have your whole code, here is how it could be done:

def get_image(width, height, collection_sku, order_id, name, handle):
    """image processing pipeline"""

    # Check-1 image exception should be checked before proceeding further
    # covers both images in exception(currently bookshelves) list and also personal true doors
    #   1. bookshelves need to be cropped
    #   2. personal TD images needs to be added in the required size when it's submitted by the user.
    if not helpers.image_exception(collection_sku):
        return None

    # Step-1 Find image folder
    directory_sizes = helpers.find_directory(width, height)

    if directory_sizes is None:
        return None

    # Step-2 Find the image from the given folder.
    key = helpers.find_image(directory_sizes, collection_sku)

    if key is None:
        return None

    requires_resize = helpers.should_resize(width, height)
    requires_flip = helpers.should_flip(handle) and not helpers.flip_exception(collection_sku)

    # Check-2 check whether image requires resizing or flipping
    if requires_resize or requires_flip:

        # Step-3 Create local directory to store temporary images
        location, is_created = helpers.local_directory(
            order_id)

        if not is_created:
            return None

        # Step-4 Download image
        image = helpers.download_image(key, location, name)

        if image is not None:
            width_px = helpers.cm_to_pixel(width)
            height_px = helpers.cm_to_pixel(height)
            image_modified = False

            # Step-5.1 Resize image
            if requires_resize:
                helpers.resize_image(image, width_px, height_px)
                image_modified = True

            # Step-5.2 Flip image
            if requires_flip:
                helpers.flip_image(image)
                image_modified = True

            # Step-6 Upload image
            if image_modified:
                helpers.upload_image(image, key)

        # Step-3 No local dir created
        # Step-4 Can't download image
        # Step-5.1 Can't resize image
        # Step-5.2 Can't flip image
        # Step-6 Can't upload image

    # Check-2 if no image processing required then copy image over
    helpers.copy_image(key, target = None) # TODO
    return True

    # raise exception in sub job
    #   Check-1 if image door image cannot be processed
    #   Step-1 if directory not available
    #   Step-2 if image not available

This, for me, vastly increases readability -- before, it was hard to figure out which if-statements do what (especially with fragments of code at the ends of them). However, your comments are useful in describing the process as it goes along.

I'm sure it could be flattened more, but this should demonstrate the idea of it. Hope this helps!

\$\endgroup\$
2
  • \$\begingroup\$ Does look more readable indeed, thanks for this suggestion, much appreciated. For some reason I thought that adding return statements in "Ifs" in a scenario like above was discouraged. You do mention that it can be flattened further. I was wondering the same, where do we draw the line in the above implementation? Because if I look at it, I can make it more flat by adding more return statements inside the Ifs. Is there a convention ? \$\endgroup\$ Commented Jun 1, 2018 at 10:37
  • 1
    \$\begingroup\$ @Vinod I would say you could stop flattening when it becomes tedious or hard to follow. For example, If you get complex "if x and not y or not z and x or y" types of if-statements, or many variances on if-statements that do the same thing (ie "if x and not y and not z: return 35" with "if x and z and w: return 35"), it may mean you've gone too far. There are no definitive rules on this type of thing, it just comes down to how easily you and your peers can read and understand the code. \$\endgroup\$ Commented Jun 1, 2018 at 17:59

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.