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; } } ... } |