1
\$\begingroup\$

My __main__.py file has a function's flow, based on methods of each class instance, which each one has its own methods and attributes, like this:

from xlrd                              import open_workbook
from pprint                            import pprint
from rubrica_manager                   import RubricaManager
from rectified_rubrica_manager         import RectifiedRubricaManager
from rectification_spreadsheet_manager import RectificationSpreadsheetManager
from gps_manager                       import GPSManager
from sys                               import exit
from data_comparison                   import DataComparison

def main():
    '''
    Execute PostDiagnosisChecker
    '''
    rm  = RubricaManager("File1.xlsx")
    rrm = RectifiedRubricaManager("File2.xlsx")
    rsm = RectificationSpreadsheetManager("File3.xlsx")
    gm  = GPSManager("File4.xlsx")

    object_collection = [rm, rrm, rsm, gm]

    for current_spreadsheet in object_collection:
        '''
        evaluate each object if it has the method or attribute before executing
        '''
        methods_list = dir(current_spreadsheet)
        if "load_data" in methods_list.get("load_data"):
            current_spreadsheet.load_data()
        if "spreadsheet_to_array" in methods_list:
            current_spreadsheet.spreadsheet_to_array()
        if "headers_array" in methods_list:
            current_spreadsheet.headers_array
        if "headers_used_indexes" in methods_list:
            current_spreadsheet.get_used_indexes(0)
        if "remove_duplicity_by_timestamp" in methods_list:
            current_spreadsheet.remove_duplicity_by_timestamp()
        if "array_to_dictionary" in methods_list:
            current_spreadsheet.array_to_dictionary()

There are methods commonly to all of instances, some of them are not. Because of that, I thought about a way to evaluate if each instance has its respective method, but I don't know if this is the most performative way. Any ideas?

Python version 2.6.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ It'd be nice if you provided more code and context, you mention performance as a concern, yet you've only supplied static code. \$\endgroup\$
    – Peilonrayz
    Jan 17 '17 at 9:00
  • \$\begingroup\$ Welcome to Code Review! Please explain what this code actually accomplishes, and retitle the question accordingly, as stated in the How to Ask guidelines. It looks like you are executing some code needlessly for its side-effect and then discarding the result — like current_spreadsheet.headers_array. But we can't tell what is really going on without being able to see the code behind it, or the code that follows this excerpt. Please provide more context for this question. \$\endgroup\$ Jan 17 '17 at 15:38
1
\$\begingroup\$

You've got a random string inside your function, it is unhelpful, you should remove that. If, however, you meant to make a comment, you should have used the comment sign (#).

For the existence of methods, combining dir and if .. in is inneficient as lists lookups are \$\mathcal{O}(n)\$ but attribute access is usually done through an instance's __dict__. You have two possibilities: either you execute it nonetheless and handle the AttributeError if it didn't exist, or use the getattr function to retrieve the method and then you execute it. The advantages of the latter being that:

  1. You can easily encapsulate that behaviour in a function;
  2. You can use the third parameter of getattr to provide a default value in case the attribute doesn't exist.

Something like this should be very generic, remove functionalities as you don't need them:

def do_nothing(*args, **kwargs):
    pass

def execute_method(name, instance, *args, **kwargs):
    method = getattr(name, instance, do_nothing)
    return method(*args, **kwargs)

Going through the rest of the code, you may also allow to pass the names of the files as parameters.

Proposed improvements look like:

FILES_TYPES = [
    RubricaManager,
    RectifiedRubricaManager,
    RectificationSpreadsheetManager,
    GPSManager,
]
METHODS = [
    'load_data',
    'spreadsheet_to_array',
    'headers_array',
    'get_used_indexes',
    'remove_duplicity_by_timestamp',
    'array_to_dictionary',
]


def do_nothing(*args, **kwargs):
    pass


def execute_method(name, instance, *args, **kwargs):
    method = getattr(name, instance, do_nothing)
    return method(*args, **kwargs)


def main(filenames=('File1.xlsx', 'File2.xlsx', 'File3.xlsx', 'File4.xlsx')):
    for filename, cls in zip(filenames, FILES_TYPES):
        spreadsheet = cls(filename)
        for method_name in METHODS:
            execute_method(method_name, speadsheet)
\$\endgroup\$
7
  • \$\begingroup\$ Why the default parameter filenames, zipping a list of classes, etc? Could just have `FILES = [Class("file"), ...] \$\endgroup\$
    – Caleth
    Jan 17 '17 at 16:26
  • \$\begingroup\$ @Caleth Because creating the objects in advance (with files that may not exist) would defeat the purpose of being able to choose which files will be used. \$\endgroup\$ Jan 17 '17 at 16:33
  • \$\begingroup\$ But you still have a fixed sequence of file types. Either take a sequence of pairs (Class, filename), a sequence objects, or have a static sequence. Having a mix just seems pointlessly brittle \$\endgroup\$
    – Caleth
    Jan 17 '17 at 16:50
  • \$\begingroup\$ @Caleth But we'd like to have a minimum of control about what to do with those files, whereas filenames should be a decision of the caller. This also allows the caller to call the function without impirting our entire API. \$\endgroup\$ Jan 17 '17 at 17:03
  • \$\begingroup\$ I absolutely disagree. main(files = (('File1.xlsx', Class1), ...)): for name, cls in files: keeps the filename and loading method together, and they belong together. OP has them together statically, you have half static and half dynamic. If you change what files there are, you will most likely have to change how to load those files \$\endgroup\$
    – Caleth
    Jan 17 '17 at 17:07

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.