Re: [cp-patches] Suggesting headers for public methods for javax.swing.Timer

[Michael Koch]

On Thu, Feb 24, 2005 at 10:37:56PM +0100, Meskauskas Audrius wrote:
> This class is undocumented. I suggest some headers at least for for public 
> methods and also tried to apply several Michael's recommendations on 
> formatting. 
> The code itself is not altered.

Some comments from me.

>@@ -34,45 +34,47 @@
> this exception to your version of the library, but you are not
> obligated to do so.  If you do not wish to do so, delete this
> exception statement from your version. */
>-
>-
> package javax.swing;
> 
>-import java.awt.event.ActionEvent;
>-import java.awt.event.ActionListener;
> import java.io.Serializable;
> import java.util.EventListener;
> 
>+import java.awt.event.ActionEvent;
>+import java.awt.event.ActionListener;
> import javax.swing.event.EventListenerList;

We sort them imports alphabeticaly and group them by there first package
part name, e.g. java, javax, gnu, etc.

>-  private static final long serialVersionUID = -1116180831621385484L;
>+  private final static long serialVersionUID = -1116180831621385484L;

We use the modidier order specified by JLS which is "static final"
 
>-  /** DOCUMENT ME! */
>   private Object queueLock = new Object();

Even private stuff should be documented. Just removing the signal to
document it is of no help. Some explaining words are always good,

> 
>-  /** DOCUMENT ME! */
>   private Waker waker;
> 
>-  private Runnable drainer = new Runnable() 
>+  private Runnable drainer =
>+    new Runnable()

The new should be on the same line.

>     {
>       public void run()
>       {
>@@ -80,103 +82,105 @@
>       }
>     };
> 
>-  /**
>-   * DOCUMENT ME!
>-   */
>   private void queueEvent()
>   {
>     synchronized (queueLock)
>-      {
>-      queue++;
>-      if (queue == 1)
>-        SwingUtilities.invokeLater(drainer);
>-      }
>+    {
>+      queue++;
>+      if (queue == 1)
>+        SwingUtilities.invokeLater(drainer);
>+    }

This should be indented as it was. We indent by two spaces after
keywords, exceptions are class, interface and method bodies only.

>-      if (isCoalesce())
>-        {
>-          if (queue > 0)
>-            fireActionPerformed();
>-        }
>-      else
>-        {
>-          while (queue > 0)
>-            {
>-              fireActionPerformed();
>-              queue--;
>-            }
>-        }
>-      queue = 0;
>-      }
>+    {
>+      if (isCoalesce())
>+        {
>+          if (queue > 0)
>+            fireActionPerformed();
>+        }
>+      else
>+        {
>+          while (queue > 0)
>+            {
>+              fireActionPerformed();
>+              queue--;
>+            }
>+        }
>+      queue = 0;
>+    }
>   }

Same here.

