Skip to content

Conversation

SelAnt
Copy link
Contributor

@SelAnt SelAnt commented Jun 1, 2017

Hi,

I added additional encoding for UTF-8 characters which are out of standard ASCII range.

Copy link
Member

@ajantis ajantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just some minor stuff to fix before merging.
Thanks! 👍

@@ -178,7 +177,7 @@ public void save(String dir, String fileName, boolean includeNamespace, String o
*/
public String toString(String tabs)
{
StringBuffer out = new StringBuffer();
StringBuilder out = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a changed indentation (with one space?). Can be reverted.

/**
* Encodes strings as XML. Check for <, >, ', ", &.
// /**
// * Encodes strings as XML. Check for <, >, ', ", &.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about removing the commented out block? :)

Pattern gtPat = Pattern.compile( ">" );
Pattern aposPat = Pattern.compile( "\'" );
Pattern quotPat = Pattern.compile( "\"" );
static String encode(String originalUnprotectedString) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broken indentations in this method make it really hard to follow the flow


String out = new String( in );
StringBuilder stringBuffer = new StringBuilder(originalUnprotectedString.length());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that there are both StringBuffer and StringBuilder classes in java, I wouldn't call a variable of type StringBuilder like that. :) It's really misleading.

anyCharactersProtected = true;
}
}
if (anyCharactersProtected == false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyCharactersProtected is always true, as far as I can see.

I'd just get rid of this if block (and the whole anyCharactersProtected flag altogether) and always return stringBuffer.toString() as this seems like a really minor optimisation to me (if at all).

but if you want to keep it, then, probably, in the default case of the switch it should be:

default:
      stringBuffer.append(ch);
      anyCharactersProtected = false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants