Unit tests & Singelton

[Thomas Smets]

In need to test the behavior of a String buffer (spelled in 2 words). For our purpose we had to implement it as Singelton. The "buffer" just holds Strings read by a FileReader before they'd processed by multiple "String processors".
My question is how can I test the Singelton close method for instance without poluting the code with what I have currently done (default access flush / reOpen methods).
Tx for any hint(s),
>>>>>>>>>>>>>>>
This is the reply to Frank's response below.
Yes we effectively moved towards a "simple" Factory where I can give a parameter, so the final testing is much simpler. However beside using smthg like :

Class (for which I only want one instance)

ClassManager (which takes care of returning ONE & ONLY ONE instance of the target class)

I don't see how I could easily test my singelton.

I don't see why the class itself is burdened with reading a system parameter and parsing an interger stack size out of it. Put that code somewhere else, and pass the int value in as a constructor parameter. This will let you test the class behaviour for different values of the stack size, which is needlessly hard at the moment.

Because the initial version was reading config of configs of configs ... & I did a quick & dirty hack ! ! !
This can definitively be improved !!! but it takes time & the hack fits the purpose right now

I'm a bit worried by the isClosed stuff. All that seems to happen if the stack is "closed" is that some operations throw a generic IOException. I can't see that this is especially useful behaviour from the perspecive of the client code. If the client code gets one of those exceptions, what should it do then? I assume it is there because client code is "not supposed" to call "put" or "flush" when it's not appropriate. Why not move this open/close/prevent behaviour and its flag outside to a "gatekeeper" or "decorator" class which you can choose to use or not.

I don't agree on all of it !
Out primary goal is to gain speed !
I therefore need something close to C / C++ to have rocketing number first !
Yes in it's current form the last put can be followed by a close
& it's just to prevent any issued that I did throw an Exception.

<<<<<<<<<<<<<<<
\T,

  
/*
 * Created on 14-Aug-2003
 */
package com.tls.util.common;
import java.io.IOException;
import org.apache.log4j.Logger;
import com.tls.util.Helper;
/**
 * 
 * 
 * @author Thomas SMETS
 *
 */
public class StringBufferer
{
    private static final Logger log = Logger.getLogger (StringBufferer.class);
    public static final String ELEMENTS_STACK_SIZE = "ELEMENTS_STACK_SIZE";
    public static final int DEFAULT_STACK_SIZE = 4096;
    public static final int START_IN = -1;
    public static final int START_OUT = -1;
    
    /**
     * The fact that we create the <code>instance</code> reference makes it 
     * Thread-safe inherently.
     */
    private static final StringBufferer instance = new StringBufferer ();
    
    private String [] in = null, 
                     out = null;
    private Object inLock = new Object (),
                    outLock = new Object ();       
                         
    private int stackSize = DEFAULT_STACK_SIZE;    
    private int stackSizeLimit = DEFAULT_STACK_SIZE - 1;
    private int inPos = START_IN, 
                outPos = stackSizeLimit;
                
    private boolean isClosed = false;                
    
    /**
     * Hidden constructor     
     */
    StringBufferer ()
    {
        log.info ("Created");
        String property = System.getProperty(ELEMENTS_STACK_SIZE);
        try
        {            
            stackSize = Integer.parseInt( property ); 
        } catch (NumberFormatException nfe)
        {
            log.warn ("Number format Exception. Property value was : " + property, nfe);
            stackSize = DEFAULT_STACK_SIZE;
        }
        stackSizeLimit = stackSize - 1 ;
        outPos = stackSizeLimit;
    }
    
    /**
     * Singelton thread safe.
     * 
     * @return
     */
    public static StringBufferer getInstance ()
    {
        return instance;
    }
    
    /**
     * 
     * 
     * @param aDomeElementAsString
     */
    public void put (String aDomeElementAsString)
        throws IOException
    {
        if (aDomeElementAsString==null)
            return;
        
        if (isClosed)
        {        
            log.error ("Stack closed...");
            throw new IOException ("Stack closed");            
        }
            
        log.debug ("Putting : " + aDomeElementAsString);
        while (inPos==stackSizeLimit)
        {    
            checkSwapNeeded();
            synchronized (inLock)
            {                                           
                try
                {                
                    log.debug ("Waiting that some free space is made available");
                    inLock.wait ();
                    log.debug ("Woken up ! There might be some free space");                    
                } catch (InterruptedException ie)
                {
                    log.info ( "Thread has been interrupted. Message is " + ie.getMessage(),
                               ie );
                }        
            }
        }     
        if (in == null)
            in = new String[stackSize];            
        
        in[++inPos] = aDomeElementAsString;                                       
        
        checkSwapNeeded ();             
    }
    
