PNode.toString refactoring

[GoogleCodeExporter]

With all of its string concatenation and null value checking, 
PNode.paramString is a bug magnet. I'd suggest that anything we can do to 
make it easier for users subclassing PNode, the better.

Attached you will find a patch that changes PNode's toString method to use a 
PParamStringBuilder object instead. It makes writing nicer toStrings trivial, 
and drops cyclomatic complexity of the whole code base to boot.

To retain binary compatability, I've made it use paramString() in case users 
had already implemented classes making use of it, but it has been marked as 
deprecated.

Original issue reported on code.google.com by allain.lalonde on 17 Jul 2009 at 6:18

Attachments:

• PNode.toString-refactoring.diff

[GoogleCodeExporter]

If no one has objections, I'll be folding this in soon. It makes extending 
toString
in a consistent way less error prone.  I realize I'm talking about toString() 
being
hard, and it's usually dead simple, but because of all the null checks the 
cyclomatic
complexity in most of the toString methods in the framework is quite high.

Original comment by allain.lalonde on 21 Jul 2009 at 3:15

• Changed state: Accepted

[GoogleCodeExporter]

My opinion would be to @deprecate all the paramString methods and remove them 
in the
next released version.

I would rather not add the new class PParamStringBuilder to core.  It might fit 
in
extras/.../piccolox/util, or we might point to external code like

http://commons.apache.org/lang/api/org/apache/commons/lang/builder/ToStringBuild
er.html
http://commons.apache.org/lang/api/org/apache/commons/lang/builder/ReflectionToS
tringBuilder.html

for those who wish to implement a paramString()-like toString() method in their 
own
subclasses of PNode.

Original comment by heue...@gmail.com on 21 Jul 2009 at 4:39

[GoogleCodeExporter]

Sure. Of course we'd be adding an external dependency for something rather 
trivial.

Original comment by allain.lalonde on 21 Jul 2009 at 4:42

[GoogleCodeExporter]

Sorry, I meant point to as in documentation, not as in adding a dependency.

Original comment by heue...@gmail.com on 21 Jul 2009 at 4:48

[GoogleCodeExporter]

I agree to Michael and suggest, as a quick solution, do as the documentation
explicitly allows:

return "";

Original comment by mr0...@mro.name on 21 Jul 2009 at 10:26

[GoogleCodeExporter]

Very strange, majority rules I guess. PParamStringBuilder is basically the same 
thing
as the ToStringBuilder class, and the change is binary compatible to the 
current code.

Oh, well.

Should this be marked as "Will Not Fix" then?

Original comment by allain.lalonde on 21 Jul 2009 at 11:08

[GoogleCodeExporter]

Hi Michael,
if we want to keep the dubugging output of toString, I'd second Allain in 
adding a
new class to the core. Expanding the interface gives me quite a headache, but 
for the
sake of DRY-ness of the toStrings, I follow Allains argument.

An external dependency is a no-go. Maybe a reflected toString is an option?

What's your concerns about adding the class (which could be deprecated from the
beginning)?

P.S.: Ceterum censeo toString esse delendam - see comment 5 #c5.

Original comment by mr0...@mro.name on 21 Jul 2009 at 7:02

[GoogleCodeExporter]

I agree with Allain in that paramString methods smell bad.  I also see them of
little-to-no utility.  If someone wants to debug, use a debugger.  My full 
suggestion
is to

1.  Remove any implementation of toString(), defaulting to Object.toString()
2.  Mark all paramString() methods as @deprecated
3.  Refactor all paramString() methods to call toString()
4.  Release version 1.3 with them @deprecated and delegating to toString()
5.  Remove all paramString() methods for the next release

PParamStringBuilder can be added to extras now or later independently.

Original comment by heue...@gmail.com on 21 Jul 2009 at 7:14

[GoogleCodeExporter]

http://stackoverflow.com/questions/1161228/tostring-in-java 'nuf said, but of 
course 
majority rules, just doesn't seem to be the majority of java users. :)

Oh well.  I'll let one of you two make these changes since I think they're 
terrible.

Anyway, there are bigger fish to fry. Onto bigger and brighter bugs.

Original comment by allain.lalonde on 21 Jul 2009 at 10:23

[GoogleCodeExporter]

I'll do the job on Saturday, expect some commit activity between 12:00-14:00 
UTC.

Original comment by mr0...@mro.name on 23 Jul 2009 at 6:05

[GoogleCodeExporter]

Do what exactly?  Have we reached consensus on this yet?

Original comment by heue...@gmail.com on 23 Jul 2009 at 6:59

[GoogleCodeExporter]

I'll just remove the toString implementation entirely, so they fall back to 
Object.

Consensus not yet, Allain has his pain with removing a functioning method 
that's a
pleasure to the stackoverflowers but I think we two are pretty much aligned.

Original comment by mr0...@mro.name on 23 Jul 2009 at 7:30

[GoogleCodeExporter]

please verify and vote r563

Original comment by mr0...@mro.name on 25 Jul 2009 at 1:39

• Changed state: Fixed

[GoogleCodeExporter]

needs more work, see discussion at r563.

Original comment by mr0...@mro.name on 28 Jul 2009 at 9:48

• Changed state: Started

[GoogleCodeExporter]

undone in r598. I'll be back.

Original comment by mr0...@mro.name on 28 Jul 2009 at 7:17

[GoogleCodeExporter]

again and more profound: r602. Please Verify and vote.

Original comment by mr0...@mro.name on 28 Jul 2009 at 8:59

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 30 Oct 2009 at 3:06

• Changed state: Verified