merging equal code (exercise in refactoring)

[Stefan Ram]

Supersedes: <[email protected]>

I have made up a little exercise.

I have a straightforward solution that I will post
later, but not before 2008-01-19T20:00:00+01:00
(i.e., not before in about 24 hours).

There is a class »Util« implementing a verb »format«
that can format a sequence of texts in a certain way.

The requirements are that a verb »format« of this
class can be used like

Util.format( sequence )

to get the formatted text for the sequence »sequence«,
where the sequence »sequence« is either

- an array of texts or
- a list of texts,

where a »list« is a »java.util.List« object and a
text is a java.lang.CharSequence object.

The implementation of »format« contains two methods,
one for arrays and one for lists.

The body of both method declarations is exactly the same, so
the source code of the class »Util« contains much redundancy.

The exercise is to reduce this redundancy (repetition of code)
in a readable and simple way, but still allow for the same
call pattern »util.format( sequence )« for both arrays and lists.

class Util
{
public static java.lang.String indent( final java.lang.String text )
{ final java.lang.String text1 = text.replaceAll( "\n", "\n " );
return text1; }

public static java.lang.CharSequence format
( final java.lang.CharSequence[] source )
{ final java.lang.StringBuilder result;
final java.lang.StringBuilder text;
boolean first;
result = new java.lang.StringBuilder();
result.append( "Listing:\n" );
text = new java.lang.StringBuilder();
text.append( "[ " );
first = true;
for( final java.lang.CharSequence component : source )
{ if( first )first = false;
else text.append( "\n" );
text.append( component ); }
final char last = text.charAt( text.length() - 1 );
boolean space = true;
switch( last ){ case ' ': case ']': space = false; }
text.append( space ? " ]" : "]" );
result.append( indent( text.toString() ));
return result; }

public static java.lang.CharSequence format
( final java.util.List<java.lang.CharSequence> source )
{ final java.lang.StringBuilder result;
final java.lang.StringBuilder text;
boolean first;
result = new java.lang.StringBuilder();
result.append( "Listing:\n" );
text = new java.lang.StringBuilder();
text.append( "[ " );
first = true;
for( final java.lang.CharSequence component : source )
{ if( first )first = false;
else text.append( "\n" );
text.append( component ); }
final char last = text.charAt( text.length() - 1 );
boolean space = true;
switch( last ){ case ' ': case ']': space = false; }
text.append( space ? " ]" : "]" );
result.append( indent( text.toString() ));
return result; }}

 

[Eric Sosman]

Supersedes: <[email protected]>
[...]
The implementation of »format« contains two methods,
one for arrays and one for lists.

The body of both method declarations is exactly the same, so
the source code of the class »Util« contains much redundancy.

The exercise is to reduce this redundancy (repetition of code)
in a readable and simple way, but still allow for the same
call pattern »util.format( sequence )« for both arrays and lists.

