1

I know almost nothing about threads, synchronization, ...

That being said, I am working on a record handler, that hits Excel sheet to pull/push data. There exist, in my code base, a BaseRecordHandler, which use the entire spreadsheet to read from/write to. The game plan with it is as follows:

  • create singleton instance of the record handler
  • upon creation, we open the file, which does the following:
    • initializes the spreadsheet on the disk if not exists
    • extracts the records from the spreadsheet to a Collection (by default, this is the list usedRecords)
    • in case of any exceptions, we close everything
  • when we handle() a record, we simply write to the usedRecords
  • on close() we do the following:
    • write the newly-added usedRecords to the spreadsheet
    • close everything

Now I need to come up with a way to write record handler that works on just one Sheet, of which I can have multiple types, that work on the same containing spreadsheet...

BaseRecordHandler looks something like this:

package com.xxx.utils

// imports

public abstract class BaseRecordHandler<T> implements AutoCloseable {
    protected final String dataSourceFilename;

    protected final int firstRowNumber = 1;

    protected List<T> usedRecords = [];

    private FileInputStream inputStream;
    protected Workbook excelFile;
    protected XSSFSheet sheet;

    protected BaseRecordHandler() {
        this.init();
    }

    protected BaseRecordHandler(String dataSourceFilename) {
        this.dataSourceFilename = dataSourceFilename;
        this.init();
    }

    protected void init() {
        this.open();
    }

    public void open() {
        try {
            this.initDataSourceFile();

            this.inputStream = new FileInputStream(this.dataSourceFilename);
            this.excelFile = WorkbookFactory.create(this.inputStream);

            this.sheet = this.excelFile.getSheetAt(this.excelFile.getActiveSheetIndex());

            if (this.usedRecords.isEmpty()) {
                for (int rowNum = this.firstRowNumber; rowNum <= this.sheet.getLastRowNum(); rowNum++) {
                    this.extractRecordFromSheet(rowNum);
                }
            }
        } catch (IOException ex) {
            this.close();
        }
    }

    protected void initDataSourceFile() throws IOException {
        File file = new File(this.dataSourceFilename);
        this.excelFile = this.getWorkbook(file);

        if (!file.exists()) {
            file.getParentFile().mkdirs();
        }

        if (this.spreadsheetNeedsSetUp()) {
            FileOutputStream outputStream = new FileOutputStream(file);

            this.setupSpreadsheet({ Sheet sheet ->
                Row firstRow = sheet.createRow(0);

                this.setupFirstRow(firstRow);

                sheet.createFreezePane(0, 1);
            });

            this.excelFile.write(outputStream);
            outputStream.close();
            if (this.inputStream != null) {
                this.inputStream.close();
                this.inputStream = null;
            }
            this.excelFile.close();
        }
    }

    private Workbook getWorkbook(File file) {
        if (file.exists()) {
            this.inputStream = new FileInputStream(file);
            return WorkbookFactory.create(this.inputStream);
        }
        // NOTE: this is here only because the version of POI that Katalon Studio uses by default, doesn't support factory creation of Excel file from scratch...
        return new XSSFWorkbook();
    }

    protected boolean spreadsheetNeedsSetUp() {
        return !(new File(this.dataSourceFilename).exists());
    }

    /**
     * @param Closure onSetupSheet
     */
    protected void setupSpreadsheet(Closure onSetupSheet) {
        this.createSheets()
                .forEach(onSetupSheet);
    }

    protected List<Sheet> createSheets() {
        if (this.excelFile.getNumberOfSheets() == 0)
            this.excelFile.createSheet();
        return [this.excelFile.getSheetAt(0)];
    }

    protected abstract void setupFirstRow(Row firstRow);

    protected abstract void extractRecordFromSheet(int rowNum);

    public void handle(T record) {
        this.usedRecords.push(record);
    }

    protected void writeToFile() {
        if (!new File(this.dataSourceFilename).exists())
            return;

        if (this.excelFile == null)
            this.open();

        this.writeRecordsToFile();

        FileOutputStream outputStream = new FileOutputStream(this.dataSourceFilename);
        try {
            this.excelFile.write(outputStream);
        } finally {
            outputStream.close();
        }
    }

    protected void writeRecordsToFile() {
        for (int j = this.sheet.getLastRowNum(); j < this.usedRecords.size(); j++) {
            Row row = this.sheet.createRow(j + this.firstRowNumber);
            this.fillInRow(row, this.usedRecords[j]);
        }
    }

    @Override
    public void close() throws Exception {
        this.writeToFile();
        this.closeStreams();
    }

    protected void closeStreams() {
        this.inputStream.close();
        this.excelFile.close();

        // resetting state of streams for next use
        this.excelFile = null;
        this.inputStream = null;
    }

    protected abstract void fillInRow(Row row, T record);
}

So far, so good. But what about that BaseSheetHandler ?

It is implemneted as follows:

package com.xxx.utils

// imports

@InheritConstructors
public abstract class BaseSheetHandler<T> extends BaseRecordHandler<T> {
    protected abstract String getSheetName();

