Force StringUtil.toString(double) method to use Locale.ENGLISH

[FObermaier]

Currently StringUtil.toString(double) function uses Locale.getDefault() to write a double to a String.
This function is being used from within CoordinateSequences.toString(CoordinateSequence) making the output un[read|use]able in environments that have a comma as a decimal seperator (like e.g. german).

This PR contains

• Fix for StringUtil.toString(double) method to use Locale.ENGLISH
• Add methods to change decimal format pattern
• Add unit test

Signed-off-by: Felix Obermaier felix.obermaier@netcologne.de

[dr-jts]

Seems like a good fix. Does this problem affect WKTWriter as well?

[FObermaier]

No, WKTWriter uses its own DecimalFormat. It is created like this:

jts/modules/core/src/main/java/org/locationtech/jts/io/WKTWriter.java

Lines 152 to 162 in 5ece77b

	private static DecimalFormat createFormatter(PrecisionModel precisionModel) {
	// the default number of decimal places is 16, which is sufficient
	// to accommodate the maximum precision of a double.
	int decimalPlaces = precisionModel.getMaximumSignificantDigits();
	// specify decimal separator explicitly to avoid problems in other locales
	DecimalFormatSymbols symbols = new DecimalFormatSymbols();
	symbols.setDecimalSeparator('.');
	String fmtString = "0" + (decimalPlaces > 0 ? "." : "")
	+ stringOfChar('#', decimalPlaces);
	return new DecimalFormat(fmtString, symbols);
	}

[dr-jts]

Yes, I'm just wondering why the two approaches are so different.

[FObermaier]

The only real difference I see is that the WKTWriter sets the number of decimal places according to a PrecisionModel. StringUtil.toString(double) lacks that, instead it always used to print out at most one decimal place ("0.#"). Now that is changable.

WKTWriter could reuse a static DecimalFormatSymbols instance.
StringUtil could adjust its DecimalFormatSymbols like WKTWriter does

[dsmiley]

I recommend Locale.ROOT. In Lucene/Solr we use https://github.com/policeman-tools/forbidden-apis to catch such issues at compile time in a comprehensive way.

[FObermaier]

Locale.ROOT is fine for me, seems to be what resembles CultureInfo.InvariantInfo in the dotNet world most.

[dr-jts]

I agree 100% that number formating needs to be done more consistently in JTS. I just ran across another place where the formatting is ad hoc and hence wrong for some numbers.

But this should be done in a better place than StringUtil I think. It's probably better off in NumberUtil (and the StringUtil class can delegate to that).

It might make sense for WKTWriter to then use this code as well (which will require a method to set the maximum digits as well).

I'm happy to make a PR for this to replace this one if that's ok with @FObermaier ?

[dr-jts]

Is Locale.ROOT guarenteed to use a period as the decimal separator? Wouldn't it be clearer to specify the decimal separator explicitly?

[dr-jts]

Is it really necessary to be able to set the rounding mode?

[dr-jts]

Hmm.. and apparently DecimalFormat is not thread-safe, which complicates this further. This hasn't been a problem so far to my knowledge, but this kind of issue tends to be subtle.

Perhaps the toString(double) method should be synchronized to avoid potential issues (in spite of the potential performance hit).

[dr-jts]

This is also a bug in the WKTWriter static toXXX methods as well.

[FObermaier]

@dr-jts, I'm fine totally fine with you creating your own PR

[dr-jts]

@dr-jts, I'm fine totally fine with you creating your own PR

Great - working on it now.

[dr-jts]

@FObermaier have a look at #409 as an alternative and extension to this PR

[dr-jts]

Fixed by #409