    /**
     * 
     * 
     * @return the String
     */
    public String get ()
    {
        log.debug ("Retrieving the next element");
        while ( out==null | outPos == stackSizeLimit)
        {    
            try
            {   
                synchronized (inLock)
                {                                   
                   log.debug( "About to starve... I'll wake up a few waiting on the inLock" );                                        
                   inLock.notifyAll ();                                  
                }            
                synchronized (outLock)
                {                                   
                   log.debug( "Starving" );                                        
                   outLock.wait (100);                                  
                }
                
            } catch (InterruptedException ie)
            {
                log.warn ( "Somebody interrupted during my sleep. Message : " + ie.getMessage(), 
                           ie);
            }
        }
        return (out[++outPos]);                             
    }
        
    /**
     * This method estimates if a <code>swap</code> between the <code>in</code> 
     * & the <code>out</code> may be needed
     * 
     *
     */
    private void checkSwapNeeded ()
    {
        if (inPos==stackSizeLimit & outPos==stackSizeLimit)
            swap ();    
    }
    
    private void swap ()
    {
        synchronized (outLock)
        {
            log.debug ("Swapping");
            out = in;
            inPos = START_IN;                
            outPos = START_OUT; 
            log.debug ("outLock.notifyAll()");
            outLock.notifyAll();            
        }   
        
        synchronized (inLock)
        {            
            log.debug ("inLock.notifyAll()");
            inLock.notifyAll();                                        
        }
    }
    
    /**
     * This is to be invoked whenever the Reader has finished to <code>put ()</code> 
     * element on the stack.
     */
    public void close ()
    {
        log.info ("Closing the DOMHolder");
        isClosed = true;
    }
    
    void reOpen ()
    {
        log.info ("!!! !!! !!! void reOpen () !!! !!! !!!");
        isClosed = false;
    }
    
    /**
     *      
     *
     */
    void flush ()
        throws IOException
    {
        if (isClosed)
        {
            String msg = "Holder was closed";
            log.warn (msg);
            //throw new IOException (msg);    
        }
        in = null;
        out = null;
    }
    
    /**
     * @see Object#toString
     */
    public String toString ()
    {
        StringBuffer buf = new StringBuffer ( 2 * stackSize * 50);
        String[] inCopy = new String[stackSize], 
                 outCopy = new String[stackSize];
        int inSize = 0,
            outSize = 0;
        Helper hlpr = Helper.getInstance();
            
                             
        synchronized (inLock)
        {
            System.arraycopy(in, 0, inCopy, 0, inSize = inPos);
        }
        synchronized (outLock)
        {
            System.arraycopy(out, 0, outCopy, 0, outSize = outPos);     
        }
        
        int i = 1;
        buf.append("in = ")
            .append (hlpr.showArray(inCopy))
            .append (Helper.LINE_SEPARATOR)
            .append ("out = ")
            .append (hlpr.showArray(outCopy))
            .append (Helper.LINE_SEPARATOR);
        
            
        return buf.toString ();
    }
    
    /**
     * @return
     */
    public int getStackSize ()
    {
      return stackSize;
    }
}

[ September 05, 2003: Message edited by: Thomas SMETS ]

[Frank Carver]

For our purpose we had to implement it as Singelton
Before we go too much further, I'd like a bit more explanation of why you feel you had to implement it as a Singleton. Singletons are not universally bad, but they are almost always misused. Just because you want an easy way of locating a shared resource does not mean it has to be a Singleton.
As you have found, testing Singleton objects is a nightmare. By far the simplest way to make testing easier is not to use the Singleton pattern at all. Maybe get your shared instance from a Factory or Builder, maybe store your shared instance in some shared context, maybe pass it it as a method or constructor parameter where needed.
From a purely personal point of view there also seems to be a lot of code in this class. More than I would put in it myself. Hear are some examples of things whivh might make sense to move out, which might also make the class easier to test:

I don't see why the class itself is burdened with reading a system parameter and parsing an interger stack size out of it. Put that code somewhere else, and pass the int value in as a constructor parameter. This will let you test the class behaviour for different values of the stack size, which is needlessly hard at the moment.

I'm a bit worried by the isClosed stuff. All that seems to happen if the stack is "closed" is that some operations throw a generic IOException. I can't see that this is especially useful behaviour from the perspecive of the client code. If the client code gets one of those exceptions, what should it do then? I assume it is there because client code is "not supposed" to call "put" or "flush" when it's not appropriate. Why not move this open/close/prevent behaviour and its flag outside to a "gatekeeper" or "decorator" class which you can choose to use or not.