> 
>+  /**
>+   * If true, the timer prints a message to <code>System.out</code>
>+   * when firing each event.
>+   */
>   static boolean logTimers;
> 
>-  /** DOCUMENT ME! */
>+  /**
>+   * True if the timer coalesces events.
>+   */
>   boolean coalesce = true;
> 
>-  /** DOCUMENT ME! */
>+  /**
>+   * True if the timer is firing repetetive events.
>+   */
>   boolean repeats = true;
> 
>-  /** DOCUMENT ME! */
>+  /**
>+   * True if the timer is currently active, firing events as scheduled.
>+   */
>   boolean running;
> 
>-  /** DOCUMENT ME! */
>   int ticks;
> 
>-  /** DOCUMENT ME! */
>+  /**
>+   * The delay between subsequent repetetive events.
>+   */
>   int delay;
> 
>-  /** DOCUMENT ME! */
>-  int initialDelay;
>-
>   /**
>-   * DOCUMENT ME!
>+   * The initial delay before the first event.
>    */
>-  private class Waker extends Thread
>+  int initialDelay;
>+
>+  private class Waker
>+    extends Thread
>   {
>-    /**
>-     * DOCUMENT ME!
>-     */
>     public void run()
>     {
>       running = true;
>       try
>         {
>-        sleep(initialDelay);
>-        
>-        queueEvent();
>-
>-        while (running)
>-          {
>-            try
>-              {
>-                sleep(delay);
>-              }
>-            catch (InterruptedException e)
>-              {
>-                return;
>-              }
>-            queueEvent();
>-
>-            if (logTimers)
>-              System.out.println("javax.swing.Timer -> clocktick");
>-
>-            if (! repeats)
>-              break;
>-          }
>-        running = false;
>+          sleep(initialDelay);
>+
>+          queueEvent();
>+
>+          while (running)
>+            {
>+              try
>+                {
>+                  sleep(delay);
>+                }
>+              catch (InterruptedException e)
>+                {
>+                  return;
>+                }
>+              queueEvent();
>+
>+              if (logTimers)
>+                System.out.println("javax.swing.Timer -> clocktick");
>+
>+              if (!repeats)
>+                break;
>+            }
>+          running = false;

Same here.

>         }
>       catch (Exception e)
>         {
>-//      System.out.println("swing.Timer::" + e);
>+          //    System.out.println("swing.Timer::" + e);

This debug output should be totally removed and replaced by
"// Ignored." or somthing like this.

>         }
>     }
>   }
>@@ -191,16 +195,19 @@
>   public Timer(int d, ActionListener listener)
>   {
>     delay = d;
>-      initialDelay = d;
>+    initialDelay = d;
> 
>     if (listener != null)
>       addActionListener(listener);
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Sets whether the Timer coalesces multiple pending event firings.
>+   * If the coalescing is enabled, the multiple events that have not been
>+   * fired on time are replaced by the single event. The events may not
>+   * be fired on time if the application is busy.
>    *
>-   * @param c DOCUMENT ME!
>+   * @param c true (default) to enable the event coalescing, false otherwise

true and false should be enclosed in <code>...</code> to make them look
a little different in the final javadocs.

>    */
>   public void setCoalesce(boolean c)
>   {
>@@ -208,9 +215,12 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Checks if the Timer coalesces multiple pending event firings.
>+   * If the coalescing is enabled, the multiple events that have not been
>+   * fired on time are replaced by the single event. The events may not
>+   * be fired on time if the application is busy.
>    *
>-   * @return DOCUMENT ME!
>+   * @return true if the coalescing is enabled, false otherwise
>    */
>   public boolean isCoalesce()
>   {
>@@ -218,9 +228,9 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Add the action listener
>    *
>-   * @param listener DOCUMENT ME!
>+   * @param listener the action listener to add
>    */
>   public void addActionListener(ActionListener listener)
>   {
>@@ -228,9 +238,9 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Remove the action listener.
>    *
>-   * @param listener DOCUMENT ME!
>+   * @param listener the action listener to remove
>    */
>   public void removeActionListener(ActionListener listener)
>   {
>@@ -238,12 +248,13 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>-   *
>-   * @param listenerType DOCUMENT ME!
>+   * Get the event listeners of the given type that are listening for the
>+   * events, fired by this timer.
>    *
>-   * @return DOCUMENT ME!
>+   * @param listenerType the listener type (for example, ActionListener.class)
>    *
>+   * @return the array of event listeners that are listening for the events,
>+   * fired by this timer
>    * @since 1.3
>    */
>   public EventListener[] getListeners(Class listenerType)
>@@ -252,9 +263,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Get the array of action listeners.
>    *
>-   * @return DOCUMENT ME!
>+   * @return the array of action listeners that are listening for the events,
>+   * fired by this timer
>    *
>    * @since 1.4
>    */
>@@ -264,20 +276,20 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Fire the given action event to the action listeners.
>    *
>-   * @param event DOCUMENT ME!
>+   * @param event the event to fire
>    */
>   protected void fireActionPerformed(ActionEvent event)
>   {
>-    ActionListener[] listeners = getActionListeners();
>+    ActionListener listeners[] = getActionListeners();

Please use Java array syntax and not C/C++ array syntax. The [] has to
be after the type and not after the identifier.

>     for (int i = 0; i < listeners.length; i++)
>-      listeners[i].actionPerformed(event);
>+      listeners [ i ].actionPerformed(event);

We dont write the whitespaces, "listeners[i]" was okay.

>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Fire the action event, named "Timer".
>    */
>   void fireActionPerformed()
>   {
>@@ -285,9 +297,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Set the timer logging state. If it is set to true, the timer prints
>+   * a message to <code>System.out</code> when firing each action event.
>    *
>-   * @param lt DOCUMENT ME!
>+   * @param lt true if logging is enabled, false (default value) otherwise
>    */
>   public static void setLogTimers(boolean lt)
>   {
>@@ -295,9 +308,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Return the logging state.
>    *
>-   * @return DOCUMENT ME!
>+   * @return true if the timer is printing a message to 
><code>System.out</code>
>+   * when firing each action event
>    */
>   public static boolean getLogTimers()
>   {
>@@ -305,9 +319,11 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Set the delay between firing the subsequent events.
>+   * This parameter does not change the value of the initial delay before
>+   * firing the first event.
>    *
>-   * @param d DOCUMENT ME!
>+   * @param d The time gap between the subsequent events, in milliseconds
>    */
>   public void setDelay(int d)
>   {
>@@ -315,9 +331,9 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Get the delay between firing the subsequent events.
>    *
>-   * @return DOCUMENT ME!
>+   * @return The delay between subsequent events, in milliseconds
>    */
>   public int getDelay()
>   {
>@@ -325,9 +341,12 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Set the intial delay before firing the first event since calling
>+   * the <code>start()</code> method. If the initial delay has not been
>+   * set, it is assumed having the same value as the delay between the
>+   * subsequent events.
>    *
>-   * @param i DOCUMENT ME!
>+   * @param i the initial delay, in milliseconds
>    */
>   public void setInitialDelay(int i)
>   {
>@@ -335,9 +354,11 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Get the intial delay before firing the first event since calling
>+   * the <code>start()</code> method. If the initial delay has not been
>+   * set, returns the same value as <code>getDelay()</code>.
>    *
>-   * @return DOCUMENT ME!
>+   * @return the initial delay before firing the first action event.
>    */
>   public int getInitialDelay()
>   {
>@@ -345,9 +366,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Enable firing the repetetive events.
>    *
>-   * @param r DOCUMENT ME!
>+   * @param r true (default value) to fire repetetive events. False to fire
>+   * only one event after the initial delay
>    */
>   public void setRepeats(boolean r)
>   {
>@@ -355,9 +377,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Check is this timer fires repetetive events.
>    *
>-   * @return DOCUMENT ME!
>+   * @return true if the timer fires repetetive events, false if it fires
>+   * only one event after the initial delay
>    */
>   public boolean isRepeats()
>   {
>@@ -365,9 +388,10 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Get the timer state.
>    *
>-   * @return DOCUMENT ME!
>+   * @return true if the timer has been started and is firing the action
>+   * events as scheduled. False if the timer is inactive.
>    */
>   public boolean isRunning()
>   {
>@@ -375,7 +399,7 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Start firing the action events.
>    */
>   public void start()
>   {
>@@ -386,7 +410,8 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Cancel all pending tasks and fire the first event after the initial
>+   * delay.
>    */
>   public void restart()
>   {
>@@ -395,7 +420,7 @@
>   }
> 
>   /**
>-   * DOCUMENT ME!
>+   * Stop firing the action events.
>    */
>   public void stop()
>   {
>@@ -403,8 +428,8 @@
>     if (waker != null)
>       waker.interrupt();
>     synchronized (queueLock)
>-      {
>-      queue = 0;
>-      }
>+    {
>+      queue = 0;
>+    }

The old indention of { and } were correct but the "queue = 0;" seems to
miss two spaces of indention.

Thanks for your work. I dont want to be pedantic. I just want to get a
make our coding style the same in whole classpath over time.
The javadoc comments are really good.


Michael