| Name |
Severity |
Description |
| RuleAwtPeer |
Severe |
java.awt.peer interfaces were designed to handle
platform-specific graphics issues in Java. Since Java is meant to be
platform-independent, using java.awt.peer directly is forbidden.
|
public
static void
main( String[] args ) {
Frame frame = new
Frame( "Peers" );
Button button = new
Button( "Hello World!" );
frame.setLayout(
new GridLayout() );
frame.add(
button );
frame.setVisible(
true );
ButtonPeer peer = (ButtonPeer)button.getPeer();
peer.dispose();
}
|
|
|
Remove calls to peer objects.
Use java.awt or javax.swing
interfaces and classes instead.
public
static void
main( String[] args ) {
Frame frame = new
Frame( "Peers" );
Button button = new
Button( "Hello World!" );
frame.setLayout(
new GridLayout() );
frame.add(
button );
frame.setVisible(
true );
}
|
|
|
| RuleCloneableSuperClone |
Severe |
Calling super.clone() from the clone() is essential in order
to ensure that a complete copy of the object is created. Failure to do
so is likely to result in a NullPointerException thrown later or an
unexpected object state in the course of program execution.
|
public
static void
main (String[] args){
double x = 3.1;
int i = (int)x;
System.out.println(i);
}
|
|
|
Change the underlying data type
to be lower precision.
public
static void
main (String[] args){
int i = 3;
System.out.println(i);
}
|
|
|
Update the calculation to
operate on the higher precision data type.
public
static void
main (String[] args){
double x = 3.1;
System.out.println(
x );
}
|
|
|
| RuleComparisonEqualsHashCode |
Severe |
java.lang.Object.equals() and java.lang.Object.hashCode() are
designed to work together for fast hash lookups. The value returned
from the hashCode() is used to calculate the index of the bucket in
the hash table array. The equals() is then used to find the actual
matching object among all the objects that sit in the same bucket.
|
public
EqualsHashCode_Example(String s){
super();
this .s = s;
}
public
int hashCode(){
return s.hashCode();
}
private
String s = "";
|
|
|
Implement both hashCode() and
equals().
public
EqualsHashCode_Solution(String s){
super();
this .s = s;
}
public
int hashCode(){
return s.hashCode();
}
public
boolean equals( Object other ) {
return ( other instanceof EqualsHashCode_Solution
) &&
((EqualsHashCode_Solution)other).s.equals(
s );
}
private
String s = "";
|
|
|
| RuleComparisonClassNameComparison |
Severe |
Using java.lang.Class.getName() bypasses the standard way of
comparing classes and may lead to problems, including security
violations. A standard way of comparing class objects is to use
java.lang.Class.equals(). This method bases the class equality on all
aspects of the class, for example, the ClassLoader that loaded the
class.
|
public
static void
main(String[] args){
Class c1 = null;
Class c2 = null;
if ( args.length > 1){
try {
c1 =
Class.forName( args[0] );
c2 =
Class.forName( args[1] );
System.out.println(
c1.getName().equals(c2.getName()));
} catch (
ClassNotFoundException e){
e.printStackTrace();
}
}
}
|
|
|
Change the code to use
java.lang.Class.equals() to establish class equality.
public
static void
main(String[] args){
Class c1 = null;
Class c2 = null;
if ( args.length > 1){
try {
c1 =
Class.forName( args[0] );
c2 =
Class.forName( args[1] );
System.out.println(
c1.equals( c2 ) );
} catch (
ClassNotFoundException e){
e.printStackTrace();
}
}
}
|
|
|
| RuleNullEmptyArray |
Severe |
Returning null instead of empty array is error-prone in
majority of cases. By Java convention, the user of the interface that
returns array does not have to check for null, but should assume the
zero length array. Violating this convention is likely to result in
null pointer exceptions.
|
public
NullEmptyArray_Example() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
ArrayList(10);
}
values.add(
value );
}
public
Integer[] getValues() {
if ( values == null
) {
return null;
}
return (Integer[])values.toArray(
new Integer[ 0
] );
}
private
List values;
public
static void
main( String[] args ) {
NullEmptyArray_Example example = new NullEmptyArray_Example();
Integer[] values = example.getValues();
for ( int
i = 0; i < values.length; i++ )
{
System.out.println( values[ i ] );
}
}
|
|
|
Return zero length array instead
of null.
public
NullEmptyArray_Solution() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
ArrayList(10);
}
values.add(
value );
}
public
Integer[] getValues() {
return ( values == null ) ? new Integer[
0 ] :
(Integer[])values.toArray(
new Integer[ 0
] );
}
private
List values;
public
static void
main( String[] args ) {
NullEmptyArray_Solution example = new NullEmptyArray_Solution();
Integer[] values = example.getValues();
for ( int
i = 0; i < values.length; i++ )
{
System.out.println( values[ i ] );
}
}
|
|
|
| RuleSerializationReadWriteObject |
Severe |
Declaring readObject() and writeObject() private is essential
to follow Java serialization protocol. Failing to do so may lead to
security problems in the application.
|
public
static class
SerializableObject implements
Serializable{
public void
writeObject(ObjectOutputStream s) throws IOException {
s.defaultWriteObject();
}
public void
readObject(ObjectInputStream s) throws IOException,
ClassNotFoundException {
s.defaultReadObject();
}
static final
long serialVersionUID = 123;
}
|
|
|
Declare readObject() and
writeObject() private.
public
static class
SerializableObject implements
Serializable{
private void
writeObject(ObjectOutputStream s) throws IOException {
s.defaultWriteObject();
}
private void
readObject(ObjectInputStream s) throws IOException,
ClassNotFoundException {
s.defaultReadObject();
}
static final
long serialVersionUID = 123;
}
|
|
|
| RuleSerializationResolveObject |
Severe |
Declaring resolveObject() protected is essential in order to
follow Java serialization protocol. Failing to do so may lead to
security problems in the application.
|
public
class ResolveObject implements Serializable {
public static
ResolveObject getInstance() {
return INSTANCE;
}
private static
final ResolveObject
INSTANCE = new ResolveObject();
private ResolveObject() {
super();
}
public Object
readResolve() throws
ObjectStreamException {
return
INSTANCE;
}
static final
long serialVersionUID = 123;
}
|
|
|
Declare resolveObject() and
writeReplace() protected.
public
class ResolveObject implements Serializable {
public static
ResolveObject getInstance() {
return INSTANCE;
}
private static
final ResolveObject
INSTANCE = new ResolveObject();
private ResolveObject() {}
private Object
readResolve() throws
ObjectStreamException {
return
INSTANCE;
}
static final
long serialVersionUID = 123;
}
|
|
|
| RuleSerializationSerializableField |
Severe |
Every field of a serializable class must be either
serializable or transient. Declaring non-transient fields of
non-serializable type inside of a serializable class will result in an
exception thrown during the serialization.
|
public static
final class
SomeType {}
public
class SerializableField implements Serializable {
public
SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private SomeType
nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
Make the field type
serializable.
public
static final
class SomeType implements Serializable {}
public
class SerializableField()
implements Serializable {
public SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private SomeType
nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
Declare the field transient.
public
static final
class SomeType {}
public
class SerializableField implements Serializable {
public SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private transient
SomeType nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
| nonsynchronizednotify |
Severe |
A thread that is currently locking sections of the code that
use a common lock should invoke java.lang.Object.notify() to tell the
blocking thread to resume processing. Calling notify() from
non-synchronized blocks will result in IllegalMonitorStateException,
because no lock has been obtained.
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
lock.unlock();
lock.notify();
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Place the call to
java.lang.Object.notify() inside of the synchronized block.
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notify();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| nonsynchronizednotifyall |
Severe |
A thread that is currently locking sections of the code that
use common lock should invoke java.lang.Object.notifyAll() to tell the
blocking threads to resume processing. Calling notify() from
non-synchronized blocks will result in IllegalMonitorStateException,
because no lock has been obtained.
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer"
+ id + ": waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
lock.unlock();
lock.notifyAll();
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Place the call to
java.lang.Object.notifyAll() inside of the synchronized block.
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer"
+ id + ": waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| nonsynchronizedwait |
Severe |
A thread that needs to wait on a common lock should invoke
java.lang.Object.wait() to tell the JVM to suspend its processing
until it receives notification from another thread. Calling wait()
from non-synchronized blocks will result in
IllegalMonitorStateException, because no lock has been obtained.
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
while (
!lock.isAvailable() ) {
System.out.println(
"Consumer: waiting..." );
try {
lock.wait();
} catch (InterruptedException
e) {}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Place the call to
java.lang.Object.wait() inside of the synchronized block.
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| destroy |
Severe |
java.lang.Thread.destroy() method destroys the thread without
releasing the monitors or doing a cleanup. Calling this method is
likely to lead to synchronization and resource management issues.
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
synchronized
( lock ) {
Thread.currentThread().destroy();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Do not call destroy(), rely on
default thread termination.
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| resume |
Severe |
The following is the extract from Java doc (copyright by Sun
Microsystems): This method exists solely for use with suspend(), which
has been deprecated because it is deadlock-prone.
|
public
static final
Boolean lock = Boolean.TRUE;
public
static boolean
runCond = true;
public
static void
main( String[] args ) {
Runnable runnable = new Runnable() {
public void run() {
synchronized (lock){
while (
runCond ) {
System.out.println(
"Hello World!" );
try { wait( 1000
); } catch (InterruptedException e) {}
}
}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
thread.suspend();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
thread.resume();
runCond =
false;
}
|
|
|
Remove the code that calls
suspend/resume thread.
Use provided SuspendSafeRunnable.
public
static abstract
class SuspendSafeRunnable extends StopSafeRunnable {
public void
doRun() {
checkForPause();
doWork();
}
public void
suspend() {
synchronized
( lock ) {
suspended = true;
lock.notifyAll();
}
}
public void
resume() {
synchronized
( lock ) {
suspended = false;
lock.notifyAll();
}
}
public boolean
isPaused() {
return
suspended;
}
protected abstract
void doWork();
private void
checkForPause() {
synchronized
( lock ) {
while ( suspended ) {
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
}
private boolean
suspended = false;
private Object
lock = new Object();
}
public
static void
main( String[] args ) {
SuspendSafeRunnable runnable = new SuspendSafeRunnable() {
public void doWork() {
System.out.println(
"Hello World!" );
try { Thread.sleep( 1000 ); } catch
(InterruptedException e) {}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.suspend();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.resume();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.stop();
}
|
|
|
| stop |
Severe |
The following is the extract from Java doc (copyright by Sun
Microsystems): This method is inherently unsafe. Stopping a thread
with Thread.stop causes it to unlock all of the monitors that it has
locked (as a natural consequence of the unchecked ThreadDeath
exception propagating up the stack). If any of the objects previously
protected by these monitors were in an inconsistent state, the damaged
objects become visible to other threads, potentially resulting in
arbitrary behavior.
|
public
static final
Boolean lock = Boolean.TRUE;
public
static void
main( String[] args ) {
Runnable runnable = new Runnable() {
public void run() {
synchronized (lock){
while ( true ) {
System.out.println(
"Hello World!" );
try { wait( 1000
); } catch (InterruptedException e) {}
}
}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
thread.stop();
}
|
|
|
The following is the extract
from Java doc (copyright by Sun Microsystems)
Many uses of stop should be replaced by code that simply modifies
some variable to indicate that the target thread should stop
running.
The target thread should check this variable regularly, and return
from its run method in an orderly fashion if the variable indicates
that it is to stop running. If the target thread waits for long
periods (on a condition variable, for example), the interrupt method
should be used to interrupt the wait.
Use provided StopSafeRunnable.
public
static abstract
class StopSafeRunnable implements Runnable {
public final
void run() {
while (
!stopped ) {
doRun();
}
}
public void
stop() {
stopped = true;
}
public boolean
isStopped() {
return
stopped;
}
protected abstract
void doRun();
private boolean
stopped = false;
}
public
static void
main(String[] args) {
StopSafeRunnable runnable = new StopSafeRunnable() {
public void doRun() {
System.out.println(
"Hello World" );
try { Thread.sleep( 1000 ); } catch
(InterruptedException e) {}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.stop();
}
|
|
|
| suspend |
Severe |
The following is the extract from Java doc (copyright by Sun
Microsystems): This method has been deprecated, as it is inherently
deadlock-prone. If the target thread holds a lock on the monitor
protecting a critical system resource when it is suspended, no thread
can access this resource until the target thread is resumed. If the
thread that would resume the target thread attempts to lock this
monitor prior to calling resume, deadlock results. Such deadlocks
typically manifest themselves as "frozen" processes.
|
public
static final
Boolean lock = Boolean.TRUE;
public
static boolean
runCond = true;
public
static void
main( String[] args ) {
Runnable runnable = new Runnable() {
public void run() {
synchronized (lock){
while (
runCond ) {
System.out.println(
"Hello World!" );
try { wait( 1000
); } catch (InterruptedException e) {}
}
}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
thread.suspend();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
thread.resume();
runCond =
false;
}
|
|
|
Remove the code that calls
suspend/resume thread.
Use provided SuspendSafeRunnable.
public
static abstract
class SuspendSafeRunnable extends StopSafeRunnable {
public void
doRun() {
checkForPause();
doWork();
}
public void
suspend() {
synchronized
( lock ) {
suspended = true;
lock.notifyAll();
}
}
public void
resume() {
synchronized
( lock ) {
suspended = false;
lock.notifyAll();
}
}
public boolean
isPaused() {
return
suspended;
}
protected abstract
void doWork();
private void
checkForPause() {
synchronized
( lock ) {
while ( suspended ) {
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
}
private boolean
suspended = false;
private Object
lock = new Object();
}
public
static void
main( String[] args ) {
SuspendSafeRunnable runnable = new SuspendSafeRunnable() {
public void doWork() {
System.out.println(
"Hello World!" );
try { Thread.sleep( 1000 ); } catch
(InterruptedException e) {}
}
};
Thread thread = new
Thread( runnable );
thread.start();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.suspend();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.resume();
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
runnable.stop();
}
|
|
|
| overridesynchronizable |
Severe |
Synchronized methods use this as a lock to prevent other
methods and blocks with the same lock from parallel execution.
Overriding the synchronized method with a non-synchronized one
introduces the potential for problems and data inconsistency in the
base class.
|
public
static class
Base {
public synchronized
void setValue( int value ) {
try {
Thread.sleep( 1000 ); } catch (InterruptedException
e) {}
this
.value = value;
}
public synchronized
int getValue() {
return
value;
}
protected int
value = 0;
}
public
static class
Derived extends Base {
public int
getValue() {
return this .value;
}
}
public
static void
main(String[] args) {
System.out.println(
"Testing Base..." );
test( new Base() );
try { Thread.sleep( 5000 ); } catch
(InterruptedException e) {}
System.out.println(
"Testing Derived..." );
test( new Derived() );
}
private
static void
test( final Base base ) {
Runnable writer = new Runnable() {
public void run() {
System.out.println(
"Writer set value " );
base.setValue(
1 );
}
};
Runnable reader = new Runnable() {
public void run() {
System.out.println(
"Reader: " + base.getValue() );
}
};
(new Thread( writer ) ).start();
(new Thread( reader ) ).start();
}
|
|
|
Change the overriding method to
be synchronized.
public
static class
Base {
public synchronized
void setValue( int value ) {
try {
Thread.sleep( 1000 ); } catch (InterruptedException
e) {}
this
.value = value;
}
public synchronized
int getValue() {
return
value;
}
protected int
value = 0;
}
public
static class
Derived extends Base {
public synchronized
int getValue() {
return this .value;
}
}
public
static void
main(String[] args) {
System.out.println(
"Testing Base..." );
test( new Base() );
try { Thread.sleep( 5000 ); } catch
(InterruptedException e) {}
System.out.println(
"Testing Derived..." );
test( new Derived() );
}
private
static void
test( final Base base ) {
Runnable writer = new Runnable() {
public void run() {
System.out.println(
"Writer set value " );
base.setValue(
1 );
}
};
Runnable reader = new Runnable() {
public void run() {
System.out.println(
"Reader: " + base.getValue() );
}
};
(new Thread( writer ) ).start();
(new Thread( reader ) ).start();
}
|
|
|
| RuleCastingPrimitives |
Warning |
Casting primitives to a lower precision may result in the
information loss. Such casting indicates an incorrect choice of the
data type or a calculation that ignores properties of the data that it
operates on.
|
public
static void
main (String[] args){
double x = 3.1;
int i = (int)x;
System.out.println(i);
}
|
|
|
Change the underlying data type
to be lower precision.
public
static void
main (String[] args){
int i = 3;
System.out.println(i);
}
|
|
|
Update the calculation to
operate on the higher precision data type.
public
static void
main (String[] args){
double x = 3.1;
System.out.println(
x );
}
|
|
|
| RuleCloneableOverrideClone |
Warning |
Implementing Cloneable without overriding clone() is
confusing and unclear. By implementing Cloneable, the class signals
that the default, shallow copy mechanism, is not sufficient, and that
the class needs to perform additional operations in order to create a
valid copy.
|
public
OverrideClone_Example() {
super();
elements =
new int[ 1
];
}
public
void setElements( int value ) {
for ( int
i = 0; i < elements.length; i++
) {
elements[ i ] = value;
}
}
public
void print() {
for ( int
i = 0; i < elements.length; i++
) {
System.out.println( elements[ i ] );
}
}
private
int[] elements;
public
static void
main(String[] args){
OverrideClone_Example original = new OverrideClone_Example();
original.setElements(
1 );
OverrideClone_Example clone = null;
try {
clone = (OverrideClone_Example)original.clone();
System.out.println( "Clone
before modification of original" );
clone.print();
original.setElements( 2 );
System.out.println( "Clone
after modification of original" );
clone.print();
}catch ( CloneNotSupportedException e ) {
e.printStackTrace();
}
}
|
|
|
Remove the implements Cloneable
declaration or implement the clone() properly.
public
OverrideClone_Solution() {
super();
elements =
new int[ 1
];
}
public
void setElements( int value ) {
for ( int
i = 0; i < elements.length; i++
) {
elements[ i ] = value;
}
}
public
void print() {
for ( int
i = 0; i < elements.length; i++
) {
System.out.println( elements[ i ] );
}
}
public
Object clone() throws CloneNotSupportedException {
OverrideClone_Solution clone = (OverrideClone_Solution)super .clone();
clone.elements
= new int[ elements.length ];
for ( int
i = 0; i < elements.length; i++
) {
clone.elements[ i ] = elements[ i ];
}
return clone;
}
private
int[] elements;
public
static void
main(String[] args){
OverrideClone_Solution original = new OverrideClone_Solution();
original.setElements(
1 );
OverrideClone_Solution clone = null;
try {
clone = (OverrideClone_Solution)original.clone();
System.out.println( "Clone
before modification of original" );
clone.print();
original.setElements( 2 );
System.out.println( "Clone
after modification of original" );
clone.print();
}
catch ( CloneNotSupportedException
e ) {
e.printStackTrace();
}
}
|
|
|
| RuleCloneableImplementCloneable |
Warning |
Overriding clone() without implementing Cloneable is
confusing and unclear. By implementing Cloneable, the class signals
that the default, shallow copy mechanism, is not sufficient, and that
the class needs to perform additional operations in order to create a
valid copy.
|
public class
ExampleClone {
public ExampleClone() {
super();
elements = new int[
1 ];
}
public void
setElements( int value ) {
for ( int i = 0;
i < elements.length; i++ ) {
elements[ i ] =
value;
}
}
public void
print() {
for ( int i = 0;
i < elements.length; i++ ) {
System.out.println(
elements[ i ] );
}
}
public Object
clone() throws
CloneNotSupportedException {
ExampleClone
clone = (ExampleClone)super .clone();
clone.elements = new int[
elements.length ];
for ( int i = 0;
i < elements.length; i++ ) {
clone.elements[
i ] = elements[ i ];
}
return
clone;
}
private int[]
elements;
}
|
|
|
Use the implements Cloneable
declaration or change the name of the clone() method.
public
class ExampleClone implements Cloneable {
public ExampleClone() {
super();
elements = new int[
1 ];
}
public void
setElements( int value ) {
for ( int i = 0;
i < elements.length; i++ ) {
elements[ i ] =
value;
}
}
public void
print() {
for ( int i = 0;
i < elements.length; i++ ) {
System.out.println(
elements[ i ] );
}
}
public Object
clone() throws
CloneNotSupportedException {
ExampleClone
clone = (ExampleClone)super .clone();
clone.elements = new int[
elements.length ];
for ( int i = 0;
i < elements.length; i++ ) {
clone.elements[
i ] = elements[ i ];
}
return
clone;
}
private int[]
elements;
}
|
|
|
| RuleComparisonInstanceOfEquals |
Warning |
Using instanceof in the equals() of an object is critical in
order to ensure that equals does not throw a ClassCastException and
returns the correct value.
|
public
InstanceofEquals_Example( String str
) {
super();
this .str = str;
}
public
String getString(){
return str;
}
public
int hashCode() {
return str.hashCode();
}
public
boolean equals(Object other){
return str.equals( ( (InstanceofEquals_Example)other
).getString() );
}
private
String str;
|
|
|
Use instanceof
in the equals() before casting.
public
InstanceofEquals_Solution( String
str ) {
super();
this .str = str;
}
public
String getString(){
return str;
}
public
int hashCode() {
return str.hashCode();
}
public
boolean equals(Object other) {
return ( other instanceof InstanceofEquals_Solution)
&&
str.equals( ( (InstanceofEquals_Solution)other
).getString() );
}
private
String str;
|
|
|
| RuleComparisonReferenceEquality |
Warning |
Operators == and != compare references of objects. In most
cases, the two objects should be considered the same if they have the
same state. It is better to use java.lang.Object.equals() for
comparison, since this method allows for the implementer to make the
object state comparison.
|
public
static void
main( String[] args ){
Integer first = new
Integer( 1 );
Integer second = new Integer( 1
);
System.out.println(
first == second );
}
|
|
|
Use java.lang.Object.equals().
public
static void
main( String[] args ){
Integer first = new
Integer( 1 );
Integer second = new Integer( 1
);
System.out.println(
first.equals( second ) );
}
|
|
|
| RuleExceptionsInstanceofCatch |
Warning |
Since Java supports multiple catch clauses in the try block,
there is never a need to check for the instance of the exception in
the catch clause. Catch clauses should be short, clear statements that
deal directly with the specific exception that was thrown. Adding
instanceof exception checks is likely to lead to non-standard,
difficult to maintain code.
|
public
static class
SomeException extends Exception{
public SomeException(String str, int
value ){
super( str );
this
.value = value;
}
public int
getValue() {
return
value;
}
private int
value;
}
public
static void
createProblem() throws
SomeException {
throw new
SomeException( "Problem", 10 );
}
public
static void
main(String[] args){
try {
createProblem();
} catch ( Exception
e ){
if ( e instanceof SomeException
) {
System.out.println(
((SomeException)e).getValue() );
} else {
System.out.println(
e.getLocalizedMessage() );
e.printStackTrace();
}
}
}
|
|
|
Remove instanceof checks on
exceptions and add dedicated catch clauses to the try/catch block.
public
static class
SomeException extends Exception{
public SomeException(String str, int
value ){
super( str );
this
.value = value;
}
public int
getValue() {
return
value;
}
private int
value;
}
public
static void
createProblem() throws
SomeException {
throw new
SomeException( "Problem", 10 );
}
public
static void
main(String[] args){
try {
createProblem();
} catch ( SomeException
e ){
System.out.println( ((SomeException)e).getValue() );
}
}
|
|
|
| RuleLoopAssignLoopVariable |
Warning |
Assigning the for loop control variable in the body of the
loop is likely to lead to an error. For loops already provide the
default increment mechanism that should be followed. When the default
increment mechanism does not apply, the for loop should be changed to
a while or a do loop.
|
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
System.out.println( args[ i ] );
i = i + 1;
}
}
|
|
|
Move the assignment-to-control
variable from inside of the for loop to the increment clause of the
loop.
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i = i +
2 ) {
System.out.println( args[ i ] );
}
}
|
|
|
Change the for loop to a while
loop
public
static void
main( String[] args ) {
int i = 0;
while ( i < args.length ) {
System.out.println( args[ i ] );
i = i + 2;
}
}
|
|
|
| RuleNullEnumeration |
Warning |
Returning null instead of empty Enumeration is error-prone in
majority of cases. By Java convention, the client of the interface
that returns Enumeration does not have to check for null, but should
assume the empty enumeration that returns false in hasMoreElements().
Violating this convention is likely to result in null pointer
exceptions.
|
public
NullEnumeration_Example() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
Vector(10);
}
values.add(
value );
}
public
Enumeration getValues() {
if ( values == null
) {
return null;
}
return values.elements();
}
private
Vector values;
public
static void
main( String[] args ) {
NullEnumeration_Example example = new NullEnumeration_Example();
for ( Enumeration
enum = example.getValues(); enum.hasMoreElements(); ) {
System.out.println( enum.nextElement() );
}
}
|
|
|
Return empty Enumeration instead
of null.
public
NullEnumeration_Solution() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
Vector(10);
}
values.add(
value );
}
public
Enumeration getValues() {
return ( values != null ) ? values.elements() : new Enumeration() {
public boolean hasMoreElements() {
return false;
}
public Object nextElement() {
throw new
UnsupportedOperationException();
}
};
}
private
Vector values;
public
static void
main( String[] args ) {
NullEnumeration_Example example = new NullEnumeration_Example();
for ( Enumeration
enum = example.getValues(); enum.hasMoreElements(); ) {
System.out.println( enum.nextElement() );
}
}
|
|
|
| RuleNullIterator |
Warning |
Returning null instead of empty Iterator is error-prone in
majority of cases. By Java convention, the client of the interface
that returns Iterator does not have to check for null, but should
assume the empty Iterator that returns false in hasNext(). Violating
this convention is likely to result in null pointer exceptions.
|
public
NullIterator_Example() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
ArrayList(10);
}
values.add(
value );
}
public
Iterator getValues() {
if ( values == null
) {
return null;
}
return values.iterator();
}
private
List values;
public
static void
main( String[] args ) {
NullIterator_Example example = new NullIterator_Example();
for ( Iterator
iter = example.getValues(); iter.hasNext(); ) {
System.out.println( iter.next() );
}
}
|
|
|
Return empty Iterator instead of
null.
public
NullIterator_Solution() {
super();
}
public
void addValue( Integer value ) {
if ( values == null
) {
values = new
ArrayList(10);
}
values.add(
value );
}
public
Iterator getValues() {
return ( values != null ) ? values.iterator() :
new
Iterator() {
public boolean
hasNext() {
return false;
}
public Object
next() {
throw new UnsupportedOperationException();
}
public void
remove() {
throw new UnsupportedOperationException();
}
};
}
private
List values;
public
static void
main( String[] args ) {
NullIterator_Example example = new NullIterator_Example();
for ( Iterator
iter = example.getValues(); iter.hasNext(); ) {
System.out.println( iter.next() );
}
}
|
|
|
| RulePortabilitySystemGetenv |
Warning |
The following is the extract from Java doc (copyright by Sun
Microsystems): The preferred way to extract system-dependent
information is the system properties of the
java.lang.System.getProperty methods and the corresponding getTypeName
methods of the Boolean, Integer, and Long primitive.
|
public
static void
main(String[] args){
System.out.println(
System.getenv("PATH") );
}
|
|
|
Use
java.lang.System.getProperty()
public
static void
main(String[] args){
System.out.println(
System.getProperty("PATH") );
}
|
|
|
| RulePortabilityLineSeparators |
Warning |
Hard coding line separators as '\n' or '\r' makes the code
non portable. In Java there is a system property called
'line.separator' specifically designed for portability.
|
public
static void
main( String[] args) {
FileWriter fw = null;
try {
fw = new
FileWriter( "LineSeparators.txt" );
fw.write( "Hello
World!" );
fw.write("\n");
}catch (Exception
e){
e.printStackTrace();
} finally {
if ( fw !=
null ) {
try { fw.close(); } catch ( Exception
e ) {}
}
}
}
|
|
|
Use System.getProperty(
"line.separator" ) instead.
public
static void
main( String[] args) {
FileWriter fw = null;
try {
fw = new
FileWriter( "LineSeparators.txt" );
fw.write( "Hello
World!" );
fw.write( System.getProperty( "line.separator" ) );
}catch (Exception
e){
e.printStackTrace();
} finally {
if ( fw !=
null ) {
try { fw.close(); } catch ( Exception
e ) {}
}
}
}
|
|
|
| RulePortabilityFileSeparator |
Warning |
Hard coding a file separator as '/' or '\' makes the code non
portable. The java.io.File class contains a special field called
'separator' specifically designed for portability.
|
public
static void
main(String[] args) {
if ( args.length > 2 ) {
File f = new File( args[ 0 ] + "/"
+ args[ 1 ] );
System.out.println( f.getAbsolutePath() );
}
}
|
|
|
Use java.io.File.separator
instead.
public
static void
main(String[] args) {
if ( args.length > 2 ) {
File f = new File( args[ 0 ] + File.separator + args[ 1 ] );
System.out.println( f.getAbsolutePath() );
}
}
|
|
|
| RuleSwitchBreak |
Warning |
If a break is missing in a case branch of a switch statement,
the code continues by executing the next case branch. While there are
situations when this is the intent, they are quite rare. More often,
the intent is not to execute the following case but to break out of
the current case branch. Putting the break at the end of each case
branch makes code safer and more clear.
|
public
static void
main(String[] args){
switch ( args.length ){
case 0:
System.out.println("No
parameters");
case 1:
System.out.println("Only
one parameter: args[0]");
default :
System.out.println("Parameters: ");
for ( int i = 0,
n = args.length; i < n; i ++){
System.out.println(args[i]);
}
}
}
|
|
|
Insert the break statement.
public
static void
main(String[] args){
switch ( args.length ){
case 0:
System.out.println("No
parameters");
break ;
case 1:
System.out.println("Only
one parameter: args[0]");
break ;
default :
System.out.println("Parameters: ");
for ( int
i = 0, n = args.length; i < n; i
++){
System.out.println(args[i]);
}
break ;
}
}
|
|
|
| RuleCastingObjectDowncast |
Recommendation |
Returning java.lang.Object is unclear and error prone. The
problem can manifest itself as the runtime ClassCastException. In
order to avoid this exception, the users of the interface need to read
(and often re-read) the documentation and maybe even look into the
details of the implementation.
|
public
ObjectDowncast_Example( String str )
{
super();
this .str = str;
}
public
Object getString(){
return str;
}
private
String str;
public
static void
main(String[] args){
ObjectDowncast_Example example = new ObjectDowncast_Example("Hello World!");
String str = (String)example.getString();
System.out.println(
str );
}
|
|
|
Change the method to return the
actual data type instead of java.lang.Object.
If the method returns objects of types that do not share a common
interface or a class, create the new interface or class and
encapsulate the object in it.
public
ObjectDowncast_Solution( String str
) {
super();
this .str = str;
}
public
String getString(){
return str;
}
private
String str;
public
static void
main(String[] args){
ObjectDowncast_Solution example = new ObjectDowncast_Solution("Hello World!");
String str = example.getString();
System.out.println(
str );
}
|
|
|
| RuleComparisonPlaceConstants |
Recommendation |
Placing constants on the left side of equals() eliminates the
need for extra checking and reduces incidences of null pointer
exceptions.
|
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if ( args[
i ].equals( "Hello World!" ) ) {
System.out.println(
i );
}
}
}
|
|
|
Change the
equals () expression to have the constant on the left side.
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if ( "Hello World!".equals( args[ i ] ) ) {
System.out.println(
i );
}
}
}
|
|
|
| RuleConditionalTernary |
Recommendation |
Using an if/else statement instead of a ternary operator
makes code longer than necessary. A ternary operator makes the code
more compact, while preserving the clarity of the if/else statement.
|
public
static void
main( String[] args ) {
ternaryVariableAssignment(
args );
ternaryReturn(
args );
}
public
static void
ternaryVariableAssignment( String[]
args ) {
int j = 0;
for ( int
i = 0; i < args.length; i++ ) {
if ( i
> 5 ) {
j = i;
} else {
j = 10;
}
}
}
public
static boolean
ternaryReturn( String[] args ) {
if ( args.length > 5 ) {
return true;
} else {
return false;
}
}
|
|
|
Use a ternary operator instead
of an if/else statement.
public
static void
main( String[] args ) {
ternaryVariableAssignment(
args );
ternaryReturn(
args );
}
public
static void
ternaryVariableAssignment( String[]
args ) {
int j = 0;
for ( int
i = 0; i < args.length; i++ ) {
j = (i > 5)
? i : 10;
}
}
public
static boolean
ternaryReturn( String[] args ) {
return (args.length > 5) ? true
: false;
}
|
|
|
| RuleConditionalNegationIfCondition |
Recommendation |
Negation in if/else conditions makes the code more confusing.
Since if/else branches have statements for cases when the condition is
either true or false, there is no need to use negation.
|
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if ( !
Character.isDigit(args[i].charAt(0)))
{
System.out.println("Not a digit");
} else {
System.out.println("A digit");
}
}
}
|
|
|
- Remove the negation
- Switch if and else parts
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if
(Character.isDigit(args[i].charAt(0)))
{
System.out.println("A digit");
} else {
System.out.println("Not a digit");
}
}
}
|
|
|
| RuleConstructorsAbstractMethods |
Recommendation |
Calling an abstract method in the object constructor of an
abstract class is problematic because its subclass may not have been
properly initialized. Such calls may result in NullPointerException or
an incorrect object state.
|
public
static abstract
class Base implements Comparable {
public Base( Object
other ) {
super();
System.out.println( compareTo( other ) );
}
}
public
static final
class Derived extends Base {
public Derived( String
str, Object other ) {
super( other );
this .str
= str;
}
public int
compareTo( Object other ) {
return
str.compareTo( other.toString() );
}
private String
str;
}
public
static void
main( String[] args ) {
Derived derived = new Derived( "Hello
World!", "Hello World!" );
}
|
|
|
Remove the call to abstract
method from the constructor.
public
static abstract
class Base implements Comparable {
public Base() {}
}
public
static final
class Derived extends Base {
public Derived( String
str ) {
super();
this .str
= str;
}
public int
compareTo( Object other ) {
return
str.compareTo( other.toString() );
}
private String
str;
}
public
static void
main( String[] args ) {
Derived derived = new Derived( "Hello
World!" );
System.out.println(
derived.compareTo( "Hello World!" )
);
}
|
|
|
| RuleConstructorsOverridableMethods |
Recommendation |
Calling an overridable method in an object constructor is
problematic because the subclass may not have been properly
initialized. Such calls may result in NullPointerException or an
incorrect object state.
|
public
static class
Base {
public Base( Integer
value1, Integer value2 ) {
super();
this
.value1 = value1;
this
.value3 = getThirdValue();
this
.value2 = value2;
}
protected Integer
getThirdValue() {
return
value1;
}
protected Integer
value1;
protected Integer
value2;
protected Integer
value3;
}
public
static class
Derived extends Base {
public Derived( Integer
value1, Integer value2 ) {
super( value1, value2 );
}
protected Integer
getThirdValue() {
return new Integer( value2.intValue() * 10 );
}
}
public
static void
main( String[] args ) {
Derived derived = new Derived( new
Integer( 1 ), new Integer( 2
) );
}
|
|
|
Avoid complex computations in
constructors, use constructors only to initialize the fields of an
object.
public
static class
Base {
public Base( Integer
value1, Integer value2 ) {
this
.value1 = value1;
this
.value2 = value2;
}
public void
init() {
value3 = getThirdValue();
}
protected Integer
getThirdValue() {
return
value1;
}
protected Integer
value1;
protected Integer
value2;
protected Integer
value3;
}
public
static class
Derived extends Base {
public Derived( Integer
value1, Integer value2 ) {
super( value1, value2 );
}
protected Integer
getThirdValue() {
return new Integer( value2.intValue() * 10 );
}
}
public
static void
main( String[] args ) {
Derived derived = new Derived( new
Integer( 1 ), new Integer( 2
) );
derived.init();
}
|
|
|
| RuleDeclarationCArrayStyle |
Recommendation |
The C-style array declaration is uncommon and archaic in
Java. Avoid it because most Java programmers are not familiar with it.
|
public
static void
main( String[] args ) {
String copy[] = new String[ args.length ];
for ( int
i = 0; i < args.length; i++ ) {
copy[ i ] = args[ i ];
}
}
|
|
|
Use the Java standard for
declaring arrays.
public
static void
main( String[] args ) {
String[] copy = new String[ args.length ];
for ( int
i = 0; i < args.length; i++ ) {
copy[ i ] = args[ i ];
}
}
|
|
|
| RuleDeclarationMultipleDeclaration |
Recommendation |
Declaring several variables in the same statement makes the
code unclear. Such declaration leads to cumbersome initialization and
may increase the time needed to maintain the code.
|
public
static void
main( String[] args ) {
String var1 = null,
var2 = null;
var1 =
args[ 0 ];
var2 =
args[ 1 ];
}
private
int field1, field2;
|
|
|
Split the declaration into
separate declarations.
public
static void
main( String[] args ) {
String var1 = null;
String var2 = null;
var1 =
args[ 0 ];
var2 =
args[ 1 ];
}
private
int field1;
private
int field2;
|
|
|
| RuleDeclarationMixedDeclaration |
Recommendation |
Declaring basic and array variables in the same statement
makes the code harder to comprehend. Such declaration leads to
cumbersome initialization and may increase the time needed to maintain
the code.
|
public
static void
main( String[] args ) {
String notClear1 = null, notClear2[] = null;
notClear1
= args[ 0 ];
notClear2[
0 ] = args[ 1
];
}
|
|
|
Split the declaration of basic
and array variables.
public
static void
main( String[] args ) {
String notClear1 = null;
String[] notClear2 = null;
notClear1
= args[ 0 ];
notClear2[
0 ] = args[ 1
];
}
|
|
|
| RuleDeclarationDeclareConstants |
Recommendation |
Hard coding string literals is problematic for readability
and globalization. To improve the performance of the code and to
ensure that it is easy to read and make globalization compliant, avoid
declaring explicit string literals, especially duplicate ones.
|
public
static void
main(String[] args){
System.out.println(
"Hello" );
System.out.println(
"Bye" );
System.out.println(
"Hello" );
}
|
|
|
Declare constant
- Declare constant with a meaningful name to represent the
string literal.
- Replace all occurences of the string literal with
references to declared constant.
public
static void
main(String[] args) {
System.out.println(
HELLO );
System.out.println(
BYE );
System.out.println(
HELLO );
}
private
static final
String HELLO = "Hello";
private
static final
String BYE = "Bye";
|
|
|
| RuleDeclarationInterfaceConstants |
Recommendation |
Interfaces in Java provide a mechanism for behavioral
inheritance without implementation inheritance. While it is possible
to declare constants in an interface and then extend this interface,
this is not recommended. Constants should be declared in an interface
or a class to which they logically belong. All other classes and
interfaces should then refer to the constant declarations.
|
public
interface ICommonParsingConstants
{
public static
final int
IF_TOKEN = 1;
public static
final int
WHILE_TOKEN = 2;
public static
final String
END_OF_LINE = "\n";
}
public
class Parser implements ICommonParsingConstants {
public void
processToken( int iToken ) {
if (
iToken == IF_TOKEN ) {
System.out.println(
"if" );
} else if ( iToken == WHILE_TOKEN ) {
System.out.println(
"while" );
}
}
}
public
class Tokenizer implements ICommonParsingConstants {
public String
getToken( String str ) {
return (
END_OF_LINE.equals( str ) ? "" :
str );
}
}
|
|
|
- Move each constant to the interface or to the class to
which it logically belongs.
- Remove the empty interface.
public
class Parser {
public static
final int
IF_TOKEN = 1;
public static
final int
WHILE_TOKEN = 2;
public void
processToken( int iToken ) {
if (
iToken == IF_TOKEN ) {
System.out.println(
"if" );
} else if ( iToken == WHILE_TOKEN ) {
System.out.println(
"while" );
}
}
}
public
class Tokenizer {
public static
final String
END_OF_LINE = "\n";
public String
getToken( String str ) {
return (
END_OF_LINE.equals( str ) ? "" :
str );
}
}
|
|
|
| RuleExceptionsSpecificExceptions |
Recommendation |
For clarity it is better to maintain the complete list of
thrown exceptions in the throws clause.
|
public
class SpecificException extends Exception {
public SpecificException() {
super();
}
}
public
class Example {
public void
compute(Object subject) throws SpecificException {
if
(subject == null){
throw new
SpecificException();
}
}
public void
declaresException(Object o) throws Exception {
compute(o);
}
}
|
|
|
Declare all checked exceptions
in the throws clause.
public
class SpecificException extends Exception {
public SpecificException() {
super();
}
}
public
class Solution {
public void
compute(Object subject) throws SpecificException {
if
(subject == null) {
throw new
SpecificException();
}
}
public void
declaresException(Object o) throws SpecificException {
compute(o);
}
}
|
|
|
| RuleExceptionsCatchGeneric |
Recommendation |
Creating broad catch clauses that handle generic exceptions
such as java.lang.Exception or java.lang.Throwable exceptions may make
it difficult to correct the problem. Since exceptions in Java are
designed as regular classes, it is better to create and throw a
specific type of an exception and to have a dedicated catch clause for
each exception.
|
public
static class
Loader {
public void
load() throws
UnsupportedOperationException, ClassNotFoundException {}
}
public
static void
main(String[] args) {
Loader loader = new
Loader();
try {
loader.load();
} catch ( Exception
e ) {
e.printStackTrace();
}
}
|
|
|
Create a dedicated catch clause
for each exception that is declared in the throws clause.
public
static class
Loader {
public void
load() throws
UnsupportedOperationException, ClassNotFoundException {}
}
public
static void
main(String[] args) {
Loader loader = new
Loader();
try {
loader.load();
} catch ( UnsupportedOperationException
e1 ) {
System.out.println( "load
is not implemented" );
} catch ( ClassNotFoundException
e2 ) {
System.out.println( "No
class " + e2.getMessage() );
}
}
|
|
|
| RuleExceptionsExtendError |
Recommendation |
java.lang.Error is a runtime exception, primarily meant to be
extended and used for signaling runtime problems encountered by the
Java environment. The runtime exceptions are unchecked, that is the
compiler will not require the user of the interface to handle it.
Generally, it is preferable to create the checked exception instead of
the unchecked ones.
|
public
static class
ErrorSubclass extends Error{
public ErrorSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() {
throw new ErrorSubclass( "Problem" );
}
}
|
|
|
Extend java.lang.Exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleExceptionsExtendRuntimeException |
Recommendation |
java.lang.RuntimeException is a runtime exception, primarily
meant to be extended and used for signaling runtime problems
encountered by the Java environment. The runtime exceptions are
unchecked, that is the compiler will not require the user of the
interface to handle it. Generally, it is preferable to create the
checked exception instead of the unchecked ones.
|
public
static class
RuntimeExceptionSubclass extends
RuntimeException {
public RuntimeExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() {
throw new RuntimeExceptionSubclass( "Problem" );
}
}
|
|
|
Extend java.lang.Exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleExceptionsExtendThrowable |
Recommendation |
java.lang.Throwable is a common base class for both checked
and unchecked exceptions in Java. This class is not meant to be
extended directly. It is preferable to create the checked
exceptionthat descends from java.lang.Exception.
|
public
static class
ThrowableSubclass extends
Throwable {
public ThrowableSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ThrowableSubclass {
throw new ThrowableSubclass( "Problem" );
}
}
|
|
|
Extend java.lang.Exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleExceptionsThrowError |
Recommendation |
java.lang.Error is a runtime exception, primarily meant to be
extended and used for signaling runtime problems encountered by the
Java environment. The runtime exceptions are unchecked, that is the
compiler will not require the user of the interface to handle it.
Generally, it is preferable to throw checked exceptions instead of the
unchecked ones.
|
public
class Example {
public void
run() {
throw new Error( "Problem"
);
}
}
|
|
|
- Extend the java.lang.Exception.
- Change the throws clause to use this new exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleExceptionsThrowException |
Recommendation |
java.lang.Exception is a generic checked exception in Java.
Since exceptions in Java are designed as regular classes, it is better
to create and throw specific sub classes of this generic class and to
have a dedicated catch clause for each exception.
|
public
class Example {
public void
run() throws Exception {
throw new Exception( "Problem"
);
}
}
|
|
|
Create the specific subclass of
java.lang.Exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleExceptionsThrowThrowable |
Recommendation |
java.lang.Throwable is a common base class for both checked
and unchecked exceptions in Java. This class is not meant to be used
directly in the throws clause. It is preferable to create and use
specific checked exceptions thfrom java.lang.Exception.
|
public
class Example {
public void
run() throws Throwable {
throw new Throwable( "Problem"
);
}
}
|
|
|
Create specific subclass of java.lang.Exception.
public
static class
ExceptionSubclass extends
Exception {
public ExceptionSubclass( String s ){
super(s);
}
}
public
class Example {
public void
run() throws ExceptionSubclass {
throw new ExceptionSubclass( "Problem" );
}
}
|
|
|
| RuleInitializationInitializeStaticFields |
Recommendation |
Uninitialzed static fields may lead to problems such as
NullPointerException. Also, when a field is not explicitely
initialized, someone reading the code may not know the default value
and may have difficulty understanding the code.
|
public
interface IHandler {
public void
process( Object o );
}
public
static void
registerHandler( Class c, IHandler handler ) {
handlers.put(
c, handler );
}
public
static IHandler
getHandler( Class c ) {
return (IHandler)handlers.get(
c );
}
private
InitializeStaticFields_Example() {
super();
}
private
static Map
handlers;
public
static void
main(String[] args){
IHandler handler = new IHandler() {
public void process( Object
o ) {
System.out.println(
String.valueOf( o ) );
}
};
InitializeStaticFields_Example.registerHandler(
Boolean.class, handler );
InitializeStaticFields_Example.getHandler(
Boolean.class ).process(
Boolean.TRUE );
}
|
|
|
Always initilize static fields.
public
interface IHandler {
public void
process( Object o );
}
public
static void
registerHandler( Class c, IHandler handler ) {
handlers.put(
c, handler );
}
public
static IHandler
getHandler( Class c ) {
return (IHandler)handlers.get(
c );
}
private
InitializeStaticFields_Solution() {
super();
}
private
static Map
handlers = new HashMap(10);
public
static void
main(String[] args){
IHandler handler = new IHandler() {
public void process( Object
o ) {
System.out.println(
String.valueOf( o ) );
}
};
InitializeStaticFields_Solution.registerHandler(
Boolean.class, handler );
InitializeStaticFields_Solution.getHandler(
Boolean.class ).process(
Boolean.TRUE );
}
|
|
|
| RuleInitializationChainingAssignments |
Recommendation |
Chaining assignment operators introduces the potential for
errors and makes the code unclear. The code is more robust and
self-explanatory if the assignments are separated.
|
public
static void
main( String[] args ) {
int x = 0;
int y = 0;
x = y = 10;
System.out.println(
x + " " + y );
}
|
|
|
Assign the variables in separate
statements.
public
static void
main( String[] args ) {
int x = 0;
int y = 0;
x = 10;
y = 10;
System.out.println(
x + " " + y );
}
|
|
|
| RuleInitializationAssigningMethodParameter |
Recommendation |
Changing the value of a method parameter alters its original
intent and makes the code unclear.
|
public
static void
main( String[] args ) {
loop(
Integer.parseInt( args[ 0 ] ) );
}
private
static void
loop( int iMax ) {
iMax =
Math.max( iMax, 20 );
for ( int
i = 0; i < iMax; ) {
System.out.println( i );
}
}
|
|
|
Introduce another variable
instead of changing the value of a method parameter.
public
static void
main( String[] args ) {
loop(
Integer.parseInt( args[ 0 ] ) );
}
private
static void
loop( int iMax ) {
int iBoundedMax = Math.max( iMax, 20 );
for ( int
i = 0; i < iBoundedMax; ) {
System.out.println( i );
}
}
|
|
|
| RuleLoopNoConditionForLoop |
Recommendation |
A for loop without a condition may lead to an infinite loop.
Such for loops are also more difficult to understand and maintain.
|
public
static void
main( String[] args ) {
for ( int
i = 0; ; i++ ) {
System.out.println( args[ i ] );
if ( i
< args.length ) {
break ;
}
}
}
|
|
|
Insert a condition into the for loop.
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
System.out.println( args[ i ] );
}
}
|
|
|
| RuleLoopNoInitIncrForLoop |
Recommendation |
A for loop without an initializer and an incrementor is
misleading and error-prone. Besides hiding the intend, the loop may
lead to 'infinite loop' problems.
|
public
static void
main( String[] args ) {
int i = 0;
for ( ; i < args.length ; ) {
System.out.println( args[ i ] );
i++;
}
}
|
|
|
Replace the for loop with a while loop.
public
static void
main( String[] args ) {
int i = 0;
while ( i < args.length ) {
System.out.println( args[ i ] );
i++;
}
}
|
|
|
| RuleLoopContinueStatement |
Recommendation |
Continue statements add additional, unnecessary complexity to
the body of a loop. Continue statements typically occur in long loops
with non-trivial logic. Such loops need to be refactored into smaller,
more clear calculations. After performing the refactoring, it will be
possible to replace the continue statement with an if statement
without complicating the code.
|
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if ( i
< 5 ) {
continue ;
}
System.out.println( args[ i ] );
}
}
|
|
|
- While continue is part of
a long loop, refactor the loop using Extract Method refactoring
- Negate the condition before the continue
statement
- Move the code below the continue
statement to inside of the if
block
public
static void
main( String[] args ) {
for ( int
i = 0; i < args.length; i++ ) {
if ( i
>= 5 ) {
System.out.println(
args[ i ] );
}
}
}
|
|
|
| RuleReflectionGetDeclaredField |
Recommendation |
Java reflection facilities are designed for generic
processing of class content. For example, the reflection is used by
Java Beans framework to enable generic inspection of properties of
Java Beans. The reflection is not meant to circumvent the standard
java field or method access. Thus, calling getDeclaredField() with the
hard coded name of the field is not recommended.
|
public
GetDeclaredField_Example() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
try {
Field field =
GetDeclaredField_Example.class.getDeclaredField(
"value" );
GetDeclaredField_Example
obj = new
GetDeclaredField_Example();
field.set( obj, new
Integer( 1 ) );
System.out.println( obj.getValue() );
} catch (SecurityException
e) {
System.out.println( "Can't
access field 'value'" );
} catch (NoSuchFieldException
e) {
System.out.println( "No
field 'value'" );
} catch (IllegalArgumentException
e) {
System.out.println( e.getMessage() );
} catch (IllegalAccessException
e) {
System.out.println( "Can't
access private field 'value'" );
}
}
|
|
|
Use direct field access instead
of getDeclaredField() with the hard coded name of the field.
public
GetDeclaredField_Solution() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
GetDeclaredField_Example obj = new GetDeclaredField_Example();
obj.setValue(
1 );
System.out.println(
obj.getValue() );
}
|
|
|
| RuleReflectionGetDeclaredMethod |
Recommendation |
Java reflection facilities are designed for generic
processing of class content. For example, the reflection is used by
Java Beans framework to enable generic inspection of properties of
Java Beans. The reflection is not meant to circumvent the standard
java field or method access. Thus, calling getDeclaredMethod() with
the hard coded name of the method is not desirable.
|
public
GetDeclaredMethod_Example() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
try {
Method method
= GetDeclaredMethod_Example.class.getDeclaredMethod(
"getValue", new Class[] { Void.class
} );
GetDeclaredMethod_Example
obj = new
GetDeclaredMethod_Example();
method.invoke( obj, new Object[] { new
Integer( 1 ) } );
System.out.println( obj.getValue() );
} catch (IllegalAccessException
e) {
System.out.println( "Can't
access private method 'getValue'" );
} catch (InvocationTargetException
e) {
System.out.println( "Problem
calling method" );
} catch (NoSuchMethodException
e) {
System.out.println( "No
method getValue" );
}
}
|
|
|
Use direct method call instead
of getDeclaredMethod() with the hard coded name of the method.
public
GetDeclaredMethod_Solution() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args) {
GetDeclaredMethod_Example obj = new GetDeclaredMethod_Example();
obj.setValue(
1 );
System.out.println(
obj.getValue() );
}
|
|
|
| RuleReflectionGetField |
Recommendation |
Java reflection facilities are designed for generic
processing of class content. For example, the reflection is used by
Java Beans framework to enable generic inspection of properties of
Java Beans. The reflection is not meant to circumvent the standard
java field or method access. Thus, calling getField() with the hard
coded name of the field is not desirable.
|
public
GetField_Example() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
try {
Field field =
GetField_Example.class.getField( "value" );
GetField_Example
obj = new GetField_Example();
field.set( obj, new
Integer( 1 ) );
System.out.println( obj.getValue() );
} catch (SecurityException
e) {
System.out.println( "Can't
access field 'value'" );
} catch (NoSuchFieldException
e) {
System.out.println( "No
field 'value'" );
} catch (IllegalArgumentException
e) {
System.out.println( e.getMessage() );
} catch (IllegalAccessException
e) {
System.out.println( "Can't
access private field 'value'" );
}
}
|
|
|
Use direct field access instead
of getField() with the hard coded name of the field.
public
GetField_Solution() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
GetField_Solution obj = new GetField_Solution();
obj.setValue(
1 );
System.out.println(
obj.getValue() );
}
|
|
|
| RuleReflectionGetMethod |
Recommendation |
Java reflection facilities are designed for generic
processing of class content. For example, the reflection is used by
Java Beans framework to enable generic inspection of properties of
Java Beans. The reflection is not meant to circumvent the standard
java field or method access. Thus, calling getMethod() with the hard
coded name of the method is not desirable.
|
public
GetMethod_Example() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args){
try {
Method method
= GetMethod_Example.class.getMethod(
"getValue", new Class[] { Void.class
} );
GetMethod_Example
obj = new GetMethod_Example();
method.invoke( obj, new Object[] { new
Integer( 1 ) } );
System.out.println( obj.getValue() );
} catch (IllegalAccessException
e) {
System.out.println( "Can't
access private method 'getValue'" );
} catch (InvocationTargetException
e) {
System.out.println( "Problem
calling method" );
} catch (NoSuchMethodException
e) {
System.out.println( "No
method getValue" );
}
}
|
|
|
Use direct method call instead
of getMethod with the hard coded name of the method.
public
GetMethod_Solution() {
super();
}
public
void setValue( int value ) {
this .value = value;
}
public
int getValue() {
return value;
}
private
int value;
public
static void
main(String[] args) {
GetMethod_Solution obj = new GetMethod_Solution();
obj.setValue(
1 );
System.out.println(
obj.getValue() );
}
|
|
|
| RuleSerializationSerialVersionUID |
Recommendation |
The serialVersionUID field is used by Java serialization to
ensure correct deserialization of old objects when the underlying
class is altered. Failing to declare the serialVersionUID inside of a
serializable class will result in the IOException when attempting to
load old instances of this class.
|
public
class SerialVersionUID implements Serializable {
public SerialVersionUID() {
super();
}
public String
getString(){
return
field;
}
public void
setString(String field){
this
.field = field;
}
private String
field;
}
|
|
|
Declare static final
serialVersionUID field.
public
class SerialVersionUID implements Serializable {
public SerialVersionUID() {
super();
}
public String
getString(){
return
field;
}
public void
setString(String field){
this
.field = field;
}
private String
field;
static final
long serialVersionUID = 123;
}
|
|
|
| RuleSerializationTransientField |
Recommendation |
The transient keyword is used in Java to denote the
non-serializable fields inside of serializable classes. Using this
keyword inside of non-serializable classes makes the code unclear and
cluttered.
|
public static
final class
SomeType {}
public
class SerializableField implements Serializable {
public
SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private SomeType
nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
Make the field type
serializable.
public
static final
class SomeType implements Serializable {}
public
class SerializableField()
implements Serializable {
public SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private SomeType
nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
Declare the field transient.
public
static final
class SomeType {}
public
class SerializableField implements Serializable {
public SerializableField( String str ) {
super();
this .str
= str;
nonser = new
SomeType();
}
private String
str;
private transient
SomeType nonser;
private static
final long
serialVersionUID = 123;
}
|
|
|
| RuleStatementSurroundWithBraces |
Recommendation |
An unsurrounded if or loop statement introduces ambiguity and
the possibility of an error into the code. For example, if a line
below the statement is removed, then the next line below it becomes
part of the statement, which may not be the intent. Also, statements
without curly braces rely on indentation to communicate the meaning.
It is better to be explicit and demonstrate the intent by surrounding
all such statements with curly braces.
|
public
static void
main( String[] args ) {
surroundIf(
args );
surroundLoop(
args );
}
private
static void
surroundIf(String[] args) {
if ( args.length == 0 )
System.out.println( "No
arguments" );
else
System.out.println( args.length + " arguments");
System.out.println( args[ 0 ] );
}
private
static void
surroundLoop( String[] args ) {
for ( int
i = 0; i < args.length; i++ )
System.out.println(args[i]);
System.out.println(
"Done" );
}
|
|
|
Surround if and loop
statements with curly braces.
|
|
| RuleStatementEmptyStatement |
Recommendation |
An empty statement is an extraneous piece of code. Such
statements make code more difficult to understand and maintain. Empty
statements typically arise as a result of accidental code deletions.
|
public
static void
main( String[] args ) {
emptyIf(
args );
emptyLoop(
args );
}
private
static void
emptyIf( String[] args ) {
if ( args.length == 0 ){
}else {
for ( int i = 0;
i < args.length; i++ ) {
if ( i > 0
);
System.out.println(
args[ i ] );
}
}
}
private
static void
emptyLoop( String[] args ) {
int i = 0;
while ( i < args.length );
System.out.println(
args[ i++ ] );
for ( int
j = 0; j < args.length; j++ ){}
}
|
|
|
Enclude the statements below
into the loop or if statement.
public
static void
main( String[] args ) {
emptyIf(
args );
emptyLoop(
args );
}
private
static void
emptyIf( String[] args ) {
if ( args.length > 0 ){
for ( int i = 0;
i < args.length; i++ ) {
if ( i > 1
) {
System.out.println( args[ i ] );
}
}
}
}
private
static void
emptyLoop( String[] args ) {
int i = 0;
while ( i < args.length ){
System.out.println( args[ i++ ] );
}
for ( int
j = 0; j < args.length; j++ ){
System.out.println( args[ j++ ] );
}
}
|
|
|
| RuleSwitchDefaultLabel |
Recommendation |
A missing default label in a switch statement makes the code
incomplete because the intended behavior is not clear. Providing the
default label results in more explicit and robust code.
|
public
static final
int YESTERDAY =
0;
public
static final
int TODAY = 1;
public
static final
int TOMORROW =
2;
public
static void
main( String[] args ) {
int dateType = Integer.parseInt( args[ 0 ] );
Calendar calendar =
Calendar.getInstance();
switch ( dateType ) {
case 0:
calendar.add(
Calendar.DATE, -1 );
break ;
case 1:
break ;
case 2:
calendar.add(
Calendar.DATE, 1 );
break ;
}
System.out.println(
calendar.getTime() );
}
|
|
|
Add the default label to the switch statement.
public
static final
int YESTERDAY =
0;
public
static final
int TODAY = 1;
public
static final
int TOMORROW =
2;
public
static void
main( String[] args ) {
int dateType = Integer.parseInt( args[ 0 ] );
Calendar calendar =
Calendar.getInstance();
switch ( dateType ) {
case 0:
calendar.add(
Calendar.DATE, -1 );
break ;
case 1:
break ;
case 2:
calendar.add(
Calendar.DATE, 1 );
break ;
default :
break ;
}
System.out.println(
calendar.getTime() );
}
|
|
|
| RuleSwitchBranch |
Recommendation |
The switch statement is meant to be used to avoid a lot of
nested if/else constructs. However, when there are just two choices,
using a switch statement is unnecessary and unclear.
|
public
static void
main(String[] args){
switch ( args.length ) {
case 0:
System.out.println("No parameters");
break ;
default :
System.out.println("Parameters: ");
for ( int
i = 0, n = args.length; i < n; i
++){
System.out.println(args[i]);
}
break ;
}
}
|
|
|
Use if/else instead.
public
static void
main(String[] args){
if ( args.length == 0 ) {
System.out.println("No
Paramaters");
} else {
System.out.println("Parameters:
");
for ( int i = 0,
n = args.length; i < n; i ++){
System.out.println(args[i]);
}
}
}
|
|
|
| yield |
Recommendation |
Thread.yield() is meant to suspend the execution of the
current thread and let the other active threads run. Yield is
equivalent to Thread.sleep( 0 ). Even though the yield() is guaranteed
to work, there is no guarantee that another thread will not be
suspended again. This is because the task intended by yield() call may
not be fully accomplished before the control is returned to the
yielding thread. When designing threading behavior in complex,
large-scale programs it is not recommended to use yield to ensure the
order of thread execution. The lock-based wait()/notify() approach is
preferred instead.
|
public
static void
main( String[] args ){
Data data = new
Data();
final Consumer
consumer = new Consumer( data );
final Producer
producer = new Producer( data );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Data {
public Data() {
super();
}
public int
getValue() {
return
value;
}
public void
setValue( int value ) {
this
.value = value;
}
private int
value = 0;
}
public
static final
class Consumer {
public Consumer( Data data ) {
this .data
= data;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
System.out.println( "Consumer:
yielding..." );
Thread.yield();
System.out.println( "Consumer:
value is " + data.getValue() );
System.out.println( "Consumer:
exiting" );
}
private Data
data;
}
public
static final
class Producer {
public Producer( Data data ) {
this .data
= data;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
try {
Thread.sleep( 3000 ); } catch (InterruptedException
e) {}
data.setValue( 1
);System.out.println( "Producer: exiting" );
}
private Data
data;
}
|
|
|
Use wait()/notify() instead of
yield().
public
static void
main( String[] args ){
Data data = new
Data();
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( data,
lock );
final Producer
producer = new Producer( data,
lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Data {
public Data() {
super();
}
public int
getValue() {
return
value;
}
public void
setValue( int value ) {
this
.value = value;
}
private int
value = 0;
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Data data, Lock
lock ) {
this .data
= data;
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
System.out.println( "Consumer:
waiting..." );
synchronized
( lock ) {
try {
lock.wait();
} catch (InterruptedException
e) {}
}
System.out.println( "Consumer:
value is " + data.getValue() );
System.out.println( "Consumer:
exiting" );
}
private Data
data;
private Lock
lock;
}
public
static final
class Producer {
public Producer( Data data, Lock
lock ) {
this .data
= data;
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
synchronized
( lock ) {
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
data.setValue(
1 );System.out.println(
"Producer: exiting" );
lock.unlock();
lock.notifyAll();
}
}
private Data
data;
private Lock
lock;
}
|
|
|
| extendthread |
Recommendation |
Extending Thread vs. implementing Runnable problem is an
example of the Inheritance vs. Delegation Principle. In most cases,
delegation is preferred to inheritance. Here as well, there is no good
reason to extend Thread class. Instead, it is cleaner to implement
Runnable interface and instantiate Thread to run it.
|
public
static class
ICThread extends Thread {
public void
run() {
System.out.println(
System.currentTimeMillis() );
}
}
public
static void
main( String[] args ) {
(new ICThread()).start();
}
|
|
|
Implement Runnable
instead of extending Thread.
public
static class
ICRunnable implements Runnable {
public void
run() {
System.out.println(
System.currentTimeMillis() );
}
}
public
static void
main( String[] args ) {
(new Thread( new
ICRunnable() )).start();
}
|
|
|
| synchronizedthis |
Recommendation |
Synchronization problems are difficult to identify and debug,
so it is critical to consider what exactly is being locked and why.
Creating separate locks to protect different data sets is a good
practice that will lead to improved performance and clarity. Using
this as a lock is not recommended because it is also used as an
implicit lock when the method is declared synchronized.
|
public
int getNumber() {
synchronized ( this ) {
try {
Thread.sleep( 3000 ); } catch (InterruptedException
e) {}
return
number;
}
}
public
String getString() {
synchronized ( this ) {
return
str;
}
}
private
String str = "Hello
world!";
private
int number = 10;
public
static void
main( String[] args ) {
final SynchronizedThis_Example
data = new
SynchronizedThis_Example();
Runnable numberReader = new Runnable() {
public void run() {
System.out.println(
"Getting number..." );
System.out.println(
data.getNumber() );
}
};
Runnable stringReader = new Runnable() {
public void run() {
System.out.println(
"Getting string..." );
System.out.println(
data.getString() );
}
};
(new Thread( numberReader ) ).start();
(new Thread( stringReader ) ).start();
}
|
|
|
Declare an explicit separate
lock to protect different data sets.
public
int getNumber() {
synchronized ( numberLock ) {
try {
Thread.sleep( 3000 ); } catch (InterruptedException
e) {}
return
number;
}
}
public
String getString() {
synchronized ( stringLock ) {
return
str;
}
}
private
String str = "Hello
world!";
private
int number = 10;
private
Object numberLock = new Object();
private
Object stringLock = new Object();
public
static void
main( String[] args ) {
final SynchronizedThis_Solution
data = new
SynchronizedThis_Solution();
Runnable numberReader = new Runnable() {
public void run() {
System.out.println(
"Getting number..." );
System.out.println(
data.getNumber() );
}
};
Runnable stringReader = new Runnable() {
public void run() {
System.out.println(
"Getting string..." );
System.out.println(
data.getString() );
}
};
(new Thread( numberReader ) ).start();
(new Thread( stringReader ) ).start();
}
|
|
|
| notifyall |
Recommendation |
The difference between java.lang.Object.notify() and
java.lang.Object.notifyAll() is that the first method tells JVM to
inform the first thread that is waiting on a lock, while the second
tells JVM to inform all the threads that are waiting on a lock. Since
the number of waiting threads is often difficult to anticipate, it is
recommended to call notifyAll() instead of notify().
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer"
+ id + ": waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notify();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Call notifyAll() instead of
notify()
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer"
+ id + ": waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| waitoutsideloop |
Recommendation |
The call to wait() suspends the execution of the calling
thread until another thread notifies it that the intended state of the
program is achieved. The wait() should occur based on the termination
condition. It is highly recommended that the wait() call is made
inside of the while loop, because another thread may invoke notify
even if the condition on which the current thread is waiting is not
met
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
if (
!lock.isAvailable() ) {
System.out.println(
"Consumer" + id + ": waiting..." );
synchronized ( lock ) {
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
synchronized
( lock ) {
lock.notifyAll();
}
synchronized
( lock ) {
System.out.println(
"Producer: unlocking..." );
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Place wait() inside of a while
loop.
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer1 = new Consumer( lock, 1 );
final Consumer
consumer2 = new Consumer( lock, 2 );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer1.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
consumer2.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock, int
id ) {
this .lock
= lock;
this .id =
id;
}
public void
consume() {
System.out.println( "Consumer
" + id + ": consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer"
+ id + ": waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer"
+ id + ": exiting" );
}
private Lock
lock;
private int
id;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
synchronized
( lock ) {
lock.notifyAll();
}
synchronized
( lock ) {
System.out.println(
"Producer: unlocking..." );
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
| synchronizedmethod |
Recommendation |
Synchronized method implicitly uses 'this' as a lock to
prevent other methods and blocks with the same lock from parallel
execution. There are several reasons why this is undesirable. Most
important is that since synchronized blocks are computationally
expensive, it is desirable to place as less code as possible inside of
these blocks. If the method that is declared as synchronized is
operating on more data that needs to be protected then such method is
unnecessarily expensive. Another reason is that some programmers are
not aware that 'this' is used as a lock when method is declared
synchronized, so it is better to be explicit
|
public
synchronized int getNumber() {
try { Thread.sleep( 3000 ); } catch
(InterruptedException e) {}
return number;
}
public
synchronized String getString() {
return str;
}
private
String str = "Hello
world!";
private
int number = 10;
public
static void
main( String[] args ) {
final SynchronizedMethod_Example
data = new
SynchronizedMethod_Example();
Runnable numberReader = new Runnable() {
public void run() {
System.out.println(
"Getting number..." );
System.out.println(
data.getNumber() );
}
};
Runnable stringReader = new Runnable() {
public void run() {
System.out.println(
"Getting string..." );
System.out.println(
data.getString() );
}
};
(new Thread( numberReader ) ).start();
(new Thread( stringReader ) ).start();
}
|
|
|
Create synchronized block inside
of the method that only locks necessary statements.
public
int getNumber() {
synchronized ( numberLock ) {
try {
Thread.sleep( 3000 ); } catch (InterruptedException
e) {}
return
number;
}
}
public
String getString() {
synchronized ( stringLock ) {
return
str;
}
}
private
String str = "Hello
world!";
private
int number = 10;
private
Object numberLock = new Object();
private
Object stringLock = new Object();
public
static void
main( String[] args ) {
final SynchronizedMethod_Solution
data = new
SynchronizedMethod_Solution();
Runnable numberReader = new Runnable() {
public void run() {
System.out.println(
"Getting number..." );
System.out.println(
data.getNumber() );
}
};
Runnable stringReader = new Runnable() {
public void run() {
System.out.println(
"Getting string..." );
System.out.println(
data.getString() );
}
};
(new Thread( numberReader ) ).start();
(new Thread( stringReader ) ).start();
}
|
|
|
| whilesleep |
Recommendation |
The sleep() call causes the currently executing thread to
cease execution for the specified number of milliseconds. The thread
does not, however, loose the ownership of any of its monitors (locks).
Calling sleep inside of the while loop is known as a busy-wait, since
it does not allow other threads to operate on the shared data.
|
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
while (
!lock.isAvailable() ) {
System.out.println(
"Consumer: sleeping..." );
try { Thread.sleep( 1000 ); } catch
(InterruptedException e) {}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
try {
Thread.sleep( 5000 ); } catch (InterruptedException
e) {}
System.out.println( "Producer:
unlocking..." );
lock.unlock();
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|
Change while()/sleep() to
wait()/notify().
public
static void
main( String[] args ){
Lock lock = new
Lock();
final Consumer
consumer = new Consumer( lock );
final Producer
producer = new Producer( lock );
(new Thread( new
Runnable() {
public void run() {
consumer.consume();
}
})).start();
(new Thread( new
Runnable() {
public void run() {
producer.produce();
}
})).start();
}
public
static final
class Lock {
public Lock() {
super();
lock();
}
public boolean
isAvailable() {
return
available;
}
public void
lock() {
available = false;
}
public void
unlock() {
available = true;
}
private boolean
available = false;
}
public
static final
class Consumer {
public Consumer( Lock lock ) {
this .lock
= lock;
}
public void
consume() {
System.out.println( "Consumer:
consume was called" );
synchronized
( lock ) {
while ( !lock.isAvailable() ) {
System.out.println( "Consumer:
waiting..." );
try {
lock.wait();
} catch (InterruptedException e) {}
}
}
System.out.println( "Consumer:
exiting" );
}
private Lock
lock;
}
public
static final
class Producer {
public Producer( Lock lock ) {
this .lock
= lock;
}
public void
produce() {
System.out.println( "Producer:
produce was called" );
System.out.println( "Producer:
unlocking..." );
synchronized
( lock ) {
lock.unlock();
lock.notifyAll();
}
System.out.println( "Producer:
exiting" );
}
private Lock
lock;
}
|
|
|