    @Override
    protected void initDataSourceFile() throws IOException {
        super.initDataSourceFile();
        new File(this.dataSourceFilename).withInputStream({ is ->
            this.excelFile = WorkbookFactory.create(is);
            this.setupSpreadsheet { Sheet sheet -> excelFile.setActiveSheet(excelFile.getSheetIndex(sheet)) }
            this.excelFile.close();
        })
    }

    @Override
    protected void setupSpreadsheet(Closure onSetupSheet) {
        Sheet sheet = this.excelFile.getSheet(this.getSheetName());
        if (sheet == null)
            sheet = this.excelFile.createSheet(this.getSheetName());

        onSetupSheet(sheet);
    }

    @Override
    protected boolean spreadsheetNeedsSetUp() {
        return super.spreadsheetNeedsSetUp() ||
                (this.excelFile.getSheet(this.getSheetName()) == null);
    }
}

I go to test it, by creating some PracticeURLHandler and OrganizationNameHandler from the BaseSheetHandler that points to the same excel filename and simply implements the abstract methods, both handle some PracticeProfile that has practiceURL, practiceName, and organizationName, test it like so:

PracticeProfile profile1 = new PracticeProfile(someOrganizationName, somePracticeName, somePracticeURL);

PracticeURLHandler.GetInstance().handle(profile1);
OrganizationNameHandler.GetInstance().handle(profile1);

PracticeURLHandler.GetInstance().close();

WebUI.verifyEqual(OrganizationNameHandler.GetInstance().sheet.getSheetName(), "Practice Organization Names")
WebUI.verifyEqual(OrganizationNameHandler.GetInstance().excelFile.getSheet("Practice URLs").getRow(1).getCell(1).getStringCellValue(), 
        "example.com")

OrganizationNameHandler.GetInstance().close();

The verification seems to fail because, by the time the PracticeURLHandler closes, the OrganizationNameHandler doesn't have that updated excelFile, nor any of the streams thereof reflecting what the practice URL handler just did!

I try to address this on the BaseSheetHandler, via this attempt to "sync" the excelFile:

@Override
        // TODO: I don't think this is working
        protected void writeToFile() {
            this.syncExcelFile();
            super.writeToFile();
        }
    
        // TODO: I don't think this is working
        protected void syncExcelFile() {
            this.closeStreams();
            this.open();
        }

I go to re-run the test, from clean slate. The second assert fails this time, too, for the same reason! Indeed, when I go to check the excel spreadsheet manually, I see that the practice URL record got written to the practice organization names spreadsheet, and not the other one. Somehow, the practice URL handler's excel file is out of sync!!

What are the best strategies/design patterns/tools/... to make sure that any modification we do on one instance of the BaseSheetHandler, modify only the up-to-date version of the excelFile (and get updated sheet, inputStream, ...)?

3
  • 2
    It seems the best thing would be to never read multiple copies of the same file in the first place. Files are naturally singular filesystem objects (at least in all of the popular, widely used filesystems anyway); so it's rarely a good idea to read the same file into multiple different instances. At the best of times it's just unnecessary duplication (in the case of a read-only file), and at worst it's a recipe for race conditions such as this when its content is cached, for what seems to be no benefit. Sep 20 at 9:33
  • I see. What is the best way to implement this solution, without changing the use case of PracticeURLHandler/OrganizationNameHandler? Sep 20 at 15:50
  • UPDATE: After some careful thinking, I have decided to ditch my use case of the PracticeURLHandler and OrganizationNameHandler. Those will be contained in a parent class. I will provide detailed answer, if nobody else posts answer to this question, once I have implemented it. Sep 20 at 18:38

1 Answer 1

0

UPDATE: Per Ben Cottrell's comment, the answer here is to NOT have multiple copies of the excelFile spread across objects.

The way I chose to implement that, is to have some parent-child relationship between the spreadsheet handlers and the sheet handlers. This allow me to simplify my BaseRecordHandler by the following:

  • ditch the spreadsheetNeedsSetUp() method, refactoring initDataSourceFile() to the following:
    protected void initDataSourceFile() throws IOException {
        File file = new File(this.dataSourceFilename);

        if (file.exists())
            return;

        file.getParentFile().mkdirs();

        //  NOTE: this is here only because the version of POI that Katalon Studio uses by default, doesn't support factory creation of Excel file from scratch...
        this.excelFile = new XSSFWorkbook();
        FileOutputStream outputStream = new FileOutputStream(file);

        this.setupSpreadsheet({ Sheet sheet ->
            setupFirstRow(sheet.createRow(0));

            sheet.createFreezePane(0, 1);

            // returning the sheet allows for the functional programming technique known as composition
            return sheet;
        });

        this.excelFile.write(outputStream);
        outputStream.close();
        this.excelFile.close();
        this.excelFile = null;
    }

(We separated out the concerns and made things simpler here.)

  • ditch the getWorkbook() method. Because initDataSourceFile() is now, as far as base class is concerned, handling only the creation of the spreadsheet itself, we know what type of spreadsheet (Workbook) we need for it.

The rest of the code base is now as follows:

