I have a project that reads spreadsheets as a rectangular list of lists (matrix) transforms the matrix and then writes it to another spreadsheet.  I want to be able to read from and write to multiple formats.  Consequentially, the program should double as spreadsheet conversion tool.Here is an example of just copying one sheet to another:def ConvertSheet(in_sheet, out_sheet)    sheet_io = SheetIO()    data_matrix = sheet_io.read_sheet(in_sheet)    # different functions would operate on data_matrix before writing.    sheet_io.write_sheet(out_sheet, modified_data)Depending on what type of object in_sheet and out_sheet are determine how SheetIO writes it but as far as the client cares, that's SheetIO's problem.The design I have implemented is an odd factory pattern.  It's primarily why I am asking for advice.Abstract base classThis is pretty basic and would essentially be the interface in a proper factory pattern:class ISheetIO(object):     Abstract base class...        def read_sheet(self):        raise NotImplementedError(read_sheet not implemented)    def write_sheet(self):        raise NotImplementedError(write_sheet not implemented)Concrete ClassThe only concrete base class I have implemented so far.  Note that it reads and writes sub-vectors as rows.  I am specifying that all Concrete classes operate that way.Also note that I am allowing optional arguments, specifying the range to write them to.class ExcelSheet(ISheetIO):    def read_sheet(self, sheet, col_start=0, row_start=0, col_cnt=-1, row_cnt=-1):        # TODO: Raise an error instead. Research which error to raise        assert not sheet.ragged_rows        if row_cnt < 0:            row_cnt += sheet.nrows + 1        if col_cnt < 0:            col_cnt += sheet.ncols + 1        return [sheet.row_values(r, col_start, row_start)                 for r in xrange(row_start, row_cnt + 1)]    def write_sheet(self, data, sheet, col_start=0, row_start=0):        for r, row, in enumerate(data):            for c, el in enumerate(row):                sheet.write(row_start + r, col_start + c, el)# class CSVSheet(ISheetIO):# class OpenXMLSheet(ISheetIO):       Wrapper ClassThis is where things get hairy.  My understanding of the factory pattern is that this should be the factory at it would return a concrete class.  Instead the client instructs SheetIO to read or write and SheetIO gets and uses the concrete class itself through its own factory. The reason for composing the concrete class instead of returning it is, I am permitting the client to request the data with sub-lists as columns.  SheetIO encapsulates that state with read_by_row and remembers to transpose the data again when writing.class SheetIO(ISheetIO):    def __init__(self, read_by_row=True):        self.by_row = read_by_row    def _factory(self, sheet):        sheet_type = type(sheet)        if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:            return ExcelSheet()        # elif CSV        #    return CSVSheet()        # elif OpenXML        #    return OpenXMLSheet()        else:            raise TypeError(Unknown Sheet type or not a sheet)    def read_sheet(self, sheet, **kwargs):        data = self._factory(sheet).read_sheet(sheet, **kwargs)        return self.orient_data(data)    def write_sheet(self, data, sheet, **kwargs):        oriented_data = self._orient_data(data)        self._factory(sheet).write_sheet(sheet, oriented_data, **kwargs)    def _orient_data(self, data):        if self.by_row:            return data        else:            return self._transpose(data)     def _transpose(self, matrix):        return zip(*matrix)
Universal Spreadsheet reader/writer
python;object oriented;excel;factory method
Indeed, this factory pattern is not right. A factory should have the single responsibility of creating instances. You violated that the moment you made it a subclass of ISheetIO, which is an entirely different responsibility.The first step to make this better is to change the factory to be more factory-like:Rename it to SheetIOFactoryExtend object (stop extending ISheetIO)Rename the _factory method to create_sheet method: this will become the official way to create sheetsThe methods that remain, read_sheet, write_sheet, _orient_data, _transpose belong in another class. Let's call it SheetIOHelper.This class should not extend ISheetIO.Why? You would rightfully ask:it has read_sheet and write_sheet methods.It even uses the same kind of parameters (uhm, more or less).But a crucial (and somewhat subtle) difference is that the methods in the helper class and the methods in ExcelSheet have completely different contracts:The methods in ExcelSheet are designed to work with Excel sheetsThe methods in SheetIOHelper are designed to work with whatever kind of sheet you throw at themThat is, the methods in SheetIOHelper are expected to detect appropriate source / target sheet type, the methods in ExcelSheet are just not designed that way, they work with only a specific type of sheet.In strongly typed languages,these two interfaces wouldn't be compatible, the code wouldn't compile.In other words,the helper and the concrete classes don't follow the same interface.You could create a dedicated parent class for SheetIOHelper.That might sound like overkill,since you only plan to have just one implementation,it might be worth it,to make the distinction from ISheetIO loud and clear.Or you could be lazy and just make it extend object.And when you use the helper,you should add helper in the variable name to avoid confusion:def ConvertSheet(in_sheet, out_sheet)    sheet_io_helper = SheetIOHelper()    data_matrix = sheet_io_helper.read_sheet(in_sheet)    # different functions would operate on data_matrix before writing.    sheet_io_helper.write_sheet(out_sheet, modified_data)Actually,since you probably won't ever work with the concrete classes directly,it would be better to rename the interface of the concrete classes instead.So in the end, have this class structure:SheetIOHelper: interface of the helper, that can read / write any types of sheetSheetIO: implementation of the helperConcreteSheetIO: interface of concrete implementations that can only read / write sheets of type TExcelSheetIO: concrete implementation to read / write Excel sheetsCsvSheetIO: concrete implementation to read / write CSV sheetsSheetIOFactory: implementation of the factory, creates ConcreteSheetIO instances and does nothing elseIn the end, the names are quite different from what I used in the beginning.I hope you see the evolution that lead us here, and hopefully it's not too confusing.If this is not clear,let me know, I can restructure your original if it would help.Code reviewIn this code:def read_sheet(self, sheet, col_start=0, row_start=0, col_cnt=-1, row_cnt=-1):    # ...    if row_cnt < 0:        row_cnt += sheet.nrows + 1    if col_cnt < 0:        col_cnt += sheet.ncols + 1When I see row_cnt += sheet.nrows + 1 I have 2 thoughts:What is the value of row_cnt before the += ?Why the +1 in sheet.nrows + 1 ?These are very easy to answer, but I still have to think of them.It would be better if I didn't have to think.It seems to me that this simpler variant would be just as good,and it won't make me think:if row_cnt < 0:    row_cnt = sheet.nrowsif col_cnt < 0:    col_cnt = sheet.ncolsIn this code:if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:    return ExcelSheet()# elif CSV#    return CSVSheet()# elif OpenXML#    return OpenXMLSheet()else:    raise TypeError(Unknown Sheet type or not a sheet)It's a minor thing,but since each elif branch returns,I prefer this writing style:if sheet_type is xlrd.sheet or sheet_type is xlwt.WorkSheet:    return ExcelSheet()# if CSV#    return CSVSheet()# if OpenXML#    return OpenXMLSheet()raise TypeError(Unknown Sheet type or not a sheet)