Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#495 Dynamic IO Settings

closed
master (162)
5
2012-10-08
2012-03-20
John May
No

I was using IOSetting's class for the recent patch to IteratingMDLReader when I spotted lots of redundant code in the base reader/writer classes. The settings implementation was also quite constrained requiring access to a fixed size array via 'getIOSettings()'. I thought it would be nice to try and simplify the API by removing redundant code (mainly listener handling) and also added some management of IOSettings. The main bonus is that you can now inherit settings and from super class readers/writer and also add/get specific settings easier (for example force read of 3D coordinates pops up more then once). The existing usage is still valid and so all test cases still pass. It turned out to be a little bit of a bigger patch then I would like and in fact there's a little bit more work to convert classes over but i'd thought I should post this first.

Summary of changes:

New Classes

ISetting: interface for which IOSetting now implements (IOSetting is in package 'io' and we needed a way of using it in 'interfaces')

SettingManager: simple wrapper around a map which handles adding/getting ISetting objects

ChemObjectIO: abstract base class for all reader's and writers. Looking through the reader and writer code there was a lot of repeat code for adding and remove listeners. There was actually   already a IChemObjectIO interface which provided the listener management but there was no implementation. Centralising the listener management to a single class means we only need to maintain it in one place, I also needed a place to add the setting manager to ChemObjectIO was created.

New Tests
new test in IChemObjectTest for adding dynamic settings (check for non-null entries)

Modified Classes

Major Changes
Removed listener management code from DefaultChemObjectReader, DefaultChemObjectWriter, IteratingChemObjectReader and DefaultEventChemObjectReader. These are all now sub-classes of the new abstract ChemObjectIO which is doing the listener and setting management. There were also some minor changes to the methods that access listeners due them now being encapsulated.

IChemObjectIO - Added methods for setting management and provided a 'getListeners()' method to allow subclasses to access the managed
listeners. In addition I deprecated addChemObjectIOListener(...)/removeChemObjectIOListener(...) and added addListener(...)/removeListener(...) which is far less verbose and
simplifies the API.

IteratingMDLReader - now uses the new setting management.

Minor changes
IOSetting - implements ISetting interface
CustomWriter - extends ChemObjectIO

Readers/writers that use IOSettings (Currently all working but I will convert these):
CDKSourceCodeWriter
ChemObjectIO
CMLWriter
Gaussian98Reader
MDLReader
MDLRXNV3000Reader
MDLV2000Reader
MDLV2000Writer
MDLV3000Reader
PDBReader
PDBWriter
SDFWriter
SMILESWriter
GaussianInputWriter

Cheers,
J

Discussion

  • I will review this patch (not meaning that others should not). Looks very interesting!

     
  • John, some initial comments:

    1. great changelog!
    2. thanx for using generics!
    3. please port the patch to master, and file a bug report about removing the deprecated code (once all IO classes are ported to the new API).
    4. for all new files (new interface, etc), please update the following header to start copyright at this year, and add your name. For example, for ISetting.java replace

    /*
    * Copyright (C) 2003-2007 The Chemistry Development Kit (CDK) project

    with

    /*
    * Copyright (C) 2012 John May jwmay@users.sf.net

    1. class JavaDoc should start with a description of functionality. The following information is available anyway, and should not be part of the class JavaDoc (in several classes):

    +/**
    + * ISetting - 20.03.2012

    + *

    (The data you can add as @cdk.created 2012-03-20 ...)

    1. please check missing periods at the end of JavaDoc sentences. Particular the period after the first sentence is important, as the JavaDoc utility uses that to determine the first sentence, which is used as method summary in the TOC of methods at the start of a JavaDoc HTML page.

    2. (optional, or for future patch) Methods like the following should be replaced or deprecated:

    @@ -328,7 +323,11 @@ implements IChemObjectIOListener {
    * @param skip ignore error molecules continue reading
    */
    public void setSkip(boolean skip){
    - this.skip = skip;
    + try {
    + getSetting("skip").setSetting(Boolean.toString(skip));
    + } catch (CDKException ex){
    + logger.debug("Unable toset 'skip' setting: ", ex);
    + }
    }

     private String extractFieldData(String str) throws IOException {
    

    Catching exceptions like this, which you are kind of forced to now, is something we should not be doing...

    1. SettingManager has a main() method. Should that remain there? Or move to a unit test?

    2. The SettingManager has some unfinished JavaDoc, like:

      • @param name
      • @param <S>
    3. *
      • @return

    Otherwise, I like where this is going! Thanx!

     
  • John May
    John May
    2012-03-21

    Okay sounds good, will try and get the changes done today.

    J

     
  • John May
    John May
    2012-03-21

    Quick question on 2.

    So I should still flag as deprecated in this patch, the bug report is to indicate the old methods need changing?

     
  • John May
    John May
    2012-03-21

    Argh! Why does SF have no edit button!

    ...in the previous comment by change I meant: no usage so are in need removal.

     
  • Yes, two options, whatever is quicker for you:

    1. keep marked @deprecated, and file a bug to get them removed, ro
    2. just remove them (which is fine in master)
     
  • John May
    John May
    2012-03-23

    Dynamic Settings Core Changes

     
  • John May
    John May
    2012-03-23

    Okay have added two patch files. One is the core changes (as before) but this time patched against master. The other is the changes to the classes that use the IOSettings.

    • A made some minor changes to the core settings management where by if you add a setting which is already added then you get the original instance back. The manager is now also generic allowing it to store IOSetting's correctly. I wanted to avoid having 'getSetting("name...")' in the IO reader/writer code so now you can add and assign at the same time:

    private BooleanIOSetting setting;
    ....
    setting = addSetting(new BooleanIOSetting("....")); // assign to field using the managed variable

    • Settings are now propagated through 'addSettings()' now which means you don't need to have nested listeners in IteratingSDFReader/SDFWriter (but that method would still work).

    • Some unit tests also needed some tweaks as they were depending on a fixed array for 'getIOSettings' of which the order is now not guaranteed:

    • writer.getIOSettings()[0].setSetting("true");

    • writer.getSetting("WriteAsHET").setSetting("true");

    • I need to write some test's for the new methods but this will probably be next week now. It also might be nice to have a controlled vocabulary of IOSettings. I experimented with an enumeration but it might be simpler to use the old school static string method:

    writer.getSetting(PDBWriter.WRITE_AS_HET).setSetting("true"); // thoughts?

    • I didn't change the method/deprecate as I thought as it's working at the moment there's no need to change it.
    • Have correct issues mentioned by Egon (2012-03-21 02:00:02 PDT)
     
  • Thanx for the updates! Looks good. Applied to master.