class Util
{
[...]
public static java.lang.CharSequence format
( final java.lang.CharSequence[] source )
{ [...] }

public static java.lang.CharSequence format
( final java.util.List<java.lang.CharSequence> source )
{

return format(source.toArray(
new java.lang.CharSequence[source.size()]);
}
}

A dual solution using Arrays.asList() is also possible.
(I've wondered, albeit only idly, why arrays don't implement
Iterable -- that'd make these two methods just one, instead
of one plus a wrapper. There's probably a reason that'll be
"obvious to even the feeblest intellect" once explained ...)

 

[Stefan Ram]

return format(source.toArray(
new java.lang.CharSequence[source.size()]);
A dual solution using Arrays.asList() is also possible.

This answer is better than my own, because it requires
the least amount of work of the programmer.

When a list is copied to an array or an array is copied
to a list, this needs O(n) operations. Therefore, I have
hesitated to use this myself. But a programmer's time
usually is more expensive than a processor's time.

My own solution tries to avoid an additional copy operation.

The code given was like

public static java.lang.CharSequence format
( PARAMETERS )
{ PREPARATION
for( LOOP_CONTROL )LOOP_STATEMENT
COMPLETION }

The »LOOP_CONTROL« varied with »PARAMETERS«, while
»PREPARATION«, »LOOP_STATEMENT« and »COMPLETION«
did not change.

When something varies with a parameter, the usual
solution is to make this a parameter, too.

But in Java, the LOOP_CONTROL of a for) statement
can not be used as a parameter (it would have to be
something like »quoted code«).

So one has to invert the usual procedure and to make
everything else /except/ the loop control the »parameter«.

This can be done by creating an object for the
PREPARATION, LOOP_STATEMENT and COMPLETION.

Then the two redundant method declarations become:

public static java.lang.CharSequence format
( LIST_PARAMETER )
{ final Delegate delegate = new Delegate();
for( LIST_LOOP_CONTROL )delegate.loopStatement( /* ... */ );
return delegate.completion(); }

public static java.lang.CharSequence format
( ARRAY_PARAMETER )
{ final Delegate delegate = new Delegate();
for( ARRAY_LOOP_CONTROL )delegate.loopStatement( /* ... */ );
return delegate.completion(); }

There still is some redundancy, but most of the formerly
duplicate code now has a single position within the
declaration of the Delegate class.

The actual code is following. As an additional benefit,
the code now is split into more method declarations
than before, which gives more anchor points for documentation.

class Util
{
public static java.lang.String indent( final java.lang.String text )
{ final java.lang.String text1 = text.replaceAll( "\n", "\n " );
return text1; }

public static java.lang.CharSequence format
( final java.lang.CharSequence[] source )
{ final Util util = new Util();
for( final java.lang.CharSequence component : source )
util.process( component );
return util.complete(); }

public static java.lang.CharSequence format
( final java.util.List<java.lang.CharSequence> source )
{ final Util util = new Util();
for( final java.lang.CharSequence component : source )
util.process( component );
return util.complete(); }

private final java.lang.StringBuilder result;
private final java.lang.StringBuilder text;
private boolean first;

public Util()
{ this.result = new java.lang.StringBuilder();
this.result.append( "Listing:\n" );
this.text = new java.lang.StringBuilder();
this.text.append( "[ " );
this.first = true; }

public void process( final java.lang.CharSequence component )
{ if( this.first )this.first = false;
else this.text.append( "\n" );
this.text.append( component ); }

public java.lang.CharSequence complete()
{ final char last = this.text.charAt( this.text.length() - 1 );
boolean space = true;
switch( last ){ case ' ': case ']': space = false; }
this.text.append( space ? " ]" : "]" );
this.result.append( indent( this.text.toString() ));
return result; }}

 

[Lew]

class Util
{
public static java.lang.String indent( final java.lang.String text )
{ final java.lang.String text1 = text.replaceAll( "\n", "\n " );
return text1; }

public static java.lang.CharSequence format
( final java.lang.CharSequence[] source )
{ final Util util = new Util();
for( final java.lang.CharSequence component : source )
util.process( component );
return util.complete(); }
...

<http://java.sun.com/docs/codeconv/html/CodeConventions.doc6.html#430>
in particular,
<http://java.sun.com/docs/codeconv/html/CodeConventions.doc6.html#15395>
and
<http://java.sun.com/docs/codeconv/html/CodeConventions.doc6.html#454>

 

[Piotr Lipski]

return format(source.toArray(
new java.lang.CharSequence[source.size()]);
A dual solution using Arrays.asList() is also possible.

This answer is better than my own, because it requires
the least amount of work of the programmer.

When a list is copied to an array or an array is copied
to a list, this needs O(n) operations. Therefore, I have
hesitated to use this myself. But a programmer's time
usually is more expensive than a processor's time.

My own solution tries to avoid an additional copy operation.

The code given was like

public static java.lang.CharSequence format
( PARAMETERS )
{ PREPARATION
for( LOOP_CONTROL )LOOP_STATEMENT
COMPLETION }

The »LOOP_CONTROL« varied with »PARAMETERS«, while
»PREPARATION«, »LOOP_STATEMENT« and »COMPLETION«
did not change.

When something varies with a parameter, the usual
solution is to make this a parameter, too.

But in Java, the LOOP_CONTROL of a for) statement
can not be used as a parameter (it would have to be
something like »quoted code«).

So one has to invert the usual procedure and to make
everything else /except/ the loop control the »parameter«.

This can be done by creating an object for the
PREPARATION, LOOP_STATEMENT and COMPLETION.

One can also abuse Collection API:

import java.util.*;

class Util {
private static class Formatter extends HashSet<CharSequence> {
StringBuilder result = new StringBuilder();
{
result.append( "Listing:\n" );
result.append( "[ " );
}

public boolean add(CharSequence component) {
result.append(component);
result.append("\n ");
return true;
}

CharSequence toCharSequence() {
if(result.charAt(result.length() - 2) != '[')
result.delete(result.length() - 3, result.length());
result.append(" ]");
return result;
}
}


public static <T extends CharSequence> CharSequence format(final T[]
source ) {
return new Formatter() {{ Collections.addAll(this,
source); }}.toCharSequence();
}

public static <T extends CharSequence> CharSequence format(final List<T>
source) {
return new Formatter() {{ addAll(source); }}.toCharSequence();
}
}

PL

 

[Piotr Kobzda]

return format(source.toArray(
new java.lang.CharSequence[source.size()]);
A dual solution using Arrays.asList() is also possible.

This answer is better than my own, because it requires
the least amount of work of the programmer.

When a list is copied to an array or an array is copied
to a list, this needs O(n) operations. Therefore, I have
hesitated to use this myself.

The asList() is not O(n) -- there is no copy of elements. It just wraps
a given array with a List implementation that access an array elements
directly. So, a "dual solution" mentioned by Eric is better, and should
be preferred here IMHO.


piotr

 

[Stefan Ram]

The asList() is not O(n) -- there is no copy of elements. It just wraps

Thank you. I do not understand all details of this, because
»asList( T... )« is defined as »new ArrayList<T>(a)«, where
»a« is the array, but I am not able to find a corresponding
constructor in »ArrayList«.

I see only »ArrayList(int initialCapacity)«, »ArrayList()«,
and »ArrayList(Collection<? extends E> c)«. None of those
constructors matches an array argument.

 

[Patricia Shanahan]

Thank you. I do not understand all details of this, because
»asList( T... )« is defined as »new ArrayList<T>(a)«, where
»a« is the array, but I am not able to find a corresponding
constructor in »ArrayList«.

I see only »ArrayList(int initialCapacity)«, »ArrayList()«,
and »ArrayList(Collection<? extends E> c)«. None of those
constructors matches an array argument.

The Arrays implementation reuses the identifier "ArrayList". It has a
private static member class ArrayList that is a List view of an array.

Patricia

 

[Hendrik Maryns]

Stefan Ram schreef:

Thank you. I do not understand all details of this, because
»asList( T... )« is defined as »new ArrayList<T>(a)«, where
»a« is the array, but I am not able to find a corresponding
constructor in »ArrayList«.

I see only »ArrayList(int initialCapacity)«, »ArrayList()«,
and »ArrayList(Collection<? extends E> c)«. None of those
constructors matches an array argument.

You’re looking in the wrong place! The ArrayList created in
Arrays.asList is an Arrays.ArrayList, declared as follows:

private static class ArrayList<E> extends AbstractList<E>

You’d have found that by Ctrl-clicking on the constructor in Eclipse…

H.
--
Hendrik Maryns
http://tcl.sfs.uni-tuebingen.de/~hendrik/
==================
http://aouw.org
Ask smart questions, get good answers:
http://www.catb.org/~esr/faqs/smart-questions.html


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.4-svn0 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFHo0O9e+7xMGD3itQRAjySAJ0Qgj7SKNs3Y8GmXwI1iyHzAYgpgwCeJoFn
ZcKDeWVJ7ixb9U7v4wgy5Ig=
=n33x
-----END PGP SIGNATURE-----

 

[Daniel Pitts]

Supersedes: <[email protected]>
[...]
The implementation of »format« contains two methods,
one for arrays and one for lists.

The body of both method declarations is exactly the same, so
the source code of the class »Util« contains much redundancy.

The exercise is to reduce this redundancy (repetition of code)
in a readable and simple way, but still allow for the same
call pattern »util.format( sequence )« for both arrays and lists.

class Util
{
[...]
public static java.lang.CharSequence format
( final java.lang.CharSequence[] source )
{ [...] }

public static java.lang.CharSequence format
( final java.util.List<java.lang.CharSequence> source )
{

return format(source.toArray(
new java.lang.CharSequence[source.size()]);
}
}

A dual solution using Arrays.asList() is also possible.
(I've wondered, albeit only idly, why arrays don't implement
Iterable -- that'd make these two methods just one, instead
of one plus a wrapper. There's probably a reason that'll be
"obvious to even the feeblest intellect" once explained ...)

Arrays aren't objects proper.
There are actually special VM level opcodes for array access.

 

[SlowStrider]

Elegant solution

I admit this is a little late (I found your exercise through google), but I think I have quite an elegant solution that is equivalent:

import static java.util.Arrays.asList;

import java.util.List;

import com.google.common.base.Joiner;

class UtilRefactored {

	public static CharSequence format(List<CharSequence> source) {
		return "Listing:\n" + addBrackets(Joiner.on("\n ").join(source));
	}

	public static CharSequence format(CharSequence... source) {
		return format(asList(source));
	}

	private static String addBrackets(String text) {
		String result = "[ " + text;
		if (!result.endsWith(" ") && (!result.endsWith("]"))) {
			result += " ";
		}
		return result + "]";
	}
}

If you don't wish to depend on Google guava, you can also replace "Joiner.on("\n ").join(source)" by a method call to join("\n ", source) with the following join method

	private static String join(String separator, CharSequence... source) {
		StringBuilder text = new StringBuilder();
		for (final CharSequence component : source) {
			text.append(component);
			text.append(separator);
		}
		if (text.length() == 0) {
			return "";
		}
		// Remove excess last separator
		return text.substring(0, text.length() - separator.length());
	}