2
\$\begingroup\$

I have the following function:

def save_to_file(data, columns_order, save_filename):
    """ Saves data to file
    :param data: data to be saved
    :param columns_order: the order of columns
                          if needed to rearrange
                          else put ""
    :param save_filename: the filename (csv, xls, xlsx)
    :return: True if done or None if error
    """
    try:
        df = pd.DataFrame(data)
        if columns_order:  # Rearrange columns if needed
            df = df[columns_order]
        if save_filename.endswith(".csv"):  # Save as csv
            df.to_csv(save_filename, index=False)
            return True
        elif any(save_filename.endswith(ext) for ext in [".xls", ".xlsx"]):  # Save as xls or xlsx
            df.to_excel(save_filename)
            return True
        else:
            print "Choose between csv, xls, xlsx."
            return None
    except Exception, e:
        print "Save_to_file error:", e
        return None

I don't know if I should leave it like that or put the functions in a dictionary:

func_dict = {".csv": df.to_csv(save_filename, index=False), ".xlsx:": df.to_excel(save_filename)}
\$\endgroup\$
1
\$\begingroup\$

Yes, you could reduce duplication (and make it easier to add new exporters) if you used a dictionary to map extensions to functions. However, you couldn't do it quite as you suggest; you would store the result of the method call in the dictionary, not the method itself. Here is one suggestion, using os.path.splitext to get the extension defined in save_filename and functools.partial to supply the additional argument required for CSVs.

from functools import partial
import os

EXPORTERS = {'.xls': pd.DataFrame.to_excel,
             '.xlsx': pd.DataFrame.to_excel,
             '.csv': partial(pd.DataFrame.to_csv, index=False)}

def save_to_file(data, save_filename, columns_order=None):
    """ Saves data to file

    :param data: data to be saved
    :param save_filename: the filename
    :param columns_order: the order of columns
                          if needed to rearrange

    :return: True if done or None if error

    """
    try:
        df = pd.DataFrame(data)
        if columns_order is not None:  # Rearrange columns if needed
            df = df[columns_order]
        _, ext = os.path.splitext(save_filename)
        if ext in EXPORTERS:
            EXPORTERS[ext](df, save_filename)
            return True
        else:
            print "Not a supported filetype: {}".format(ext)
    except Exception, e:
        print "Save_to_file error:", e

Note the following:

  • I have stored the methods on the class in the dictionary, allowing it to be defined outside the function, so you need to pass the instance df as well as the first argument save_filename;
  • Rather than force the user to specify "" if the columns_order isn't required, I have given it a default None value; and
  • return None is implicit.

Including the list of supported filetypes in the docstring and output message is duplication of the information now stored in the dictionary's keys - if you add another supported type, you now need to change three parts of your code.

Finally, you should review your error handling. For one thing, except Exception as e: is more modern than except Exception, e: (which is retained on;ly for backwards compatibility, and doesn't work at all in 3.x). Secondly, you should use more specific error handling - what could go wrong, and how should it be dealt with? At the very least, I would differentiate between:

  1. The conversion to DataFrame or column rearrangement not working; and
  2. A failure in file operations.

Thirdly, rather than return either True or None, it would be conventional to raise an error from your function in the cases where you currently print a message - if the data hasn't been saved, the calling function should do something about it - and return None (again, implicitly) otherwise.

\$\endgroup\$
  • 1
    \$\begingroup\$ We should use except Exception as e: instead of except Exception, e:. The latter is present only for backwards compatibility in Python 2. \$\endgroup\$ – Ashwini Chaudhary Jan 27 '15 at 15:05
  • \$\begingroup\$ @AshwiniChaudhary good point, updated \$\endgroup\$ – jonrsharpe Jan 27 '15 at 15:16
  • \$\begingroup\$ You would use multiple excepts statements for the errors ? \$\endgroup\$ – GiannisIordanou Jan 27 '15 at 18:37
  • \$\begingroup\$ @evil_inside and more, shorter try blocks, yes. \$\endgroup\$ – jonrsharpe Jan 27 '15 at 20:29

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.