HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

XLSX writer implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
writerxlsximplementation

Problem

We have multiple scripts which write into XLSX with the same format, into a different format or slide variation. I am trying to write an API which saves time for other programs, but I need input on restructuring the file.

```
import openpyxl
from xlsxwriter.workbook import Workbook

class xlsx_extend(object):
"""This class contain api of xlsx functionality"""
def __init__(self, name):
"""create xls file"""
self.xlsx_name = Workbook(name)
self.create_format('blue', 12)

def create_format(self, color, size):
"""write header"""
self.xlsx_format = self.xlsx_name.add_format()
self.xlsx_format.set_font_size(size)
self.xlsx_format.set_bold()
self.xlsx_format.set_color(color)
self.xlsx_format.set_border()
self.xlsx_format.set_center_across()
self.xlsx_format.set_align('center')
self.xlsx_format.set_rotation(90)

def add_sheet(self, ws_name):
"""Adding worksheet name"""
return self.xlsx_name.add_worksheet(ws_name)

def rotate_text_format(self, angle):
"""rotate the text data"""
self.xlsx_format.set_rotation(angle)

def get_xlsx_name(self):
"""Returing xlsx name for derive class"""
return self.xlsx_name

def close_xlsx(self):
"""close xlsx File"""
self.xlsx_name.close()

class xlsx_sheet:
"""This is xlsx worksheet class"""
def __init__(self, excel_sheet, ws_name):
"""Initialze Base function and Create Worksheet"""
self.excel_sheet = excel_sheet
self.ws_name_worksheet = excel_sheet.add_sheet(ws_name)
self.ws_name_worksheet.set_column(0, 0, 40)

def write(self, row, col, data, format=None):
"""write Add in xlsx File"""
self.ws_name_worksheet.write(row, col, data)

def write_column(self, row, col, data, format=None):
"""write data in column than need pass data as list"""
self.ws_name_worksheet.write_column(row, col

Solution

Naming Conventions

According to the official Python style guide, class names should be in PascalCase:

No:
    class my_class():
Yes:
    class MyClass():


In your case, I would capitalize the entire xlsx because it is an acroynm:

class XSLXExtend():


There also seems to be some redundancy in some of your names:

# worksheet_name_worksheet?
self.ws_name_worksheet


This next point may be speaking on an intentional design measure, but I would take a look at what properties are supposed to be 'public' and which are meant to be 'private'. Even though Python doens't truly have private variables, it is still convention to use a single underscore ('_') at the start of any intended private variables.

self._i_should_be_private = 'No touching!'


Some of your names are misleading:

# Wait. This stores the actual Workbook not its name?
self.xlsx_name = Workbook(name)


Structure

I am not a big fan of declaring instance variables outside of the __init__ method. In lieu of this, I would refactor your create_format() method as such:

def create_format(self, color, size):
    format = self.xlsx_name.addFormat()
    format.set_font_size(size)
    format.set_bold()
    format.set_color(color)
    format.set_border()
    format.set_center_across()
    format.set_align('center')
    format.set_rotation(90)

    return format


Then just put this line in the __init__ method: self.format = create_format('blue', 12).

Also, you have an accessor method (which is nice) for xlsx_name. However, in your code that actually uses your classes, you just directly access it:

# Why even have `get_xlsx_name()` if you don't use it?
red_format = report_xlsx.xlsx_name.add_format(...)


The same goes for your create_format() method. You use it once in __init__ but don't when you actually use your class. You skip straight to the Workbook:

red_format = report_xlsx.xlsx_name.add_format({'bg_color': '#FF0000'})


Essentially you are circumventing all the work you did wrapping the Workbook class in another class.

Since your xlsx_extend class is supposed to store data about and manipulate a Workbook why not store all of the formats there? Here is what I would do:

class XLSXExtension(object):
    def __init__(self, name):
        self.xlsx_name = Workbook(name)
        self.formats = {}

        self.create_format('blue', 'blue')

    def create_format(self, name, color, size=12):
        format = self.xlsx_name.addFormat()
        format.set_font_size(size)
        format.set_bold()
        format.set_color(color)
        format.set_border()
        format.set_center_across()
        format.set_align('center')
        format.set_rotation(90)

        self.formats[name] = format

    def __getattr__(self, key):
        try:
            return self.formats[key]
        except KeyError:
            print('Format "{}" not found.'.format(key))

report_xlsx = xlsxutils.xlsx_extend("report.xlsx")
report_xlsx.create_format('red', '#FF0000')
report_xlsx.create_format('green', '#99CC32', 24)

report_worksheet = xlsxutils.xlsx_sheet(report_xlsx, "Report") 
report_worksheet.text_condition_format(1, 1, row, col, 'P', report_xlsx['green'])
report_worksheet.text_condition_format(1, 1, row, col, 'F', report_xlsx['red'])

Code Snippets

No:
    class my_class():
Yes:
    class MyClass():
class XSLXExtend():
# worksheet_name_worksheet?
self.ws_name_worksheet
self._i_should_be_private = 'No touching!'
# Wait. This stores the actual Workbook not its name?
self.xlsx_name = Workbook(name)

Context

StackExchange Code Review Q#39792, answer score: 3

Revisions (0)

No revisions yet.