[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: io-2.4.13 released

From: Andrew Janke
Subject: Re: io-2.4.13 released
Date: Fri, 18 Oct 2019 16:04:53 -0700
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.9.0

On 10/17/19 12:51 PM, Philip Nienhuis wrote:

[...] I do want to adapt the spreadsheet I/O functions code base to a much more easily maintainable status. A benefit is that it'll make it easier to move them to core some day, maybe.

An important part is merging the now largely separate ods????? and xls????? functions sets into just one xls????? function set and morphing the ods????? functions into wrappers for their xls????? counterparts and deprecate them. After all, Matlab's xlsread can perfectly read .ods these days and we do want to be compatible.
This way a lot of duplicate code can be eliminated.

I like this idea of merging them, especially if the file type could be auto-detected. One less thing for the user to worry about.

A next step might be splitting up the OCT interface .xlsx and maybe .ods private functions into smaller dedicated pieces. However, .xlsx and .ods are very complicated file formats so maintaining these functions still won't be easy peasy.

Here's an idea that you may or may not like: how about refactoring the internal implementation into Octave OOP classdef classes? The pattern of having "COM_", "JOD_", "JXL_", "POI_", etc variants of the same set of "__XXX_chk_sprt__", "__XXX_spsh_open" etc functions, which are called inside switch statements, just screams "polymorphism" to me. And then there's the XLS vs XLSX vs ODS axis. Turning those into a family of classes like "ComExcelAdapter", "PoiExcelAdapter", "UnoOdsAdapter" might make organizing the code easier, and less painful to split up into small methods. Then a bunch of OCT interface methods could live inside their class without cluttering up the shared function namespace and making the inst/ directory really big.

OOP could also be a nice way of representing more complicated state like cell formatting controls, style definitions, and so on. And the "file handle" structs returned by xlsopen() and odsopen() could be replaced with better-typed objects. Two "assert (isa (...), ...)" calls would replace this verbose, approximate type checking:

  ## Check if xls struct pointer seems valid
  if (! isstruct (xls))
    error ("File ptr struct expected for arg @ 1\n");
  test1 = ! isfield (xls, "xtype");
  test1 = test1 || ~isfield (xls, "workbook");
  test1 = test1 || isempty (xls.workbook);
  test1 = test1 || isempty (xls.app);
  test1 = test1 || ~ischar (xls.xtype);
  if test1
    error ("Invalid xls file pointer struct\n");

  ## Check worksheet ptr
  if (! (ischar (wsh) || isnumeric (wsh)))
    error ("Integer (index) or text (wsh name) expected for arg # 2\n");
  elseif (isempty (wsh))
    wsh = 1;

And then you could use Octave's handle class's delete() destructor to do garbage collection on resources that need to be cleaned up, like active COM Excel or UNO LibreOffice sessions running in the background, the opened workbook files, and so on.

Stick them in a "forge.io.internal" namespace to indicate that they're for Octave's internal use and end user code should use them directly.

I realize this would be a big change, though.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]