I wrote some code to create marketshare reports. However, I'm sure it can be improved. The tasks performed are:

  1. Open the global data file, refresh the data, import its content as a DF.
  2. Create a backup copy of the old report file.
  3. Read the list of Distribution companies who should receive the report with their data (plus anonymize data of others).
  4. Perform data preprocessing tasks.
  5. Swap the Data sheet in the report file, change its name accordingly (name of the report, name of the Distribution company, month number).

I'm not expecting anyone to get too much into "how it works" (tried to explain it as much as possible in the comments), but is there anything you see that could be improved in this code from the technical point of view?

Much appreciated!

# Import the necessary libraries
import pandas as pd
import numpy as np
import unidecode as ud
import string
import re
from openpyxl import load_workbook
import os
import win32com.client as win32
import datetime
from shutil import copyfile, move

# Define the methods needed

# This one is for refreshing the Excel files without having to do it manually
def refresh(directory, file_name): 
    xlapp = win32.DispatchEx('Excel.Application')
    xlapp.DisplayAlerts = False
    xlapp.Visible = True
    xlbook = xlapp.Workbooks.Open(directory + '\\' + file_name)

# This one is to clean up Distribution company names
def remove_stopwords(text):
    stopword_list = ['SP', 'S', 'SPZOO', 'ZOO', 'OO', 'POLSKA', 'SPZ', 'Z', 'A', 'AKCYJNA', 'SPOLKA', 'KOMANDYTOWA', 'SPK', 'SK', 'K', 'O', 'SA', 'SJ', 'SPJ', 'J', 'JAWNA']
    text_nopunct = ''.join([char.upper() for char in text if char not in string.punctuation])
    text_unidecoded = ud.unidecode(text_nopunct)
    tokens = re.split('\W+', text_unidecoded)
    tokens_no_stopwords = [word for word in tokens if word not in stopword_list]
    formatted_text = ' '.join(tokens_no_stopwords)
    return formatted_text

# This one is to swap sheets in the Excel file and rename it
def excel_rewriter(data_source, df_name, target_file):
    book = load_workbook(data_source)
    writer = pd.ExcelWriter(data_source, engine='openpyxl')
    writer.book = book
    writer.sheets = dict((ws.title, ws) for ws in book.worksheets)
    df_name.to_excel(writer, 'Data', index=False)
    os.rename(data_source, target_file)

# Define timeframes
month_ago = str(int(datetime.datetime.today().strftime('%Y%m'))-1)
two_months_ago = str(int(datetime.datetime.today().strftime('%Y%m'))-2)

# Data location and file name
data_folder = os.getcwd()
backup_folder = data_folder + '\\backup'
chdna_data_file = 'ChannelDnAReport.xlsx'

# Open list of Distribution companies from the text file
distis = []
with open('distis.txt', 'r') as filehandle:
    for line in filehandle:
        currentDisti = line[:-1]

# List names for old and new files
old_list = []
new_list = []
for disti in distis:
    old = disti + '_marketshare_' + two_months_ago + '.xlsx'
    new = disti + '_marketshare_' + month_ago + '.xlsx'

# Create backup copies and place them in the backup folder
for old in old_list:
    copyfile(old, old + '_copy.xlsx')
    move(old + '_copy.xlsx', backup_folder)
    os.rename(backup_folder + '\\' + old + '_copy.xlsx', backup_folder + '\\' + old)

# Refresh the main data file
refresh(data_folder, chdna_data_file)

# Read it as a dataframe
df_chdna = pd.read_excel(chdna_data_file, sheet_name='Data')

# Standarize Disti names in the DF column (they are a mess otherwise..)
df_chdna['Reporter HQ Name'] = df_chdna['Reporter HQ Name'].apply(lambda x: remove_stopwords(x))

# Create DF Total with anonymized data
df_total = df_chdna.copy()
df_total.loc[:, ]['Reporter HQ Name'] = 'TOTAL'

# Create DFs for each Disti
df_list = []
distis_chdna = []
for disti in distis:
    df_list.append('df_' + disti)
    distis_chdna.append(disti.upper().replace('_', ' '))

for df, disti_chdna in zip(df_list, distis_chdna):
    vars()[df] = pd.concat([df_chdna[df_chdna['Reporter HQ Name']==disti_chdna], df_total])

# Swap the Data sheet and refresh Pivots in Excel report files
for old, df, new in zip(old_list, df_list, new_list):
    excel_rewriter(old, vars()[df], new)
    refresh(data_folder, new)

1 Answer 1


Path management


directory + '\\' + file_name

would be better represented by a Path:

from pathlib import Path
# ...

def refresh(path: Path):
    # ...
    xlbook = xlapp.Workbooks.Open(str(path))

# ...

# Refresh the main data file
refresh(Path(data_folder) / chdna_data_file)

Also, this:

# Data location and file name
data_folder = os.getcwd()
backup_folder = data_folder + '\\backup'

can be

# Data location and file name
backup_folder = Path.cwd() / 'backup'

and similarly for

os.rename(backup_folder + '\\' + old + '_copy.xlsx', backup_folder + '\\' + old)

Set lookup

stopword_list should be a set, i.e.

stopwords = {'SP', 'S', 'SPZOO', 'ZOO', 'OO', 'POLSKA', 'SPZ', 'Z', 'A', 'AKCYJNA', 'SPOLKA', 'KOMANDYTOWA', 'SPK', 'SK', 'K', 'O', 'SA', 'SJ', 'SPJ', 'J', 'JAWNA'}

This allows for efficient O(1) instead of O(n)-time lookup.

Generator materialization

Drop the inner brackets here:

text_nopunct = ''.join([char.upper() for char in text if char not in string.punctuation])

so that the generator goes straight to the join without first being saved to an in-memory list.

For the same reason,

tokens_no_stopwords = [word for word in tokens if word not in stopword_list]

should use parentheses instead of brackets.

String interpolation

old = disti + '_marketshare_' + two_months_ago + '.xlsx'

can be

old = f'{disti}_marketshare_{two_months_ago}.xlsx'

Global code

Pull everything after

# Define timeframes

into one or more functions.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.