9
votes

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.

0

26 Answers 26

38
votes

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.

8
  • 7
    Well at least you can safely delete it. Unlike someone I've worked with that would frequently rename procedures and leave the dead code accessible. Blech.
    – Jay
    Feb 4, 2011 at 18:25
  • @Cyrena, you worked with Eric too?!?
    – CaffGeek
    Feb 4, 2011 at 18:43
  • I had a bunch of code once that used #ifdef COMMENT to lock things out. Worked great until someone defined COMMENT. Feb 4, 2011 at 19:55
  • 9
    This is another case where using version control pays off. You can delete code without having to ask whether or not you may need it in the future because it can be retrieved again with a few keystrokes.
    – Jason B
    Feb 4, 2011 at 20:28
  • 5
    I think commented-out code has a place, if you prefix it with a comment that explains why its there. Sometimes I will leave a commented block that represents a potential road I was thinking about taking, and leave it with a comment about why I didn't. @Jason, I definitely hear you about this being the role of version control, but deleted code in version control is not very discoverable.
    – nlawalker
    Feb 4, 2011 at 20:42
19
votes

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!).

16
votes

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

5
  • 3
    Awesome! Now where's the FactorialWebService? :P Feb 4, 2011 at 18:26
  • 1
    I call it Pattern Fever...everyone gets it at one point in their career. The better among us grow beyond it. I had an interview where the guy asked me when should I think about patterns. I told him that I let them evolve into the system. He said "No, you should use patterns from the start." Feb 4, 2011 at 18:30
  • FactoryFactories are just a symptom of wanting to defer the actual choice as much as possible but it still ends up in either hardcoding it or mapping an external string value to a piece of code. Dependency Injection is a better solution.
    – user1249
    Feb 4, 2011 at 18:49
  • Patterns are artifacts of good design - they should be treated as such. I like to describe them as definitions rather than solutions. They are useful when communicating, but not necessarily in designing.
    – Michael K
    Feb 4, 2011 at 21:23
  • Thanx for that, good read.
    – Syg
    Feb 10, 2011 at 15:28
14
votes

Golden Hammer

“I have a hammer and everything else is a nail.”

14
votes

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.

5
  • 15
    +1 for Regions encourage developers to "hide" their crap in plain sight. However, when used properly, I find that regions help to logically group together code and easily find at a later time. Feb 4, 2011 at 19:17
  • This has long been a problem with folding editors. I used to do the same sort of thing when I last programmed in OCCAM. Feb 4, 2011 at 19:57
  • 2
    I love regions...but only for organization...not hiding digital reams of code.
    – IAbstract
    Feb 4, 2011 at 20:31
  • 3
    +1. That's also why using regions violates StyleCop rule SA1124 DoNotUseRegions. Feb 4, 2011 at 21:01
  • 1
    I have projects which work with a huge number of fields in SQL, and the code that updates/creates it is many lines. A region #region SQL Update makes it collapsible so less scrolling is required.
    – JYelton
    Feb 5, 2011 at 0:13
12
votes

To me the worst pattern is Copy/Paste.

1
  • 28
    To me the worst pattern is Copy/Paste.
    – Andrew
    Feb 4, 2011 at 20:44
9
votes

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.

5
votes

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

2
  • Ah, a terminating state machine :)
    – user1249
    Feb 4, 2011 at 18:37
  • Why do you have to bring up these buried memories? I was having such a nice day.
    – Malachi
    Aug 25, 2011 at 14:08
5
votes

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.

4
votes

Keep state in the client.

Once worked with a web application where all state was kept in the browser. (To ensure trouble-free scalability all exceptions were ignored).

3
  • Could you please elaborate? Imho, for a Web application, it is fine when the only state is in the Browser (i.e., the cookies), and the server is 'shared nothing'. Or do you mean 'state' as in 'the applications database'?
    – keppla
    Aug 30, 2011 at 15:25
  • State: As in the actual data to operate on.
    – user1249
    Aug 30, 2011 at 19:02
  • O.O you have my deepest sympathies.
    – keppla
    Aug 31, 2011 at 6:11
