1
\$\begingroup\$

I am writing a generic text parser and have come up with the following text parsing algorithm, which works fine. I'm wondering how to refactor the following method to make it easily understandable and maintainable for the future. It has chains of conditions and nested if-else blocks to compare with each text line to find matching criteria. Any ideas or comments would be highly appreciated ...

private void seekContents (String currentLine, String fontCurrentLine, String pdfContentPageNumber, String currentSearchToken) throws Exception

{
boolean isTokensToSkip=regCompare.isTokensToSkip(currentLine);
boolean isLinesToSkip=regCompare.isLinesToAvoid(currentLine);
boolean isTokensToStop=regCompare.isTokensToStop(currentLine);
boolean isKeywordToSeekAfterToken= regCompare.isKeywordPatternsToSeekAfterToken(currentLine);

boolean isKeywordPatternToSeekAfterTokenExists=regCompare.isKeywordPatternToSeekAfterTokenExists();

boolean hasFont=regCompare.hasLineFont(fontCurrentLine);
boolean isFileHeader=regCompare.isFileHeader(currentLine);
boolean isFileFooter=regCompare.isFileFooter(currentLine);

String pageNumber=regCompare.hasPageNumber(currentLine);
String hasContentPatternToSeekAfterToken = regCompare.hasContentPatternsToSeekAfterToken(currentLine);
String hasRegexTokensToSkip=regCompare.hasRegexTokensToSkip(currentLine);
String hasRegexTokenToStop=regCompare.hasRegexTokensToStop(currentLine);
String hasRegexTokenToAvoid = regCompare.hasRegexTokensToAvoid(currentLine);



if(isFileHeader||isFileFooter||isLinesToSkip) 
{
    //System.out.println("Skipped the header/footer/lines to skip!");
}
else if(hasRegexTokenToAvoid!=null)
{
    //System.out.println("Skipped the Regex line!");
}

else if(pageNumber!=null)
{
    //System.out.println("Found the page#");
}
else if(isTokensToSkip)
{
    isSearchTokenFound=false;
}
else if(hasRegexTokensToSkip!=null)
{
    //System.out.println("Did i found regex stopper?");
    isSearchTokenFound=false;
}
else if(isTokensToStop)
{
    isSearchTokenFound=false;
    isParserEngineStop=true;
}
else if(hasRegexTokenToStop!=null)
{
    isSearchTokenFound=false;
    isParserEngineStop=true;
}
else
{
    //check for content pattern match

    if(hasContentPatternToSeekAfterToken!=null)
    {
        isContentPatternFound=true;

        if(isKeywordPatternToSeekAfterTokenExists)
        {
            contentPattern=hasContentPatternToSeekAfterToken;
            isKeywordAfterPatternFound=false;
        }
        else
        {
            keepRecordsNoKeywordPatternToSeekAfterTokenExists(hasContentPatternToSeekAfterToken,pdfContentPageNumber,currentSearchToken,currentLine);
        }
    }
    else
    {
        if(isContentPatternFound)
        {
            if(isKeywordPatternToSeekAfterTokenExists)
            {
                if(isKeywordToSeekAfterToken)
                {
                    keepRecordsKeywordToSeekAfterToken(contentPattern,pdfContentPageNumber,currentSearchToken,currentLine);
                    isKeywordAfterPatternFound=true;
                }
                else
                {
                    if(isKeywordAfterPatternFound)
                    {
                        desiredContents.add(currentLine);
                    }
                }
            }
            else
            {
                desiredContents.add(currentLine);
            }
        }
    }
}

}

\$\endgroup\$
1
  • \$\begingroup\$ This is an interesting mess you got here! Could you add some example of what you are parsing? \$\endgroup\$ – Simon Forsberg Dec 4 '14 at 21:39
2
\$\begingroup\$

As you probably suspected yourself, this looks neither readable nor maintainable.

The method makes too many references to outside: a dozen or so references in regCompare.*, and a handful variables defined outside (isSearchTokenFound, isParserEngineStop, ...), and a handful of other methods (keepRecordsNoKeywordPatternToSeekAfterTokenExists, keepRecordsKeywordToSeekAfterToken).

All this indicates that there is no good information hiding in your design. Too many dependencies on external factors makes an implementation too complex and incomprehensible. I'm afraid what you have here is effectively spaghetti code.

Fixing this is difficult. It's not unusual in software development that an approach turns into a complete mess and then you need to restart from scratch in a different direction. It can be very difficult to write a good readable parser.

Here are a few ideas to try to improve this code:

  1. Eliminate anything unused: the hasFont variable is unused, and therefore the fontCurrentLine parameter too. Remove them.

  2. Avoid unnecessary calls: if isFileHeader is true, then isFileFooter and isLinesToSkip won't have to be executed. It seems as if you're trying to declare all variables at the top for some reason. In general that's not a good practice. Try to declare and initialize variables right before they are really needed, no need to do far in advance.

  3. Improve the names: hasRegexTokensToSkip sounds like a boolean, but it's actually a String

  4. Early returns might help. The first couple of else-ifs look like guard statements. Sometimes, turning these into if + return might make the code somewhat easier to read by reducing the indent level of the last else block.

After you apply these simple transformations the code might look slightly better. But in the end, you might be better starting over from scratch.

\$\endgroup\$
0

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.