Even though the JavaDoc does not contain a hint about it, Calendars are inherently unsafe for multihtreaded use. Sharing a single instance across thread boundaries without proper synchronization will result in erratic behavior of the application. Under 1.4 problems seem to surface less often than under Java 5 where you will probably see random ArrayIndexOutOfBoundsExceptions or IndexOutOfBoundsExceptions in sun.util.calendar.BaseCalendar.getCalendarDateFromFixedDate().

You may also experience serialization problems.

Using an instance field is recommended.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.

As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use. Sharing a single instance across thread boundaries without proper synchronization will result in erratic behavior of the application.

You may also experience serialization problems.

Using an instance field is recommended.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.

This instance method synchronizes on this.getClass(). If this class is subclassed, subclasses will synchronize on the class object for the subclass, which isn't likely what was intended. For example, consider this code from java.awt.Label:

     private static final String base = "label";
     private static int nameCounter = 0;
     String constructComponentName() {
        synchronized (getClass()) {
            return base + nameCounter++;
        }
     }
     

Subclasses of Label won't synchronize on the same subclass, giving rise to a datarace. Instead, this code should be synchronizing on Label.class

     private static final String base = "label";
     private static int nameCounter = 0;
     String constructComponentName() {
        synchronized (Label.class) {
            return base + nameCounter++;
        }
     }
     

Bug pattern contributed by Jason Mehrens

The code synchronizes on a boxed primitive constant, such as an Boolean.

private static Boolean inited = Boolean.FALSE;
...
  synchronized(inited) { 
    if (!inited) {
       init();
       inited = Boolean.TRUE;
       }
     }
...

Since there normally exist only two Boolean objects, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock

The code synchronizes on a boxed primitive constant, such as an Integer.

private static Integer count = 0;
...
  synchronized(count) { 
     count++;
     }
...

Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock

The code synchronizes on an apparently unshared boxed primitive, such as an Integer.

private static final Integer fileLock = new Integer(1);
...
  synchronized(fileLock) { 
     .. do something ..
     }
...

It would be much better, in this code, to redeclare fileLock as

private static final Object fileLock = new Object();

The existing code might be OK, but it is confusing and a future refactoring, such as the "Remove Boxing" refactoring in IntelliJ, might replace this with the use of an interned Integer object shared throughout the JVM, leading to very confusing behavior and potential deadlock.

This method synchronizes on a field in what appears to be an attempt to guard against simultaneous updates to that field. But guarding a field gets a lock on the referenced object, not on the field. This may not provide the mutual exclusion you need, and other threads might be obtaining locks on the referenced objects (for other purposes). An example of this pattern would be:

private Long myNtfSeqNbrCounter = new Long(0);
private Long getNotificationSequenceNumber() {
     Long result = null;
     synchronized(myNtfSeqNbrCounter) {
         result = new Long(myNtfSeqNbrCounter.longValue() + 1);
         myNtfSeqNbrCounter = new Long(result.longValue());
     }
     return result;
 }

The code synchronizes on interned String.

private static String LOCK = "LOCK";
...
  synchronized(LOCK) { ...}
...

Constant Strings are interned and shared across all other classes loaded by the JVM. Thus, this could is locking on something that other code might also be locking. This could result in very strange and hard to diagnose blocking and deadlock behavior. See http://www.javalobby.org/java/forums/t96352.html and http://jira.codehaus.org/browse/JETTY-352.

This method performs synchronization on an implementation of java.util.concurrent.locks.Lock. You should use the lock() and unlock() methods instead.

Since the field is synchronized on, it seems not likely to be null. If it is null and then synchronized on a NullPointerException will be thrown and the check would be pointless. Better to synchronize on another field.

This method contains a call to java.lang.Object.wait() which is not guarded by conditional control flow.  The code should verify that condition it intends to wait for is not already satisfied before calling wait; any previous notifications will be ignored.

This class contains similarly-named get and set methods where the set method is synchronized and the get method is not.  This may result in incorrect behavior at runtime, as callers of the get method will not necessarily see a consistent state for the object.  The get method should be made synchronized.

