merging equal code (exercise in refactoring)

Discussion in 'Java' started by Stefan Ram, Jan 18, 2008.

  1. Stefan Ram

    Stefan Ram Guest

    Supersedes: <-berlin.de>

    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; }}
     
    Stefan Ram, Jan 18, 2008
    #1
    1. Advertising

  2. Stefan Ram

    Eric Sosman Guest

    Stefan Ram wrote:
    > Supersedes: <-berlin.de>
    > [...]
    > 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 ...)

    --
     
    Eric Sosman, Jan 18, 2008
    #2
    1. Advertising

  3. Stefan Ram

    Stefan Ram Guest

    Eric Sosman <> writes:
    >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; }}
     
    Stefan Ram, Jan 20, 2008
    #3
  4. Stefan Ram

    Lew Guest

    Stefan Ram wrote:
    > 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>

    --
    Lew
     
    Lew, Jan 20, 2008
    #4
  5. Stefan Ram

    Piotr Lipski Guest

    "Stefan Ram" <-berlin.de> wrote in message
    news:-berlin.de...
    > Eric Sosman <> writes:
    >>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 Lipski, Jan 22, 2008
    #5
  6. Stefan Ram

    Piotr Kobzda Guest

    Stefan Ram wrote:
    > Eric Sosman <> writes:
    >> 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
     
    Piotr Kobzda, Feb 1, 2008
    #6
  7. Stefan Ram

    Stefan Ram Guest

    Piotr Kobzda <> writes:
    >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.
     
    Stefan Ram, Feb 1, 2008
    #7
  8. Stefan Ram wrote:
    > Piotr Kobzda <> writes:
    >> 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.
    >


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

    Patricia
     
    Patricia Shanahan, Feb 1, 2008
    #8
  9. Stefan Ram schreef:
    > Piotr Kobzda <> writes:
    >> 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.


    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-----
     
    Hendrik Maryns, Feb 1, 2008
    #9
  10. Stefan Ram

    Daniel Pitts Guest

    On Jan 18, 11:41 am, Eric Sosman <> wrote:
    > Stefan Ram wrote:
    > > Supersedes: <-berlin.de>
    > > [...]
    > > 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.
     
    Daniel Pitts, Feb 2, 2008
    #10
  11. Stefan Ram

    SlowStrider

    Joined:
    Apr 10, 2012
    Messages:
    1
    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:

    Code:
    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
    Code:
    	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());
    	}
    
     
    SlowStrider, Apr 10, 2012
    #11
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Dean Ware

    Code Clean up (refactoring?)

    Dean Ware, Mar 3, 2005, in forum: C++
    Replies:
    6
    Views:
    435
    BigBrian
    Mar 3, 2005
  2. shuisheng
    Replies:
    4
    Views:
    348
    Jim Langston
    Mar 18, 2007
  3. Dennis Yurichev

    c code refactoring or optimizing

    Dennis Yurichev, Mar 27, 2007, in forum: C++
    Replies:
    3
    Views:
    349
    Jim Langston
    Mar 28, 2007
  4. Dennis Yurichev

    c code refactoring or optimizing

    Dennis Yurichev, Mar 27, 2007, in forum: C Programming
    Replies:
    6
    Views:
    439
    Ira Baxter
    Mar 28, 2007
  5. Stefan Ram
    Replies:
    0
    Views:
    439
    Stefan Ram
    Jan 18, 2008
Loading...

Share This Page