What are the worst anti-patterns you have came across in your career as a programmer?
I'm mostly involved in java, although it is probably language-independent.
I think the worst of it is what I call the main anti-pattern. It means program consisting of single, extremely big class (sometimes accompanied with a pair of little classes) which contains all logic. Typically with a big loop in which all business logic is contained, sometimes having tens of thousands of lines of code.
Commented out code. Blocks of it, maybe hundreds of lines. The theory being, hey it is commented out, it doesn't do any harm and maybe we will need it in the future.
I'll hit another obvious one with "copy-pasta". Copying code that is nearly identical to what you want, then changing a few things (instead of extracting into a method).
This is especially prevalent in some functional and API test code from the late 90's: Literally hundreds (or even thousands) of nearly-identical test cases that could have been broken out into a few functions that take 3 or 4 parameters - or, even better, something data-driven. My first job out of college was literally 8 months of rewriting and refactoring thousands of lines of copy-pasta that were using deprecated methods. By the time I was done, the test files were less than a 10th of their original size and far more maintainable (and readable!).
I think that I can write a lot about Pattern Mania and solutions that could be solved with a lot less effort, but I would rather point to a great article that I've read recently with a fantastic example of how a simple solution can be overcomplicated.
How (not) to write Factorial in Java or so we put a factory into your algorithm
Regions
In C# you can define a region of code that can be collapsed in the IDE, thus hiding it unless you want to deal with that code. I've been on (am currently on) a project where the regions spanned hundreds of lines (wish I was exaggerating) and there were multiple regions in a thousand line function (again I wish I was kidding).
On the upside, the developer who made the region did a very good job at identifying specific functions within a region. So much so that I was able to do an extract method on the region and move on.
Regions encourage developers to "hide" their crap in plain sight.
Not overloading methods:
// Do something
public int doSomething(Data d);
// Do same thing, but with some other data
public int doSomething2(SomeOtherData d);
Clearly shows that the programmer did not understand overloading.
Loop Switch Sequences
http://en.wikipedia.org/wiki/Loop-switch_sequence
They annoy me to no end and really show how inexperienced the dev was
Soft coding, ie when programmers go out of their way to avoid hard-coding and end up at the other side of the scale - "hard coded" dependencies.
One of the worst behavioral anti patterns I have seen was a shop that only allowed code to be checked in to version control after it was in production. Coupled with exclusive checkouts in VSS, this led to a convoluted process of undoing checkouts if a production defect needed to be corrected before the next release, let alone more than one person needing to change a given file for the upcoming release.
The fact that this was a departmental policy made it even worse than the case of a single developer behaving this way.
Once worked with a web application where all state was kept in the browser. (To ensure trouble-free scalability all exceptions were ignored).
Massive Checkins
I hate when I see a developer hasn't done a check-in in over a week. It means he's either stuck and hasn't looked for help, or he's clumping a bunch of features into one big check-in. (I left out the worst case scenario, he just isn't doing anything. That's easy to resolved...two words sounds like you're hired.)
If you're making a big check-in you lose a lot of benefits of SCM, like being able to link a changeset to a specific feature. Also you're likely to have a lot of merging to do and that's not necessarily easy to get right.
Currently, I'm working with a piece of legacy code and I love the way that previous coder gets the first element of a list:
String result;
for(int i = 0; i < someList.size(); i++) {
result = someList.get(i);
break;
}
But imho worst I've seen in this code is defining classes inline JSPs pages, write all the HTML, CSS and Javascript using scriptlet and out.println :-(
Code sprinkled with:
// TODO:
or
// TODO: implement feature
with no additional information.
Locking on a String literal
synchronized("one") { /* block one A*/ }
synchronized("one") { /* block one B*/ }
Very long class names. (in the JRE)
com.sun.java.swing.plaf.nimbus.
InternalFrameInternalFrameTitlePaneInternalFrameTitlePaneMaximizeButtonWindowNotFocusedState
Poor inheritance structure
com.sun.corba.se.internal.Interceptors.PIORB extends
com.sun.corba.se.internal.POA.POAORB extends
com.sun.corba.se.internal.iiop.ORB extends
com.sun.corba.se.impl.orb.ORBImpl extends
com.sun.corba.se.spi.orb.ORB extends
com.sun.corba.se.org.omg.CORBA.ORB extends
org.omg.CORBA_2_3.ORB extends
org.omg.CORBA.ORB
An Exception which is not.
public interface FlavorException { }
Pointless and cryptic error handling
if (properties.size() > 10000)
System.exit(0);
Unnecessary creation of objects
Class clazz = new Integer(0).getClass();
int num = new Integer(text).intValue();
Throwing an exception for other purposes
try {
Integer i = null;
Integer j = i.intValue();
} catch (NullPointerException e) {
System.out.println("Entering "+e.getStackTrace()[0]);
}
Using instance objects for static methods.
Thread.currentThread().sleep(100);
Synchronizing on a non-final field
synchronized(list) {
list = new ArrayList();
}
Pointless copy of a constant String
String s = new String("Hello world");
Pointless call to String.toString()
String s = "Hello";
String t = s.toString() + " World";
Calls to System.gc() to free some memory
Setting local variable to null to free some memory
// list is out of scope anyway.
list = null;
}
Using ++i instead of i++ for performance reasons (or any other micro-micro-optimisation)
Iterator for dummies:
Iterator iCollection = collection.iterator();
for(int i = 0 ; i < collection.size() ; i++ ){
if(collection.get(i) == something){
iCollection.remove();
}
}
Singletono-Factory:
public class SomeObject{
private SomeObject() {}
public static SomeObject getInstance(){
return new SomeObject();
}
}
if-else driven development (also called open close principle - open for modification close for understanding):
if (sth1){
...
}else if(sth2){
..
}
...
..
else if(sth1000000000000){
...
}
StringPattern (also called StringObject):
a) sendercode
if(sth1)
str+="#a";
if(sth2)
str+="#b";
...
if(sth1000)
str+="#n";
b) receiver
regexp testing if str contains #a, #b, ... #n
It's not so much of a coding pattern but a behavioural pattern, it's quite bad though: modifying something in the code (let's say the requirements changed), then tweaking all the unit tests until the code passes it. Tweaking or simply removing all test code from the test method, but leaving the method there.
This is linked to a more generic one though, the That'll Do pattern, here's a representative line of code for it:
int num = new Integer( stringParam ).parseInt( stringParam );
It works, after all.
I absolutely despise abstraction inversion, or the reinventing of low-level primitives on top of high-level primitives. Sometimes, though, this is caused by bad language designers, not bad programmers. Examples:
Using a single method class with no member variables and a corresponding interface, (implemented in terms of tables of function pointers), instead of a function pointer. Note that in languages like Java, you may have no choice.
In MATLAB and R, the insistence that everything is a vector/matrix rather than a primitive.
While we're bashing MATLAB, how about the fact that it has no integers, so when you need an integer you have to use a double.
Purely functional languages where you have to use function calls to write a loop.
A former co-worker had the habit of re-using objects by overwriting their properties, instead of simply creating new ones. I could never have imagined the amount of problems this has caused when implementing new features.
One of my favorite development anti patterns is the use of a database "design" that requires continually adding additional columns to one or more tables programmatically. This is a cousin of the "design" that creates a new table for each instance of an entity. Both inevitably hit limitations of the database server, but usually not until the system has been in production for quite some time.
Something that has caused me lots of grief is the "Big map in the sky" pattern. Throwing around a map instead of using proper objects. You have no idea of what "variables" it contains without debugging, and you don't know what it might contain without tracing the code backwards. Typically maps Strings to Objects, or Strings to Strings, which you potentially are supposed to parse into primitives.
It would be definitely copy/paste, I've seen a lot of bad code getting done because of it, that and cowboy coding and everything that derives from that. (God Classes, extra large methods, bad thinked algorithms, etc.)
And if design patterns are allowed, them I would say: Doing a project as a set of actions from a plan, without doing any design or domain analysis.
Multi-use java bean -
A java bean with lots of variables, used in a few different kinds of operations. Each operation uses some arbitrary subset of the bean variables, and ignores the others. A few variables for gui state, a few variables thrown in for passing around between components, a few that probably aren't even used anymore. Best examples have no documentation, which would hinder appreciation of the pattern.
Also, can't forget the beloved
try{
... //many lines of likely dangerous operations
}
catch(Exception e){}
IMO the worst anti-pattern I've seen is the "We don't need any stinkin' patterns" Anti-Pattern: The idea that design patterns are wastes of time and you can write code faster by just slopping it together and copy/pasting as needed.
Honorable mention goes to having code that loads an object from the database using the old VB6 style of:
Foobar oFoo = new Foobar();
oFoo.FooID = 42;
if (oFoo.Load()) {
// do something with oFoo
}
not really an anti-pattern, per se, but shows a lack of taking advantage of proper architecture and separation of concerns.
Also, things like this:
// this name is misleading, we may not always want to stand in fire,
// we may want to stand in slime or voidzones or ice patches...
public Foobar StandInFire() { }
// why is this here???
public string BeatWithNerfBat(string whom) { }
// ????
public int GivePony(string to) { }
Cart-Before-The-Horse -- aka YouMightNeedIt
For example:
I think one of the worst anti-patterns I have seen involves using a Database table as temporary storage instead of using computer memory.
The problem domain is proprietary which disallows me from explaining it, but it isn't necessary to understand the basic problem. This was a GUI application written in Java with a backend Database. It was to take certain input data, manipulate it and then commit the processed data to the database.
Our project has a fairly complicated algorithm which saves intermediate values for processing later on. Instead of encapsulating the temporary objects in... objects, a database table was created like so "t_object". Every time a value was calculated, it was added to this table. After the algorithm finished it's work, it would select out all of the intermediate values and process them all in one large Map object. After all the processing had been finished, the remaining values that were marked to be saved would be added to the real database schema and the temporary entries in the "t_object" table would be thrown away.
The table was also used like a unique list, data could only exist once. This might have been a decent feature of the design had we implemented Constraints on the table, but we ended up iterating through the whole table to see if the data existed or not. (No we didn't even use queries that used where clauses with CONTAINS)
Some of the problems we ran into due to this design were specifically debugging. The application was built to pipeline data so there would be multiple GUIs that would pre-process the data before it arrived at this algorithm. The debugging process was to process a test case and then pause right after finishing the above section. Then we would query the database to see what data was contained in this table.
Another problem we discovered was that data wasn't being deleted properly from this temporary table, which would interfere with run's in the future. We discovered this was due to Exceptions not being properly handled and therefore the application not exiting properly and not deleting the data in the table it was in control of.
If we had used Basic Object-Oriented Design and kept everything in Memory, these above problems would never have occurred. First, debugging would have been simple as we could easily setup break points in the application and then inspect the memory in the stack and heap. Secondly, upon abnormal exit from the application, java memory would have been cleaned up naturally without having to worry about deleting it from the database.
NOTE: I'm not saying this pattern is inherently bad, but in this example I found it to be unnecessary when basic OO principles would have sufficed.
I'm not sure of a name for this anti-pattern as this is the first time I have seen anything like this done. Any good names you guys can think of for this pattern?
