Overall this looks like a great refactor to me. I'd just like to ensure that all the branch points that were in valueToString are appropriately represented in writeValue since the refactor simplifies the logic there to remove duplicate code. I'm not sure I have time to go through that check myself though.

Thanks for the great contribution.

On an initial inspection, it looks like writeValue is missing:

• The conversion of Map<?> to JSONObject
• The conversion of Collection<?> to JSONArray

valueToString also has an explicit check for Boolean or JSONObject or JSONArray, but the same logic should be covered by the final else clause in writeValue

writeValue does have an explicit check for Boolean, but I'm thinking it could be removed in favor of dropping into the final else.

Does that look right? If I missed any of them feel free to just update the pull request accordingly.

Nevermind about the first part. I mistakenly looked at the diff instead of the full file. The conversion for Map<?> and Collection<?> are there.

I'm still in favor of removing the instanceof Boolean check though. It doesn't seem needed with the fallthrough else.

One last item. I'm not sure I'm a fan of this check in the instanceof JSONString branch:

if (o != null) {
    writer.append(o);
} else {
    quote(value.toString(), writer);
}

If toJSONString() return null, then null is what should get written. We should not override the return value of that method for any reason.

For the instanceof Boolean test, it's writing the value without going through the quote() method. This is so that boolean values are written as bare words, not quoted strings.

Regarding the instanceof JSONString, I was just changing the logic from a ternary operation so that I could call the two parameter form of quote(), passing in the Appendable directly, rather than going through a separate StringBuilder accumulator. You're quite correct that this is a logic change from what was in the valueToString() method, which throws an exception instead.

Good point on the boolean check, I didn't think of that.

I can't decide if the exception being thrown on null is really a good idea either. I guess the original implementation expected "" to be written out for nulls, but then you can never have toJSONString pass back the null keyword. "", "null", and null are all distinct values in JSON and I think that toJSONString should probably support it. Although, I'm not sure if an instance of an object should ever serialize to null... Maybe there is some edge-case where that could/should happen in someone's business logic?

It's a tough one to resolve satisfactorily. The Javadoc of JSONString doesn't say what should happen if null is returned from toJSONString(), so we're in to an area of programming by co-incidence.

If a JSONString object was used in a JSONObject or JSONArray object model, it would appear to work, though maybe unexpectedly, by falling back on its toString() method. If it was used in a JSONWriter or JSONStringer, it would fail with an exception.

Solutions look like:

• conservative behaviour -- always fall back on toString(), and preserve compatibility with any code that depended on this behaviour previously. It's doubtful whether any existing code depended on an exception being thrown here.
• remove bugs that could tickle someone in the future -- always throw an exception on null, forcing a decision on how to update any existing code. Defensible given that exceptions could already occur depending on how you used the API.
• silently insert a JSON null value -- probably what I would do if I was writing a JSON parser from scratch, without an existing code base, but could lead to silent unexpected failures in code that depends on existing behaviour.

Personally, I think the behaviour of (3) is too subtle a change to consider as a change for an existing API. Either (1) or (2) could be justified, but it depends on how conservative you want to be.

@stleary I think @run2000 made a decent list of the differences and problems each solution faces. I like the 2nd and 3rd options the best. I don't think we should ever fall back on toString() as that breaks the point of having the interface.

If we go with option 2, we should update the Javadoc on the JSONString class to indicate that null is not a valid return value.

Thanks for the thorough refactor and discussion, and appreciate that this does not break the unit tests. Regarding behavior changes, will need a little time to look at them. For example, it is possible that the unit test coverage was not adequate, and should have broken on a behavior change. All things being equal, strongly prefer conservative changes that do not affect existing code.

Resetting the clock on this pull request since functional changes were made without explanation or unit test results.

No, it is removing the execute flag.

And the 826ceef commit...

Looking at the combined changes, the permissions are not changed.

OK, I think I understand what is going on. Most of the changes were committed around June 18 2016. At the time, I committed to reviewing the unit tests, since even though they passed, they may have missed something. That was about the time I became heavily committed at work, and never got around to the unit tests. In the meanwhile, 826ceef was committed on July 1 2016, which in addition to the accidental mode change to 100755, appears to include functional changes to JSONArray.join(), JSONObject.valueToString(), JSONObject.writeValue(), and JSONWriter.value(). The mode change was corrected in 2917cc5, also on July 1 2016.

Someone, hopefully @johnjaylward, needs to look at these changes and provide an approval or comments. Someone else (myself) needs to run the unit tests to make sure nothing is broken, and then make sure the code coverage of the changed code is acceptable, and that there are no missing tests that should be added.

So no clock reset, because as it turns out, I never started the merge countdown. Please let me know if I am missing or misunderstanding something here.