This method calls notify() rather than notifyAll().  Java monitors are often used for multiple conditions.  Calling notify() only wakes up one thread, meaning that the thread woken up might not be the one waiting for the condition that the caller just satisfied.

This method contains a call to java.lang.Object.wait() which is not in a loop.  If the monitor is used for multiple conditions, the condition the caller intended to wait for might not be the one that actually occurred.

Waiting on a monitor while two locks are held may cause deadlock.   Performing a wait only releases the lock on the object being waited on, not any other locks.   This not necessarily a bug, but is worth examining closely.

Non-constructor methods should not have the same name as the enclosing class. Example :

public class MyClass {
  // this is bad because it is a method
  public void MyClass() {}
  // this is OK because it is a constructor
  public MyClass() {}
}

This rule is deprecated, use squid:S1223 instead.

This rule uses the NCSS (Non Commenting Source Statements) algorithm to determine the number of lines of code for a given type. NCSS ignores comments, and counts actual statements. Using this algorithm, lines of code that are split are counted as one.

This code negatives the return value of a compareTo or compare method. This is a questionable or bad programming practice, since if the return value is Integer.MIN_VALUE, negating the return value won't negate the sign of the result. You can achieve the same intended result by reversing the order of the operands rather than by negating the results.

Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed. When a block contains a comment, this block is not considered to be empty.

The following code snippet illustrates this rule:

void doSomething() {
  for (int i = 0; i < 42; i++)        // Non-Compliant
  {
  }
  for (int i = 0; i < 42; i++);       // Compliant

  if (myVar == 4)                     // Compliant - contains a comment
  {
    // Do nothing because of X and Y
  }
  else                                // Compliant
  {
    doSomething();
  }

  try                                 // Non-Compliant
  {
  }
  catch (Exception e)                 // Compliant
  {
    // Ignore
  }
}

Nested code blocks can be used to create a new scope and restrict the visibility of the variables defined within. Using this feature in a method typically indicates that it takes on too many responsibilities, and should be refactored into smaller ones.

The following code:

public void evaluate(int operator) {
  switch (operator) {
    /* ... */
    case ADD: {                                // Non-Compliant - nested code block '{' ... '}'
        int a = stack.pop();
        int b = stack.pop();
        int result = a + b;
        stack.push(result);
        break;
      }
    /* ... */
  }
}

should be refactored into:

public void evaluate(int operator) {
  switch (operator) {
    /* ... */
    case ADD:                                  // Compliant
      evaluateAdd();
      break;
    /* ... */
  }
}

private void evaluateAdd() {
  int a = stack.pop();
  int b = stack.pop();
  int result = a + b;
  stack.push(result);
}

Non-static initializers are rarely used, and can be confusing for most developers. When possible, they should be refactored into standard constructors or field initializers.

The following code:

class MyClass {
  private static final Map MY_MAP = new HashMap() {

    // Non-Compliant - HashMap should be extended only to add behavior, not for initialization
    {
      put("a", "b");
    }

  };
}

could be refactored using Guava into:

class MyClass {
  // Compliant
  private static final Map MY_MAP = ImmutableMap.of("a", "b");
}

This Serializable class defines a non-primitive instance field which is neither transient, Serializable, or java.lang.Object, and does not appear to implement the Externalizable interface or the readObject() and writeObject() methods. Objects of this class will not be deserialized correctly if a non-Serializable object is stored in this field.

The field is marked as nonnull, but isn't written to by the constructor. The field might be initialized elsewhere during constructor, or might always be initialized before use.

Object.finalize() is called by the Garbage Collector sometime after the object became unreferenced.

Overloading this method is misleading:

• The overloaded method will not be called by the Garbage Collector.
• Users are not expected to call Object.finalize() and will get confused.

Another name should be picked for the method.

The following code:

public void finalize(int someParameter) {        // Non-Compliant
  /* ... */
}

should be refactored into:

public void someBetterName(int someParameter) {  // Compliant
  /* ... */
}

The contract of the Object.finalize() method is clear: only the Garbage Collector is supposed to call this method. Making this method public is misleading, because it implies that any caller can use it.

The following code snippet illustrates this rule:

public class MyClass {

  @Override
  public void finalize() {    // Non-Compliant
    /* ... */
  }
}