1.
public static String removeBlankAndDot(String target)
{
if (target != null)
{
int len = target.length(); // move this out of loop, more efficient
StringBuffer buf = new StringBuffer(len);
// using known size to create StringBuffer, more efficient, default size is 16 char.
// If the len > 16, incorrect design will force StringBuffer to resize.
// Using pre-size Collections is more efficient, for example:
// faster, assuming expecting 1000 - 2000 elements:
// Map m = new HashMap (1439);
// Collections such as HashMap also have resizing problem when designing not properly.
// if no need for thread safe, use StringBuilder instead of StringBuffer
for (int ii=0;ii<len;ii++)
{
char c = target.charAt(ii);
if (c != ' ' && c != '.') buf.append(c);
}
return buf.toString();
}
return null;
}
2.
public class DataLogger {
private static PrintStream err = System.err;
private static String filePath = "log";
private static String fileName = "my.data";
private static PrintWriter log;
public static void logData(String logInfo) {
try {
if (log == null) {
File file = new File(filePath + System.getProperty("file.separator") + fileName);
log = new PrintWriter(new FileOutputStream(file));
}
if (logInfo.length() != 0) { // using logInfo.length() is more efficient
log.println(logInfo);
log.flush();
}
}
catch (Exception e) {
e.printStackTrace(err);
throw new RuntimeException(e);
// avoid using System.exit(1) in methods other than main()
}
}
3.
public class Hello {
public static void main(String[] args) {
String name = "John";
char[] numbers = { '1', '0' };
System.out.println(name + " is the number " + String.valueOf(numbers));
//in incorrect design, numbers will invoke Object.toString(), it is not what you want
}
}
4.
public class Hello {
public static void main(String[] args) {
Date d1 = new Date("1 Apr 98");
nextDateUpdate(d1);
System.out.println("NextDay: " + d1);
}
private static void nextDateUpdate(Date arg) {
arg.setDate(arg.getDate() + 1);
}
}
// incorrect design will print NextDay: 1 Apr 98
// and the new Date(...) is being assigned to a local object reference,
// not the original object
5.
// double values[100] initialized here
double getValueForPeriod(int periodNumber) {
if (periodNumber >= values.length) {
return 0.0;
}
return values[periodNumber];
}
// incorrect design uses unnecessary Exception; Exceptions are linchpin for
// application but are expensive, should avoid it as possible as we can.
6.
static void copy(String src, String dest) throws IOException {
InputStream in = null;
OutputStream out = null;
try {
in = new FileInputStream(src);
out = new FileOutputStream(dest);
byte[] buf = new byte[1024];
int n;
while ((n = in.read(buf)) >= 0) {
out.write(buf, 0, n);
}
} catch (Exception e) {
e.printStackTrace(err);
return;
} finally {
if (in != null) {
try {
in.close();
} catch (IOException e) {
e.printStackTrace(err); // cannot put return, throw, etc. in finally block
// because it violates the design logic of Exception.
// Finally block is only used for closing open file and
// closing db connection, etc.
}
}
if (out != null) {
try {
out.close();
} catch (IOException e) {
e.printStackTrace(err);
}
}
// Use separate try-catch to avoid potential resource leak (in case in.close() fails)
}
}
7.
public class Timer {
private static final int CURRENT_YEAR =
Calendar.getInstance().get(Calendar.YEAR);
public static final Timer INSTANCE = new Timer();
private final int diffTime;
private Timer() {
diffTime = CURRENT_YEAR - 1949;
}
public int diffTime() {
return diffTime;
}
public static void main(String[] args) {
System.out.println("diff Time is " + INSTANCE.diffTime());
}
}
// incorrect design has static field initialized order problem
8.
public String getStrTrim () {
String string = " 14 units "; // avoid using new String() inside method
// it is not necessary to create a new String instance each time.
// here it uses a single String instance
// The String class is the only class that can be instantiated without
// using the new key word.
String str = string.trim();
// string is immutable, you have to assign to another String
return str;
}
9.
public class PingPong {
public static synchronized void main(String[] a) {
Thread t = new Thread() {
public void run() { pong(); }
};
t.start(); // should use start(), because you need: "Ping" "Pong"
System.out.print("Ping");
}
static synchronized void pong() {
System.out.print("Pong");
}
}
10.
public class Stack {
private Object[] elements;
private int size = 0;
public Stack(int initialCapacity) {
this.elements = new Object[initialCapacity];
}
public void push(Object e) {
ensureCapacity();
elements[size++] = e;
}
public Object pop() {
if (size == 0) throw new EmptyStackException();
Object result = elements[--size];
elements[size] = null;
// should eliminate obsolete reference to avoid memory leak
return result;
}
}
11.
private static Date START = new Date("1 Apr 98");
private static Date END = new Date("1 Apr 99");
public boolean isYourTime(Date d) {
return d.compareTo(START) >= 0 &&
d.compareTo(END) < 0;
}
// should move constant out of method, and to be static
12.
public final class Period {
private final Date start;
private final Date end;
public Period(Date start, Date end) {
this.start = new Date(start.getTime());
this.end = new Date(end.getTime());
// verify start >= end
}
public Date getStart() {
return new Date(start.getTime()); // it is defensive copy
}
}
// incorrect design relates private objects with outside objects
// in following code it will cause problem:
// Date start = new Date();
// Date end = new Date();
// Period p = new Period(start, end);
// start.setYear(98);
// p.getStart().setYear(98);
13.
publish Map test(Map ht) {
// ...
Map newHt = new Hashtable();
// ...
return newHt; // Better to use interface instead of class
// for future new implementation
}
14.
Good OO design: need polymorphism if you do the same thing
in different ways. Such as select() // select from different tables
But here process() is to do different thing, it's wrong design.
15.
private List cheesesInStock = ...
public Cheese[] getCheeses() {
if (cheesesInStock.size() == 0) {
return new Cheese[0]; // should return zero-size array
}
}
16.
public abstract class WorkQueue {
private final List queue = new LinkedList();
protected WorkQueue() { new WorkerThread().start(); }
public final void enqueue(Object workItem) {
synchronized(queue) {
queue.add(workItem);
queue.notify();
}
}
protected abstract void processItem(Object workItem)
throws InterruptedException;
private class WorkerThread extends Thread {
public void run() {
while (true) {
synchronized(queue) {
// ...
Object workItem = queue.remove(0);
}
try {
processItem(workItem);
// should move it out of synchronized field to avoid deadlock
} catch (InterruptedException e) {
return;
}
}
}
}
}
class UseQueue extends WorkQueue {
protected void processItem(Object workItem)
throws InterruptedException {
Thread child = new Thread() {
public void run() { enqueue(workItem); }
};
child.start();
child.join();
}
}
17.
// Use interfaces, PowerSwitchable, to define functionality in unrelated classes
public class ComputerAsset extends Asset {
// ...
}
public class ComputerServer extends ComputerAsset {
// ...
}
public class BuildingAsset extends Asset {
// ...
}
public class BuildingLight extends BuildingAsset {
// ...
}
public class EmergencyLight extends BuildingLight {
// ...
}
public interface PowerSwitchable {
public void powerDown();
}
public class ComputerMonitor extends ComputerAsset implements PowerSwitchable {
// ...
public powerDown() {
// need do something
}
}
public class RoomLights extends BuildingLight implements PowerSwitchable {
// ...
public powerDown() {
// need do something
}
}
public class BuildingManagement {
Asset things[] = new Asset[24];
int numItems = 0;
public void goodNight() {
for (int i = 0; i < things.length; i++) {
if (things[i] instanceof PowerSwitchable) {
((PowerSwitchable)things[i]).powerDown();
}
}
}
public void add(Asset thing) {
// ...
}
}
public static void main(String[] args) {
BuildingManagement b1 = new BuildingManagement();
b1.add(new RoomLights(101));
b1.add(new EmergencyLight(100));
// ...
b1.add(new ComputerMonitor(200));
b1.goodNight();
}
18.
public int loadData(Connection conn, String sql) throws SQLException {
// ...
ResultSet rs = null;
Object[] record = null;
ArrayList temp = new ArrayList();
try {
// ...
while (rs.next()) {
record = new Object[columnCount];
for (int i = 0; i < columnCount; i++) {
record[i] = rs.getObject(i);
}
temp.add(record);
}
} finally {
// ...
}
this.data = temp;
// avoid exception to cause the data are not loaded completely
}
19.
public void printMetaData(Connection conn) {
// ...
LinkedList ll = new LinkedList();
DatabaseMetaData dmd = conn.getMetaData();
ResultSet rs = dmd.getTable(null,null,null,tabletypes);
while (rs.next()) {
// ...
String table = rs.getString("TABLE_NAME");
ll.add(table);
}
rs.close();
// avoid using multiple concurrent result sets.
ListIterator li = ll.listIterator(0);
While (li.hasNext()) {
String table = li.next().toString();
ResultSet rs2 = dmd.getColumns(null,null,table,null);
while (rs2.next()) {
// ...
}
rs2.close();
}
}
20.
public void add1000(Vector v, String s) {
for (int i = 0; i < 1000; i++) {
v.addElement(s); // addElement return void; it is faster
// because it can skip the overhead of a return value
// v.insertElementAt(s, 0); it is slowest
}
} // If you do not need synchronized, use ArrayList instead
// of Vector for efficiency.
// The ArrayList is basically a Vector minus synchronization.
21.
public void test() {
// ...
Hashtable ht = new Hashtable();
...
if ((val = (val) ht.get(key)) != null) {
val; // The containsKey() and get() are essentially identical
} // exceptfor having containsKey() return true/false, whereas get()
// get() returns the value or null.
ht.put(key, value); // ht.containsKey(key) is not necessary
// put() already checks for the existence of the key
// and will guard against duplicate keys.
}
22.
String s = "info";
String p = null;
public void func() {
Vector v = new Vector();
v.addElement(s);
// ...
p = (String) v.elementAt(0);
v.clear(); // reseting the Vector for re-use is efficient
// but, don't reuse StringBuffer: buffer.setLength(0), it's bad.
// do other thing
}
23.
BufferedOutputStream fos = //use Byte Stream instead of Unicode for efficiency
new BufferedOutputStream(
new FileOutputStream("stock.dat"));
byte[] b = buy.toBytes();
fos.write(b, ...);
24.
public void callManyTimes() { // reduce object Iterator creation because it is called
// many times
int size = myList.size();
for (int i = 0; i < size; i++)
{
Node node = (Node) myList.get(i);
// ...
}
25.
Map first = new ...
Map second = new ...
boolean isSubmap = true;
isSubmap = first.entrySet().containsAll(second.entrySet());
// use Collection manipulation idioms
26.
public class test {
public static void main(String[] args) {
// ...
try{
Connection con = DriveManager. ...
new TablePrinter(con, "Names").walk();
} catch (SQLException e) {
// ...
} finally { // The Split Cleaner bug. Should put con.close() here.
try { // If another class extends TableWalker
con.close(); // incorrect design will cause problem.
} catch (Exception e) {
// ...
}
}
}
}
abstract class TableWalker {
Connection con;
String tableName;
public TableWalker(Connection c, String tablename) {
this.con = c;
// ...
}
public void walk() throw SQLException {
// ...
ResultSet rs = stm.executeQuery(queryString);
while(rs.next()) {
execute(rs);
}
}
public abstract void execute(ResultSet rs) throws SQLException;
}
public class TablePrinter extends TableWalker {
public TablePrinter(Connection c, String tablename) {
super(c, tablename);
}
public void execute(ResultSet rs) throws SQLException {
// ...
}
}
27.
public class Work {
private Hashtable ht = new Hashtable();
// ...
public final void enHt(Object key, Object value) {
ht.put(key, value); // synchronized is unnecessary
}
}
28. // add BufferedOutPutStream for more efficiency.
// Using Java NIO can also improve the performance
public class A {
private static String fileName = "myfile.txt";
...
public BufferedOutPutStream writer = new BufferedOutPutStream(
new FileOutputStream(fileName));
public void write(int b) throws IOException {
writer.write((byte)b);
}
...
}
29.
// Many calls to this function will have a lock contention.
// using ConcurrentHashMap<K, V> can make multiple threads to call this function
// A lot 0f single thread code can be modified for use of multiple threads
// using new Java Concurrent feature.
public ManyMethodsCallThisFun () {
...
for (int i = 0; i < size; i++) {
ConcurrentHashMap<String, Record> map =
new ConcurrentHashMap<String, Record>(states[i]);
db.add(states[i], map);
}
...
}
30.
// incorrect design will cause memory leak
// c should call d.removeaListener(this) before to be null
public class A {
public static void main (String[] args) {
C c = new C();
// c should call d.removeaListener(this)
c = null;
System.gc();
}
class C implements aListener {
private D d = null;
public C() {
d = D.getInstance();
d.addaListener(this);
}
...
}
class D { // D is Singleton
...
public void removeaListener(aListener a) {
...
}
}
}
31.
// poor performance of string manipulation in incorrect design,
// should be
String x = "Hello " + name;
// or use StringBuffer().append()
32.
// the short-live object nodekey can be saved in the field and reuse it.
private Nodekey reuseKey = new Nodekey("");
private Node getNode(String key, Map map) {
reuseKey.setKey(key);
Node n = (Node) map.get(reuseKey);
...
return n
}
33.
// incorrect design created short-lived object; it's double objecs,
// not "double indemnity"
public class A {
private String name;
...
public A() {
setName("foo");
...
}
public void setName(String s) {
this.name = s;
}
...
}
34.
// catch the exception at the point closest the occurrence of the exception
public void readName() {
try {
name = in.read();
} catch (IOException e)
...
}
}
35.
// Don't use interface only to define instances.
// if in future the class no longer needs to use the constants
// it still must implement the interface. If a nonfinal cass implements
// a constants interface, all of its subclasses wil have their namespaces
// polluted by the constants. Use class instead and use import static.
pubic class Info {
private Info() {};
public static final int PINK = 1;
public static final int BLUE = 2;
...
}
import static pkg.Info.*
public class Graph extends Info {
...
switch (color) {
case PINK:
...
}
}
36.
//To instantiate an anonymous inner class that uses a non-final parameter
// would get compiler error
public void func(final String str) {
Runnable r = new Runnable() {
public void run() {
System.out.println(str);
}
};
Thread t = new Thread(r);
t.start();
}
37.
public class FileCopier {
public FileCopier(String source, String dest) throws FileCopierEx {
// catch IOException and throw FileCopierEx
}
}
try {
FileCopier fc = new filecopier(s, d);
} catch (FilecopierEx fce) {
...
}
fc.docopy(); // Throwing exception from constructor will cause the new
// operator not
// return, fc.docopy() will get NullPoiterException
38.
BufferedOutputStream fos =
new BufferedOutputStream(
new FileOutputStream("stock.dat"));
fos.write(buy.toBytes());
// fos.flush(); // Frequent flushes completely undermine the buffering
// mechanism. don't flush prematurely
fos.write(sell.toBytes());
fos.flush();
39.
// Use charAt() instead of startWith() for more efficiency.
if (s.charAt(0) == 'a') {...}
if (s.charAt(0) == 'c' && s.charAt(1) == 'd') {...}
40.
// System.out and System.err are instances of PrintStream class
// in which all exceptions it throws are eaten.
// Use checkError() to check errors.
...
System.out.write(somethings);
checkError();
41.
// There is no StringBuffer(char) constructor,
// So new StringBuffer('M') returns an empty string buffer
// with an initial capacity of 77 ('M' converts into int value 77).
public class A {
public static void main(String[] args) {
...
StringBuffer word = null;
switch(item) {
case 1:
word = new StringBuffer("M"); // change 'M' to be "M"
break;
...
}
word.append('e');
...
}
}
42. EJB2 JNDI
// Incorrect design assumes the AccountHome will always be located where
// EJB specification recommends. Sometime it will cause deployment problem.
// Change it as follows, then modify the deployment descriptor
// to make it more flexible in deployment.
Object obj = context.lookup("java:comp/env/FinancialService/Accounts");
43. EJB2
// In contrast to EJBExceptions, the application exceptions such as
// mysessionException don't trigger automatic rollbacks of the running
// transacion. So before re-throwing exception should call
// ctx.setRollbackOnly().
public class Mysession implements SessionBean {
SessionContext ctx;
...
public Myfuc() throws mysessionException {
try {
withdraw(...);
deposit(...);
} catch (mysessionException e) {
ctx.setRollbackOnly();
throw e;
}
}
...
}