The "in" and "out" arrays seem very similar. I'd also be tempted to move them and their synchronization locks to separate objects (possibly both the same class). Then you can comprehensively test the bahaviour of each array, independent of whether the main dual-array buffer has been set up.

I'd also delete those empty (or effectively empty) Javadocs. But that's just because I hate comments that take up space but don't pull their weight

Finally, your "flush" method is intriguing. There may be code missing for illustration, but as it stands I'd consider changing its name to "clear" or "reset" or something like that. Its semantics are not really those of common "flush" methods which ensure that any unprocessed data is used or sent. Yours seems to do the opposite and ensures that any unprocessed data is not used, and silently discarded.
Thanks for showing your code, and I hope some of my suggestions might help you with your testing and future bugfixing. Let us know how you get on.

[Ilja Preuss]

Out primary goal is to gain speed !
I therefore need something close to C / C++ to have rocketing number first !

I am not sure I fully understand your problem here. Nevertheless I want to point out one very important fact, just to get that clear:
It typically is far from being the best strategy to disregard good design from the beginning to get better performance. The best way to get to fast code is to get it well designed first. That is because well designed code is much better to profile and optimize than badly designed code - and the bottlenecks aren't where you think they are, anyway.

[Frank Carver]

Thomas SMETS wrote:
>>>>>>>>>>>>>>>
This is the reply to Frank's response below.
This mixing of original post and reply certainly confused me. It's general practice around here to post a reply in sequence with the other people's posts, so readers can see how the discussion has progressed.
However beside using smthg like :
Class (for which I only want one instance)
ClassManager (which takes care of returning ONE & ONLY ONE instance of the target class)
I don't see how I could easily test my singelton.

I'm not sure what you are saying here. let me rephrase a little:
If you made a public constructor for your class which accepts as parameters anything it needs to set it up, why would this be a bad choice? I still can't see any justification for this class being a Singleton.
I don't understand what you gain with the ClassManager example above. Surely, if you need one of these StringBufferer objects in your application, you can just call "new" and create one? Once you have an object you can put it anywhere you like, for any other object to use.
the hack fits the purpose right now
Well, except that you can't unit test your code for other "stack" sizes without restarting the JVM. That sounds like a big drawback to me.
I don't agree on all of it !
Out primary goal is to gain speed !
I therefore need something close to C / C++ to have rocketing number first !
Yes in it's current form the last put can be followed by a close
& it's just to prevent any issued that I did throw an Exception.
Taht's not quite what I was getting at. If you want speed you are always better off with simpler code. Its much easier to measure and improve, and there's less to be loaded and run. It may seem like "common sense" but it's worth reiterating: The only way to make your program run faster, is to make it run less code. All else is dreaming.
My guess is that in most normal cases with well-written client code, you will never actually need the checks for isClosed, and yet you run them every time you do a "put" or a "flush". If you move those checks to another class you may be able to get away without running them at all. Consider something like:

class StringBufferer
{
    ...
    public void put (String aDomeElementAsString)
    {
        log.debug ("Putting : " + aDomeElementAsString);
        while (inPos==stackSizeLimit)
        {    
            checkSwapNeeded();
            synchronized (inLock)
            {                                           
                try
                {                
                    log.debug ("Waiting that some free space is made available");
                    inLock.wait ();
                    log.debug ("Woken up ! There might be some free space");                    
                } catch (InterruptedException ie)
                {
                    log.info ( "Thread has been interrupted. Message is " + ie.getMessage(),
                               ie );
                }        
            }
        }     
        if (in == null)
            in = new String[stackSize];            
        
        in[++inPos] = aDomeElementAsString;                                       
        
        checkSwapNeeded ();             
    }
    ...
    void flush ()
    {
        in = null;
        out = null;
    }
}
class ParanoidStringBufferer extends StringBufferer
{
  private boolean isClosed = false;
  public void close ()
  {
    isClosed = true;
  }
    
  void reOpen ()
  {
    isClosed = false;
  }
  public void put(String dom)
    throws IOException
  {
    if (dom==null) throw new NullPointerException();
    if (isClosed) throw new IOException("buffer closed");
    super.put(dom);
  }
  public void flush ()
    throws IOException
  {
    if (isClosed)
    {
       String msg = "Holder was closed";
       log.warn (msg);
       throw new IOException (msg);    
    }
    super.flush();
  }
}

The above could be even easier and more powerful if you defined the public behaviour of the StringBufferer as an interface first. Then you would be free to code any such "decorator" behaviour you like, and operate transparently on an existing object.
I agree with Ilja on the danger of premature optimisation. Ask yourself this question: "if your code does not work properly, is it any use being fast?"