4
votes

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.

1
  • 1
    Not doing anything is not always the worst case scenario. Sometimes it's better to have to write it from scratch than to have code to rewrite...
    – Malachi
    Aug 25, 2011 at 14:10
4
votes

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 :-(

4
votes

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.

3
votes

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)

3
  • These aren't necessarily anti-patterns, just bad coding. Feb 5, 2011 at 16:02
  • @Gary, or poor patterns of development I see repeated. Perhaps not within the strict definition of an anti-pattern. Feb 5, 2011 at 19:08
  • 1
    @Peter: "Calls to System.gc() to free some memory", I have seen one occasion where .NET would explode with OutOfMemory exception unless GC was called explicitly. Of course it was more of an exception rather than a rule.
    – Coder
    Aug 25, 2011 at 21:49
3
votes

Code sprinkled with:

// TODO: 

or

// TODO: implement feature 

with no additional information.

2
votes

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
4
  • That else if is often then only way to get things done, though. I'm thinking about strings; can't use a switch. The conditions should be alike for it to be useful.
    – Michael K
    Feb 4, 2011 at 21:26
  • IMHO each if/else can be replaced with simple mapping or visitor pattern. In case of strings i.e. you may have properties file like : someString1=some.package.ObjectToHandleSomeString1 Feb 5, 2011 at 15:55
  • 2
    The singleton is a factory. There is nothing wrong with that code. The only thing that could be wrong is that outside code makes the assumption, that every call to getInstance returns the same instance. Such an assumption would break encapsulation and is in fact one of the most common anti-patterns.
    – back2dos
    Aug 25, 2011 at 14:59
  • exacly that is why it is so confusing :) if the method was called create() or it would return the very same instance each time everything would be perfectly fine Nov 25, 2011 at 6:51
2
votes

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.

2
votes

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:

  1. 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.

  2. In MATLAB and R, the insistence that everything is a vector/matrix rather than a primitive.

  3. 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.

  4. Purely functional languages where you have to use function calls to write a loop.

1
vote

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.

1
vote

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){}
2
  • IMHO, exceptions stink. They are too hard to get them right, and they add a false sense of security.
    – Coder
    Aug 25, 2011 at 21:51
  • Whatever your opinion of stinkiness of exceptions, silently swallowing one has to stink more.
    – Steve B.
    Feb 11, 2012 at 23:25
1
vote

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.

1
vote

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.

1
  • Sounds kind of like relying on Session and ViewState in ASP.NET to pass gobs of data between pages, which sadly is often required... Aug 25, 2011 at 15:30
1
vote

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.

0
votes

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?

4
  • I wouldnt call this a blanket problem. It'd depend on how much temporary data is being stored and whats being done with it. Feb 4, 2011 at 19:27
  • I should add more to the post but the main problem was that instead of modifying objects and accessing the data from memory, the data was manipulated with hack sql code. This led to increased complexity and severe performance issues.
    – jluzwick
    Feb 4, 2011 at 19:36
  • 2
    You don't mention your problem domain, but not always a bad thing, keeping state elsewhere like this could allow processes to scale and to be introspected during long run times, also to help in debugging. Was the DB access causing performance issues or concerns?
    – Jé Queue
    Feb 4, 2011 at 20:43
  • I've edited the article to better reflect how this was used in our project, but we had numerous issues when debugging, specifically because we had to query the database to see the temporary variables. Also we had large performance-related problems due to the database constantly being queried for data and checking if new data already existed.
    – jluzwick
    Feb 4, 2011 at 21:21
0
votes

Cart-Before-The-Horse -- aka YouMightNeedIt

For example:

  • Creating a RDMBs schema with abstract concepts, cross references -- in essence an over-generalized data cube... And THEN writing features around a does-everything model.
1
  • That would be YAGNI's evil twin brother, right? Aug 25, 2011 at 15:29
0
votes

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) { }

Not the answer you're looking for? Browse other questions tagged or ask your own question.