I took a quick look at the latest commits, but didn't fully review them to see why the changes were made. I probably won't have time to look at them in depth until Wednesday

Apologies, I was just commiting some outstanding code I'd chaned since sending the merge request, not realising that it would affect the merge.

The chane is adding an overload to writeValue () to take two parameters, since there two callers that took 0, 0 as the third and fourth parameters.

Sorry for the noise.

@run2000 No worries, this is a good exercise to make sure the changes are fully understood and work as intended.

Sorry, I finally got around to reviewing the latest changes and they all look good. The main changes were to remove references to the valueToString method that was refactored to just call writeValue. This should reduce stack overhead with 1 less function call.

The last change of significance is in JSONWriter with the replacement in the value(double) function. This change is also to reduce stack overhead by shortcutting to the actual call further in the stack. No functional change is here.

The last thing to decide on before approval of this PR is how we want to handle the exception case for writing out JSONString implementations that return null. (#241 (comment))

Is there anything additional I need to do to help this along? Or are you comfortable shepparding this along now? I think my comments from Jun 16 still stand regarding the tradeoffs of toString() versus null versus JSONException.

@run2000, there seems to be at least one item left from @johnjaylward's most recent comment, regarding the exception case for JSONString implementations that return null.

Otherwise, I still need to do a final review of the changes, and confirm the unit test coverage is good. If you want, you could look at the unit tests to confirm all new code has coverage and that the unit tests are "reasonable". That is, that they adequately test the functionality, ensure there are no unexpected changes to existing functionality, and there are not any uncaught bugs. If so, please post your results here.

I've been looking at test cases for JSONObject and JSONArray today. My focus so far has been covering off the toString() and write() methods, including the overloads for non-zero indentation. I've also created some tests for the new toMap() and toList() methods.

When I next find a few hours, I'll have a closer look at where JSONString is called, and how the calling code deals with nulls and exceptions.

These are in run2000/JSON-Java-unit-test if you're interested.

Thanks, tests, and progress, looks good.

I've created a new pull request #53 on the JSON-Java-unit-test project, covering the edge cases for JSONString on JSONObject#valueToString() versus JSONObject#write(). This covers the edge case mentioned above, where JSONString returns a null value.

@run2000

• Looks like the merge of reduces the use of unnecessary exceptions #249 has introduced some conflicts. Can you resolve them and resubmit?
• Does valueToString() and write() tests JSON-Java-unit-test#53 include all of the tests you are planning for this pull request?
• Do you feel you have addressed @johnjaylward's concern about the exception case for JSONString implementations that return null?

Ok, will do. Since I'm in that area, I notice JSONString doesn't have the json.org copyright notice. Is this an oversight or intentional?

it would be an oversight. If you could add that too that would be best.

Okay, Javadoc has been expanded on to document the null return scenario.

I am not strongly in favor of this change. However, if the preparation is done to spec, the commit can proceed.

• Need a clear justification for why the new code is an improvement over the old
• Format changes should not be included in the commit, they just make it harder to compare versions
• Any time the functionality is changed, e.g. JSONArray line 1447 removing synchronized, a comment should be added explaining why this is was necessary or an improvement.

What problem does this code solve?

• Changes methods that take a java.io.Writer to use a
  java.lang.Appendable instead. Situations where Appendable
  can be used where Writer cannot include the StringBuffer,
  StringBuilder, and CharBuffer classes.
• JSONObject.valueToString() simplified to delegate to
  JSONObject.writeValue(). This causes a small change in
  behaviour where the value implements JSONString and the
  toJSONString() method returns null. Now the behaviour is
  identical to the writeValue() behaviour.
• Some work has also been done to reduce buffer copies, where
  intermediate StringBuilder objects are used to create String
  objects that get written to a Writer. Write directly to the Writer
  instead.

Changes to the API?
Some method parameter types have changed from Writer to Appendable in JSONObject, JSONArray, and JSONWriter.

Will this require a new release?
Can be rolled into the next release.

Should the documentation be updated?
May want to document the JSONObject.valueToString() behaviour change noted above.

Change to unit tests?
New unit tests are added, calling out the valueToString()
writeValue() methods and the small differences. See
stleary/JSON-Java-unit-test#53

The change from Writer to Appendable looks OK. Can't accept the change to JSONObject.valueToString() at the present time. Behavior changes have a high bar for acceptance, which this does not meet. Or the changes to buffer copies, since there is no compelling case for changing the code. If still interested, please resubmit just the Appendable changes. Also, going forward, recommend that code comments actually be in the code, not the pull request.

Also, going forward, recommend that code comments actually be in the code, not the pull request.

+1 for this