[jira] Updated: (TIKA-32) XMLParser has a CDATA if clause that will never be called; also needs refactoring.

["Keith R. Bennett (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/TIKA-32?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Keith R. Bennett updated TIKA-32:
---------------------------------

    Attachment: tika-32.patch

The following issues were addressed in this patch:

* The CDATA if clauses were removed.

* Java 5 loop syntax was introduced for greater simplicity.

* StringBuffer was replaced by StringBuilder (it's more efficient, and we know it's executing on a single thread).

* StringUtils (from Commons Lang project) was introduced for greater null safety and code readability:

StringUtils.isEmpty(String s): s is null or empty
StringUtils.isNotEmpty(String s): ! isEmpty 

StringUtils.isBlank(String): is null, empty, or all whitespace
StringUtils.isNotBlank(String): ! isBlank

* Some unnecessary casts were removed.

* Other minor refactorings.

* I did *not* extract the common code from concatOccurrences() and extractContent() because the code differed slightly (though possibly inadvertently).  One has:

if (obj instanceof Element) {
    Element elem = (Element) obj;
    text = elem.getText().trim();

... and the other has:

if (node instanceof Element) {
    Element elem = (Element) node;
    if (StringUtils.isNotBlank(elem.getText())) {
        values[i] = elem.getText().trim();
    }

If it's ok to treat these the same (i.e. include blank entries in both or none), I'd like to extract the whole set of if clauses into a getNodeStringVal(Object node) method.  Otherwise, if the other changes I made are ok with you, the patch should be ready to apply.

- Keith


> XMLParser has a CDATA if clause that will never be called; also needs refactoring.
> ----------------------------------------------------------------------------------
>
>                 Key: TIKA-32
>                 URL: https://issues.apache.org/jira/browse/TIKA-32
>             Project: Tika
>          Issue Type: Bug
>          Components: general
>    Affects Versions: 0.1-incubator
>            Reporter: Keith R. Bennett
>            Priority: Minor
>             Fix For: 0.1-incubator
>
>         Attachments: tika-32.patch
>
>
> A couple of methods in this class have an if clause for CDATA that are never called because the condition just previous to it is true:
>                } else if (node instanceof Text) {
>                     Text text = (Text) node;
>                     values[i] = text.getText();
>                 } else if (node instanceof CDATA) {
>                     CDATA cdata = (CDATA) node;
>                     values[i] = cdata.getText();
>  
> Since CDATA extends Text, the Text clause will always be executed for a CDATA object, and not the CDATA clause.
> In addition, the extractContent() and concatOccurrence() methods have many lines in common that could be extracted into a single common method.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.