3
\$\begingroup\$

I am learning more about python using the book "Automate The Boring Stuff". The problem I completed was "Excel To CSV Converter". I want to kick bad habits to the curb, and make sure I'm learning the best python practices to further my development. I would really appreciate feedback on code efficiency, and code neatness. Is my code PEP-8 compliant? Does it implement all the best practices possible? Thank you in advance, all feedback is appreciated and considered!

import openpyxl, csv, os, re, shutil

path = os.getcwd()
new_path = path + '\\' + 'csv_files'

for excel in os.listdir('.'):

    if not excel.endswith('xlsx'):
        continue

    workbook = openpyxl.load_workbook(excel)

    for sheets in workbook.sheetnames:
        wb_name = re.sub('.xlsx', '', excel)
        csv_name = wb_name + '_' + sheets + '.csv'
        csv_file = open(csv_name, 'w', newline='')
        csv_writer = csv.writer(csv_file)
        sheet = workbook.active

        for row_number in range(1, sheet.max_row + 1):
            row_info = []

            for col_number in range(1, sheet.max_column + 1):
                data = sheet.cell(row=row_number, column=col_number).value
                row_info.append(data)

            csv_writer.writerow(row_info)

        csv_file.close()
        shutil.move(os.path.join(path, csv_name), os.path.join(new_path, csv_name))
\$\endgroup\$
3
\$\begingroup\$

There are a few things you could improve. First, the most important if you want to write Python code that scales to more than a script that does just one thing, you should write functions (or classes). They have the nice feature that you can give them a name, which tells you roughly what they do, and you can give them a docstring which can describe what they do in even more detail.


Bug:

While you do iterate over the sheet names, you do not actually change sheets. workbook.active does not automatically switch to another name. Instead, directly iterate over the workbook.


Otherwise here are a few suggestions:

  • Instead of using os.listdir, you can use pathlib.Path, which has a glob method, to get all excel files in the current directory:

    from pathlib import Path
    
    excel_files = Path(".").glob("*.xlsx")
    

    This way you also don't have to deal with different path delimiters in Unix and Windows and manually getting the current directory.

    Python can also work with relative paths, so no need to get the current directory at all.

  • You should use the with keyword to ensure that the files you open are closed, even in the event of an exception.

  • openpyxl sheets have the attributes rows and columns, which lets you easily iterate over them. You should also learn about list comprehensions. In addition, a csv.writer has the writerows method that can take an iterable of rows to write.

  • Using regex just to replace a single string with nothing is overkill. You could just use str.replace. But here building the string constructively is probably easier. Learn about f-strings in Python 3.6+.

  • Instead of moving the file afterwards with shutils, directly save it to the right file.

  • Use a if __ name__ == "__main__": guard to allow importing from this script from another script.

With these changes your code could become:

import csv
import openpyxl
from pathlib import Path

def convert_all_sheets_to_csv(file_name, target_dir="."):
    """Convert all sheets to csv files saved in the folder `target_dir`."""
    workbook = openpyxl.load_workbook(file_name)
    out_file_name_template = Path(target_dir) / Path(file_name).stem
    for sheet in workbook:
        out_file_name = f"{out_file_name_template}_{sheet.title}.csv"
        convert_sheet_to_csv(sheet, out_file_name)

def convert_sheet_to_csv(sheet, file_name):
    """Convert the content of an excel sheet to a csv file."""
    with open(file_name, "w") as f:
        writer = csv.writer(f)
        writer.writerows([cell.value for cell in row] for row in sheet.rows)

if __name__ == "__main__":
    for excel_file in Path(".").glob("*.xlsx"):
        convert_all_sheets_to_csv(excel_file, target_dir="csv_files")
\$\endgroup\$
2
\$\begingroup\$

@Graipher already shows you how your code can look like if you follow PEP8. But since you've explicetely mentioned the Style Guide, let's have a look closer look at some parts.

Imports

Your import look like this at the moment:

import openpyxl, csv, os, re, shutil

Now listen and repeat after PEP8 - Imports:

  • Imports should usually be on separate lines.
  • Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

    Imports should be grouped in the following order:

    1. Standard library imports.
    2. Related third party imports.
    3. Local application/library specific imports.


    You should put a blank line between each group of imports.

Strictly following the Style Guide would lead to something like:

import os
import re
import csv
import shutil

import openpyxl

I think it should be quite obvious to see how this would be applied to @Graipher's answer.

Docstrings

"""docstrings""" have not yet found their way into your code, but again @Graipher presents a good execution of their use in his answer. They are also introduced in the Style Guide in section Documentation Strings, and even more detailed in PEP257. Using the syntax as shown by @Graipher and the Style Guide has the nice benefit that Python's built-in help(...) as well as basically all Python IDEs will be able to find it easily. I highly recommend to get used to them, once you're at a point where you work on a program that spreads out over several files, these nifty little lines of documentation (together with an IDE that is able to read them) will likely help you to maintain your mental health.

Bonus: Automate the Boring Stuff

Maintaining a consistent, PEP8-compliant style can become tedious, especially on a larger scale. Luckily, there are a lot of good tools to automate the boring stuff for you. First, there are tools like pylint, pycodestyle, and flake8 that can perform style checking on your code. But you can go even further! At the moment, these tools leave fixing the "issues" they find in your code to you. Enter tools like black or yapf, just to name two of them. They can auto-format your to follow style guidelines, e.g. PEP8. If you use a feature-rich code editor like Visual Studio Code (or Atom - not 100% sure here), they can even perform the task on-the-fly while you're writing code. All[citation needed] the tools mentioned above are configurable so you don't have to have to figth windmills if they work against a pet peeve of yours. But often the default configuration will work quite reasonable.

\$\endgroup\$
  • \$\begingroup\$ Atom does have (at least one, probably multiple) package for that. \$\endgroup\$ – Graipher Jun 7 at 12:27

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.