The parent class PracticeProfileHandler:

package com.xxx.utils

// imports 

public class PracticeProfileHandler extends BaseRecordHandler<PracticeProfile> {

    private static PracticeProfileHandler _instance;

    private PracticeURLHandler childPracticeURLHandler;
    private OrganizationNameHandler childOrgNameHandler;

    public static PracticeProfileHandler GetInstance() {
        if (this._instance == null) {
            this._instance = new PracticeProfileHandler();
        }
        return this._instance;
    }

    protected PracticeProfileHandler() {
        super("./Practice Profiles.xlsx");
    }

    @Override
    public void handle(PracticeProfile record) {
        this.childPracticeURLHandler.handle(record);
        this.childOrgNameHandler.handle(record);
    }

    public PracticeURLHandler getChildPracticeURLHandler() {
        return childPracticeURLHandler;
    }

    public OrganizationNameHandler getChildOrgNameHandler() {
        return childOrgNameHandler;
    }

    public void open() {
        try {
            this.initDataSourceFile();

            if (this.excelFile == null) {
                this.initReadableExcelFile();
            }
        } catch (IOException ex) {
            this.close();
        }
    }

    @Override
    protected void initDataSourceFile() throws IOException {
        if (new File(this.dataSourceFilename).exists()) {
            this.initReadableExcelFile();
            this.setupSpreadsheet(null);
            return;
        }
        super.initDataSourceFile();
    }

    @Override
    protected void writeRecordsToFile() {
        final Closure onCloneIntoWorkbook = SMDSpreadsheetUtils.OnCopySheetIntoSpreadsheet(this.excelFile);

        this.childPracticeURLHandler.writeRecordsToSheet(onCloneIntoWorkbook);
        this.childOrgNameHandler.writeRecordsToSheet(onCloneIntoWorkbook);
    }

    @Override
    protected void setupSpreadsheet(Closure<Sheet> onSetupSheet) {

        Closure onGetAndSetupSheet = { String sheetName -> this.excelFile.getSheet(sheetName) };
        if (onSetupSheet != null)
            onGetAndSetupSheet = SMDSpreadsheetUtils.OnCreateIfNotExistSheet(this.excelFile) >> onSetupSheet;

        this.childPracticeURLHandler = new PracticeURLHandler(onGetAndSetupSheet);
        this.childOrgNameHandler = new OrganizationNameHandler(onGetAndSetupSheet);
    }

    @Override
    protected void setupFirstRow(Row firstRow) {
        throw new Exception("PracticeProfileHandler should not be trying to set up the first row of a Sheet");
    }

    @Override
    protected void extractRecordFromSheet(int rowNum) {
        throw new Exception("PracticeProfileHandler should not be trying to extract records from a Sheet");
    }

    @Override
    protected void fillInRow(Row row, PracticeProfile record) {
        throw new Exception("PracticeProfileHandler should NOT be trying to fill in Rows.");
    }
}

BaseSheetHandler, the base class of the child handlers:

package com.xxx.utils

// imports

public abstract class BaseSheetHandler<T> {
    protected Sheet sheet;
    protected List<T> usedRecords;

    protected BaseSheetHandler(Closure<Sheet> onGetSheet) {
        onGetSheet.setResolveStrategy(Closure.DELEGATE_FIRST);
        onGetSheet.setDelegate(this);
        this.sheet = onGetSheet(this.getSheetName());
        this.init();
    }

    protected void init() {
        this.usedRecords = [];
        SMDSpreadsheetUtils.ExtractDataFromSheet(this.sheet,
                { int rowNum -> this.extractRecordFromSheet(rowNum) });
    }

    public void handle(T record) {
        this.usedRecords.push(record);
    }

    protected abstract String getSheetName();

    protected abstract void setupFirstRow(Row row);

    protected abstract void extractRecordFromSheet(int rowNum);

    protected abstract void fillInRow(Row row, T record);
}

BasePracticeProfileSheetHandler (the direct superclass of PracticeProfileHandler and OrganizationNameHandler):

package com.xxx.utils

// imports 

@InheritConstructors
public abstract class BasePracticeProfileSheetHandler extends BaseSheetHandler<PracticeProfile> {
    protected Map<String, String> dict;

    protected void writeRecordsToSheet(Closure onDone) {
        this.usedRecords
                .forEach({ PracticeProfile profile ->
                    // for some odd reason, in here, this refers to this Closure, nothing else
                    fillInRow(findRowWithPracticeName(profile.getPracticeName()),
                            profile);
                });
        onDone(this.sheet);
    }

    protected Row findRowWithPracticeName(String practiceName) {
        return this.sheet
                .findResult(this.sheet.createRow(this.sheet.getLastRowNum() + 1), { Row row ->
                    // what the fuck, Groovylang?
                    final Cell firstCell = row.getCell(0);

                    if (firstCell == null)
                        return null;

                    if (!firstCell
                    .getStringCellValue()
                    .equals(practiceName))
                        return null;

                    return row;
                });
    }
}

This is a MUCH cleaner, and more correct, approach if I am not mistaken.

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.