The worst codebase I’ve seen in my life

I recently inherited a huge legacy project (let’s call it FailApp) which features brutally substandard code. I’ll present the best gems here, not as a rant, but as a case study of what is possible to be delivered in the software industry for a six figures sum of money.

The gems are categorized in three levels and (due to popular demand) each contains an oneliner of what the author should have done instead.

1) the LOLs

The LOLs are code gems that when you discover them you laugh out loud. They are so funny that you’ll mention them to your coworkers while drinking beer or even post them in a forum in order to have a laugh. They are little harmless anecdotes of failure that the average Joe programmer will do. They are usually the result of incompetent project management (stuffing serious projecs with baby developers) and total absense of industry standard practices such as code reviews, unit tests and code conventions.

2) the WTFs

The WTFs are code gems and design decisions which are totally perverted. You’ll discover some of them after an hour worth of debugging and they are not very funny but rather piss you off. They cause problems in the health of the project and they are the reason why the client ditched the original authors of the app and brought it to you.

3) the showstoppers

Things get hard and serious here. The showstoppers are fatal, dangerous and inexplicable design decisions that the original author made which make your brain explode. They are hard to deal with and will usually cause serious bottlenecks to the application and irreversible data corruptions. When you discover them you will immediatelly call an urgent meeting with the management to explain the situation. These are no fun stuff and probably written by people who have no idea of what they are doing. These problems usually cost serious amounts of money.

The code snippets of the FailApp have not been changed at all. In this case there isn’t any “innocent” to protect since serious harm has been already done with this codebase.

So, here we go.

LOL#1
“Prod” in the application context name. So the staging environment URL looks like http://123.12.34.123:7001/FailAppProd and the production http://123.12.34.134:7001/FailAppProd. Good luck remembering which one is which.

Solution: Not use the environment name in the URL at all. If necessary provide variability on deployment descriptors (possibly upon build generation) which configures “dev”, “prod”, “staging” in the context name.

LOL#2

EscapeIlegalWords eiw = new EscapeIlegalWords();
foo = eiw.escapeIlegalWords(foo);

Yes, we’ve got a spelling mistake here and a really bad local variable name. But the real laugh comes when you navigate into the method:

public static String escapeIlegalWords(String code) { 
    code = code.replaceAll("&lt;", "<");
    code = code.replaceAll("&gt;", ">");
    code = code.replaceAll("&amp;", "&");
    code = code.replaceAll("&apos;", "'");
    code = code.replaceAll("&quot;", "\"");
    code = code.replaceAll("&#13;", "");
    return code;
}

- a new instance for calling the static method? ouch…
- oh I see, there aren’t any “illegal words” here, it simply does something with html which should be done in the template anyway…
- damn, this is the opposite of escaping!
- class name doesn’t make any sense at all

Solution: Fix mentioned issues.

LOL#3

session.setAttribute("contentXML", null);
session.removeAttribute("contentXML");
session.setAttribute("contentOwnerList", null);
session.removeAttribute("contentOwnerList");
session.setAttribute("structuresVector", null);
session.removeAttribute("structuresVector");
session.setAttribute("selectedThematic", null);
session.removeAttribute("selectedThematic");
session.setAttribute("selectedTarget", null);
session.removeAttribute("selectedTarget");
session.setAttribute("targetList", null);
session.removeAttribute("targetList");
session.setAttribute("thematicList", null);
session.removeAttribute("thematicList");
session.setAttribute("metadatas", null);
session.removeAttribute("metadatas");

- yes, please do make sure that you really remove those variables from the session… removing them twice will eventually do the trick.

Solution: Remove the setAttribute(…, null) statements because they are redundant.

LOL#4

ArrayList arrActions = new ArrayList();
HashMap order = new HashMap();
public ArrayList getHighLights(int idStructure, int language, int home) { ...

- thank you very much for declaring on concrete type. ever heard of Interfaces and the Collections API design philosophy?

Solution: Declare on Interfaces which is enough for these cases.

LOL#5

ArrayList avlanguages=null;
if (request.getAttribute("avlanguages")!=null){
    avlanguages=(ArrayList)request.getAttribute("avlanguages");
}
<%if (avlanguages!=null && avlanguages.size()>0){%>
<%for(int x=0;x<avlanguages.size();x++) { %>

- yes, this is too much Java code inside JSP pages instead of using JSTL’s <c:forEach...

Solution: The view layer should use JSTL and not JSP scriptlets.

LOL#6

Some spelling mistakes which will definitely complicate greping for information in logs and code:

END GENERTAION THEMATICS.
updateStatusDocumentDeplubishCron()

Solution: Avoid spelling mistakes.

LOL#7

CommonDatosPopUp pop = new CommonDatosPopUp();
pop = (CommonDatosPopUp)popUp[i];

- that is great. thanks for instantiating something only to throw it away. it does make sense in case the constructor is firing a missile though.

Solution: Declare and fetch from array in one line avoiding the unnecessary construction of an object.

LOL#8

ArrayList List = new ArrayList();
List = (ArrayList) baseManagerDao.getPosition(baseItemIdVar);
for (int i = 0; i < List.size(); i++)

- once again, create that empty ArrayList() and then throw it away
- declaration on concrete Collection type
- iterating a Collection datastructure using index
- exceptional naming of variable List to look like the Interface List (I wonder why the language allows such a raping of itself here)

Solution: Fix mentioned problems.

WTF#1

if (cm.getBlockLevels() != null) {
    request.setAttribute("blocklevel", cm.getBlockLevels());
}
if (cm.getUsers() != null) {
    request.setAttribute("users", cm.getUsers());
}
if (pm.getSigGroupPub() != null) {
    request.setAttribute("siggroups", pm.getSigGroupPub());
}

- hm… cm and pm (really bad local variable names) are managers which delegate calls to DAOs. These DAO methods (probably hitting a database) are being called twice
- and why do we have to nullcheck in the first place? if the result is null we can simply put null (remove) into the request attrs. (and no, these variables where not previously set into the request by another piece of code).

Solution: Remove the if statements altogether. If the statements are necessary assign result from managers to local variables to avoid double DAO method execution.

WTF#2

While trying to fix something I searched for “siggroup”. Notice the variations of the key, all apearing in the same (2000 line) controller:

request.setAttribute("siggroups", pm.getSigGroupPub());
request.setAttribute("sigGroups", sigGroups);
request.setAttribute("SigGroups", hm.getSigGroups(Integer...

- zero consistency is the salt of the programmers life…
- the best part though is in the template:

List sig = (List) request.getAttribute("sigGroups");
if(sig == null) {
    sig = (List) request.getAttribute("SigGroups");
}

- good luck finding which collection you are rendering on screen now…

Solution: Be consistent in the naming of your variables, especially those which are being used in another layer of the application, the view layer. Never do such acrobatics with the capitalization of variables. Once again, use proper names.

WTF#3

There is no javadoc and the most crucial parts of the app are commented in a non English language, including non-ascii characters. http://translate.google.com/ to the rescue.

Solution: Unless your company code style guidelines allow it, never use non-English in projects, especially EU funded ones where you’ll need to deliver full source code later on. Also, non-ascii characters may make some systems of the deployment process choke (e.g peers with old editors, old source diff programs, badly installed C.I environments etc). Also they can become the causes of bugs. e.g can you spot the difference between String action = "NULL"; and String action = "ΝULL";? This can cause problems, that’s why some people decide to never allow non-ascii characters into their IDE so they can catch these issues early on.

WTF#4

Classes with 2000 lines of code methods and 14 level nested ifs. These are usually do-everything controllers which serve many unrelated things from the same codebase (page results, binary downloads, csv). These are usually replicated 10 times with minor differences to accomodate slightly different use cases. Nough said.

Solution: It has happened to all of us. It’s called spaghetti code. When it happens try to think of a better design. If you can’t, please consult the lead developer of the project for assistance. Spaghetti code will bite you back sooner than you expect.

WTF#5

pubHighlights = cm.getHighLights(structId, userLang,
					Constants.PREDEFINED_SEARCH);

- ok, we are using constants for variability instead of different methods or inheritance
body of method:

public ArrayList getHighLights(int idStructure, String lang, int home) {
    ...
    if (home == 1) {
        listado = commonDao.getHightLightHome(idStructure);
    } else {
        listado = commonDao.getHightLightPredefinedSearch(idStructure);
    }

- facepalm for home==1 instead of Constant. good luck with debugging when that constant changes
- by the way it turns out that the initial call should send Constants.HOME instead of Constants.PREDEFINED_SEARCH. It just happens that both equals 1.

Solution: Use OOP practices for solving such problems. If you can’t and need to use constants please use enums. If you can’t use enums and need to use primitive constants please do make full use of them (and not partial, as in this case).

WTF#6

Absence of templating reuse. The 150+ JSP templates contain everything from html declarations to website footer (with copyrights and everything). Only with minor and insignificant differences due to inconsistent copy pasting of headers and whole pages with minor changes. Ever heard of include?

Solution: Do not copy paste like crazy. By copy pasting sections instead of reusing them you may end up with templates which are as complex as your code. All templating systems offer facilities for reuse.

WTF#7

Form validation for the brave:

if (appForm.getFileCV() == null || StringUtils.isEmpty(appForm.getFileCV().getFileName())){
	errors.add("fileCV", new ActionError("error.fileCV.required"));
}else if (!appForm.getFileCV().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileCV().getFileName().toLowerCase().endsWith(".pdf") 
		&& !appForm.getFileCV().getFileName().toLowerCase().endsWith(".rtf") && !appForm.getFileCV().getFileName().toLowerCase().endsWith(".sdc") 
		&& !appForm.getFileCV().getFileName().toLowerCase().endsWith(".zip") )
	errorExtension = true;
if (appForm.getFileLetter() == null || StringUtils.isEmpty(appForm.getFileLetter().getFileName())){
	errors.add("fileLetter", new ActionError("error.fileLetter.required"));
}else if (!appForm.getFileLetter().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileLetter().getFileName().toLowerCase().endsWith(".pdf") 
		&& !appForm.getFileLetter().getFileName().toLowerCase().endsWith(".rtf") && !appForm.getFileLetter().getFileName().toLowerCase().endsWith(".zip")
		&& !appForm.getFileLetter().getFileName().toLowerCase().endsWith(".zip"))
	errorExtension = true;
/* if (fileForm == null || StringUtils.isEmpty(fileForm.getFileName())){
	errors.add("fileForm", new ActionError("error.fileForm.required"));
}else*/
if(appForm.getFileForm() != null && !StringUtils.isEmpty(appForm.getFileForm().getFileName()))
	if (!appForm.getFileForm().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileForm().getFileName().toLowerCase().endsWith(".pdf") 
		&& !appForm.getFileForm().getFileName().toLowerCase().endsWith(".rtf") && !appForm.getFileForm().getFileName().toLowerCase().endsWith(".sdc")
		&& !appForm.getFileForm().getFileName().toLowerCase().endsWith(".zip"))
	errorExtension = true;
if(appForm.getFileOther1() != null && !StringUtils.isEmpty(appForm.getFileOther1().getFileName()))
	if (!appForm.getFileOther1().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileOther1().getFileName().toLowerCase().endsWith(".pdf") 
			&& !appForm.getFileOther1().getFileName().toLowerCase().endsWith(".rtf")&& !appForm.getFileOther1().getFileName().toLowerCase().endsWith(".sdc")
			&& !appForm.getFileOther1().getFileName().toLowerCase().endsWith(".zip"))
		errorExtension = true;
if(appForm.getFileOther2() != null && !StringUtils.isEmpty(appForm.getFileOther2().getFileName()))
	if (!appForm.getFileOther2().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileOther2().getFileName().toLowerCase().endsWith(".pdf") 
			&& !appForm.getFileOther2().getFileName().toLowerCase().endsWith(".rtf")&& !appForm.getFileOther2().getFileName().toLowerCase().endsWith(".sdc")
			&& !appForm.getFileOther2().getFileName().toLowerCase().endsWith(".zip"))
		errorExtension = true;
if(appForm.getFileOther3() != null && !StringUtils.isEmpty(appForm.getFileOther3().getFileName()))
	if (!appForm.getFileOther3().getFileName().toLowerCase().endsWith(".doc") && !appForm.getFileOther3().getFileName().toLowerCase().endsWith(".pdf") 
			&& !appForm.getFileOther3().getFileName().toLowerCase().endsWith(".rtf")&& !appForm.getFileOther3().getFileName().toLowerCase().endsWith(".sdc")
			&& !appForm.getFileOther3().getFileName().toLowerCase().endsWith(".zip"))
		errorExtension = true;

Solution: Don’t do this. All MVC frameworks offer some sort of validation facility in order to avoid code soup such as the above. Even if it doesn’t, try to extract the most repeated part of the validation into a method which you’ll reuse.

WTF#8

There are methods which return Vector (yes, 1999 called) and it turns out that the result is never being used but instead the purpose of the method is to mutate the parameters. Smart.

Solution: Instead of Vector use a List. Methods should not have sideeffects and when they do they should mention it in the documentation or reveal it in the method name. Also, returning an unmodified object only to sideeffect the arguments is very confusing. If the method’s logic changes during implementation please try to adapt its signature so it keeps making sense.

showstopper#1

cm.getHighLights is an expensive operation (calculates the nested menu of the website) and the following code is supposed to introduce caching into the game:

ArrayList pubHighlights = (ArrayList)request.getSession().getAttribute("publicationsHighlights");
if(pubHighlights == null || pubHighlights.size() == 0)
pubHighlights = cm.getHighLights(structId, userLang, Constants.PREDEFINED_SEARCH);
request.getSession().setAttribute("publicationsHighlights", pubHighlights);

- it stores the result in the http session so there is a lot of waste of memory in case we have many users
- the generated menu takes into account “structId” and “userLang”. The cache key is only one though (“publicationsHighlights” in the session), so if the user changes structId or userLang, the menu stays the same
- changes on the menu structure are not reflected to already cached clients. They’ll see these changes only if they get a new session (come back later, use another browser etc)

Solution: Think of your cache design. Does it make sense or does it brake the UI and the server? In this case the cache should be in the application layer and the key should take into account all parameters which should modify the appearance and behavior of the cached object.

showstopper#2

Application “does things” to the database on view. Things == if stuff are not there it silently creates them, so for example if you visit the about page of the French site and there is no content there (either from CM error or data corruption) it simply creates an empty one and inserts it into the database. This is nice and useful especially when the application thinks that there isn’t any content there, so after a couple of days you’ll find thousand of empty “about” pages under the French site waiting to be fixed by you.

Solution: This is dangerous. Don’t do it. GET requests should rarely modify the database.

showstopper#3

Total incompetence in exception design and handling. The usual anti-pattern of swallow and return new ArrayList() is followed through the system. Database errors are masked and the system goes on doing what it was doing (e.g continuing with other parts of data changes, email dispatching etc).

Solution: Learn about exception handling and how they make sense in your application’s layers. Have a look at 1, 2 and 3.

showstopper#4

“Change navigation element==edit property” anti-pattern. This is sick. Imagine a CRUD page with a couple of filters on top and an entities listing below. In the year filter you choose 2010 and hit GO. The listing is updated with entries from 2010. Now you change the year filter to 2011 but do not hit GO. Instead you hit EDIT on one of the 2010 entities below. What happens is that the 2011 value from the filter is transfered into the (hidden) element of the edit form. As soon as you hit SUBMIT the entity now belongs on 2011. Nice.

Solution: Don’t do this. The application should have clearly defined use cases for modifying objects. Never trust the UI.

showstopper#5

The search is FUBAR. A single search for “foo” issues 200.000 db queries and requires 5 minutes on the production server because:
- it first does “select *” the whole publications database in sorted batches of 1000 back to a collection.
- it then feeds this collection into a method which filters things out.
- while filtering some entity methods are accessed and due to bad fetch plan from hibernate tons of N+1 statements are executed.

Solution: When developing try to fill in your test database with tons of data to see how your use cases scale. For this particular issue, learning about the ORM’s fetch plans and how these relate to your model and use cases is very useful.

showstopper#6

“Toxic servlets hierarchy”. All actions extend something (a base class) which extends servlet. The base class provides a public static spring factory field which is initialized on boot of the servlet. Yes, the only reason of existence of this base class is to provide this field and the actions extend it in order to get access to this public static field. Great.

Solution: Don’t do this. If you use Spring then use it properly.

showstopper#7

Log4j & hibernate initialization rediscovered! Both libraries are being configured in the following fashion:
- read the log4j.properties and hibernate.cfg.xml configuration files from a custom location using a ContextListener
- write contents into a new file in the server’s root folder
- load from there
- documentation states that if application cannot boot the application’s configuration files should be removed from the server’s root folder!

Solution: Both frameworks require the configuration file in the default classpath. No acrobatics required.
The end.

59 Responses to “The worst codebase I’ve seen in my life”

  1. nk Says:

    πλάκα κάνεις…. Καλή τύχη.

    υ.γ.: Κατά τα άλλα εφαρμογή γραμμένη σε Spring & Hibernate, ε; Έτσι για να ήμαστε στην κόψη….

  2. Michael Says:

    It is very difficult to read the source examples because of a UI show stopper of your own. Gray background with tiny dark gray (maybe it is black) foreground text colour. WTF!!!!!

  3. ALT-F-X Says:

    Honestly you should worry more about rewriting this code than making a blog post about it’s badness

  4. monkey Says:

    my advice: Refactor don’t throw away and rewrite. Slowly, steadily.

  5. Mark Says:

    Michael: They are readable to me on the PC and phone.
    ALT-F-X: Really? What’s wrong with sharing/commiserating? Some times you have to step away from the steaming pile and vent. Been there. Too many times.
    Monkey: Crap code is almost impossible to refactor. That is one of the major issues with it.

    Ioannis: The place i work, most of the code is this way. And we have a ton of apps. The only thing I “disagree” with you on – Sometimes you need to use the impl of a collection. The rule should be that interfaces should be returned from methods and for properties fields. If you are creating and using it in a method, then you really don’t need to use the interface. I just ran into and issue in C# where the List class had methods that the IList did not. Sigh.

  6. Nikolaos Georgiou Says:

    Solidarity from the front.

  7. Manu A. Says:

    In LOL#2 you missed the funniest point: any “”) will be replaced by “<” which will then be turned into “&lt;” … LOL!

  8. Jonix Says:

    This is a very fun read. Though you need to be familiar with JSP to really appreciate the LOL’s.

    I think I been guilty for introducing LOL’s into my own code, sometimes it takes another person to see the code with a fresh set of eye balls.

  9. cherouvim Says:

    Michael: I’ve increased the contrast. Thanks for pointing out (not a showstopper though IMO :).

    ALT-F-X: Most of it has been fixed. During the 6 months of my involvement I kept a backlog of gems I found and in the end I shared to the world.
    I’ve done worst myself and 2 years ago I exposed my own biggest stupidities in http://blog.cherouvim.com/the-stupidest-things-ive-done-in-my-programming-job/
    It’s good to document and expose mistakes in software construction. The software engineering is the only engineering principle (e.g compared to mechanical, electronic etc) where shit like that can be accepted and payed for. This shit costs money and we need to document, communicate and educate us as much as possible. Do you agree?

    monkey: Yes. A very helpful book for my quest with this spaghetti monster has been http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

    Mark: Thanks for your support. Regarding the types, in the Collections API of the Java world it’s best to always use the interface. Many frameworks (and even your own code) may need to change implementation at some point.

    Manu A.: Yes, only here it unescapes so I don’t think this is possible. Funny though :)

    Jonix: Me too, and not only “LOLs”. Have a look at http://blog.cherouvim.com/the-stupidest-things-ive-done-in-my-programming-job/

  10. Rubén Says:

    I would regard LOL#4 as an outrageous WTF.

  11. Renke Grunwald Says:

    I don’t see the problem with LOL#4, can you explain it to me? You don’t have to restrict yourself to just the interface if you can use an implementation that gives even more possibilities. Returning a ArrayList in that method is not bad either; if, however, that method would only accept an ArrayList as parameter instead of list, even though it only uses methods defined in list, I would agree with you on that LOL.

  12. cherouvim Says:

    @Renke Grunwald: You need to declare that you’ll return List. Otherwise if you later on decide to return a different flavour of List (instead of ArrayList) then all clients will break. Also, why would the client care if it is an ArrayList? The client can iterate and pick elements from the list via the Interface without a problem.

  13. Renke Grunwald Says:

    @cherouvim sometimes the implementation of an interface offers useful additions, that can be helpful for certain tasks.

    I don’t see why the client should break; if the client only cares that a method returns a list, it will just use a List.

    Say,

    List list = yourMethod()

    instead of

    ArrayList list = yourMethod();

    though it might be a problem if the client uses it as ArrayList even though a List suffices, then, indeed, it would break.

  14. Dan Says:

    >> EscapeIlegalWords eiw = new EscapeIlegalWords();
    >Yes, we’ve got a spelling mistake here and a really bad local variable name.

    We have a programmer here who strongly believes in such variable names and actually advocates using them :|

  15. Héctor Says:

    some var names seem to be spanish made… feeling embarrased right now…

    @Renke conventional wisdom says you should always code to interfaces unless you explicitly need a concrete type. What’s wrong with that snippet? The method definition. As cherouvim said, you wouldn’t be able to change from ArrayList to LinkedList in the method implementation because you have coupled your type to the caller’s code.

  16. Larry Battle Says:

    I know this is a stupid question but did the code compile before you got it?

  17. cherouvim Says:

    @Larry Battle: The code itself was in a compilable situation :) It wasn’t easy though due to environment related problems (missing dependencies, JNDI datasource misconfigurations etc).

  18. Kevin Says:

    Over your career you will find worse. I have seen these same mistakes made many times over. The most fun I had was code like:

    parent.parent.parent.parent.addButton.setVisible(true);

    Every class had a member variable called parent. What type parent happened to be made some sort of sense to that class maybe but parent was not of any particular type through the code. No OOP, just everyone having access to all public variables of every class and doing chained parent calls to get to what they needed.

    Pretty much made my eyes throw up.

  19. Adam S. Says:

    Another case when you want to use concrete classes instead of interfaces is when you program in GWT.

  20. disgruntledworshipper Says:

    >> I wonder why the language allows such a raping of itself here

    In reference to naming a variable List. the whole reason java uses packages is for this exact reason. Don’t see how the language is to blame here…

  21. cherouvim Says:

    @disgruntledworshipper: Note that we are talking about the name of an instance here, not a class. Does naming a variable “String” help? What problem does allowing this solve?

  22. Mark Says:

    cherouvim: “Regarding the types, in the Collections API of the Java world it’s best to always use the interface. Many frameworks (and even your own code) may need to change implementation at some point.”

    Right, but you won’t typically be changing the impl within a code block. So not setting it to an interface is ok. Typically you will be doing things with the class that you cannot with via interface. The rule is for Method and Accessors/Mutators (aka Properties) and Class level variables because THAT is where the impl can change.

  23. droope Says:

    never rewrite from scratch :)

  24. Eric Jablow Says:

    I’ve programmed a project in GWT, the Google Web Toolkit. GWT translates a subset of Java into Javascript. Because of this, it is most practical to use collection classes instead of interfaces. If you were to declare a method as taking a List parameter, GWT might translate ArrayList, LinkedList, and any other concrete List implementation it can find into Javascript.

    The resulting code looks peculiar.

  25. BreakTherulesIfYouUnderstandThemFirst Says:

    Using JSTL versus scriptlets is a context issue; if you work in a shop of entirely java developers, scriptlets often make more sense than using a tag library, especially in years past when Tomcat leaked memory from taglibs; the reflective nature of tag parameters makes refactoring and compile-time safety more difficult and arguably slower performing than java code. Just don’t mix non-presentation-tier logic into the JSPs.

  26. Fabricio Says:

    Regarding WTF#2: why in the hell getAttribute is case-sensitive? If it was case-INSENSITIVE, using your scale of badness, it would become just LOL#9 – as it would become an MINOR convention infringement but would NOT affect your ability to follow code flow since SIGGROUPS is the same as SigGroups or siggroups. There’s no damage to the healt of the project since you can not declare SIGGROUPS and siggroups as distinct variables in the same scope.

  27. cherouvim Says:

    Fabricio: It is case-sensitive since the underlying datastructure is a Map. The keys used (in this case Strings) feature a case-sensitive .equals() method.

  28. Fabricio Says:

    WTF#2:
    The lib doesn’t allow to declare an case-insensitive string comparer? Oh, man.
    Join that with Java allowing distinct variables which name is just different on casing on the same scope and you get WTF#2. (Thanks God my main job uses Delphi, learning Java is just a hobby).

    The rest:
    Very well written article!! Congrats.

  29. Claudion Says:

    I think you are a n00b. The code you present is an every-day occurrence in the industry. I bet your code from a few years back sucks just as bad.

    Quit hating, and fix it, DUMBASS!

  30. cherouvim Says:

    @Claudion: I live for comments such as yours. Thanks a lot :)

  31. Alpha Says:

    I pretty much wonder how you can show a issue with “Prod” environment in LOL#1 with the rest of the codebase.

    That should have not made into prod at all.

    No way.

    No.

    At all.

  32. cherouvim Says:

    @Alpha: I just find it funny when there are three clearly defined environments (dev, staging, prod) to have the application deployed in a context name with the word ‘Prod’ everywhere (dev, staging and prod).

  33. Lex Says:

    Unfortunately the stuff mentioned in the article isn’t that rare.

    I’ve been on a project where all those things could be found and then some. Like using String for everything instead of integers, booleans, etc. Not using any kind of transactions, not using primary keys and foreign keys in the DB. Spreading decissions that should be a simple if/else over 10 lines on a JSP with a hundred lines of HTML interweaved making it totally unclear what it actually is that’s being tested for. Etc etc..

  34. PhiLho Says:

    I fear a bit part of the code at my work place looks like this…
    Part of the problem is legacy: the code has been hacked over the years by people not necessarily knowing it in deep, sometime newbies (like I was, in the Java field at least, 5 years ago), often hacking a quick fix instead of finding a root cause and refactoring…
    So, the code being old, we still find some Hashmaps and Vectors, not everything have been generified yet, you will find lot of methods building data in a Collection, then converting it to an array to return it (and sometime reconverted to Collection!) because at the time, it was the only way to have a type safe return…
    We have, alas, monster classes (eg. 5000 lines, 166KiB!), extra long methods, bad design, obsolete stuff, and so on.

    The good thing is that we are aware of the issues and progressively try to fix them, but it is hard because of fear of breaking (unit tests are still sparse, and try doing unit test of monster classes depending on dozen of other complex objects…), and sometime because our saved documents are serialized versions of our objects, making hard to redesign (or we must keep legacy objects to read legacy documents).

    Oh, and we are French, and although all code, JavaDoc and documents are in English, we write often in broken English, hesitating between British and US spellings, etc. When a typo is found in a method in an interface used by dozens of classes, sometime we just don’t fix it, to avoid adding noise to the changelists (we have periods where such changes are allowed).

  35. cherouvim Says:

    @PhiLho: What you describe is natural and common. And it’s OK because there is *care* from your end. The problem with the project I took over is that there is no sign of care whatsoever.

  36. Jon Says:

    It would be nice if you could comment on how you would fix each problem, for the still noob programmers out there that can relate to writing this.

  37. OtengiM Says:

    There is nothing new in this article, Lots of projects from fortune 500 to the smallest companies have the same problem with really bad codebases and we have seen this many years, Right now still hot the OOP and Java but before it in the old times you could blame even assembly language programmers for bad codebase ROFL(Lots of spaghetti code you cant imagine), So I will suggest you fix the thing and move on. Good Luck with that because yes takes time to fix all that crap.

  38. Rajesh Says:

    yes its a nightmare looking at these kind of codes.

    add this to your list too

    boolean valid;

    if(someCondition.equals(satisfied){
    valid = true;
    }else{
    valid = false;
    }

    while that snippet in 1 line could be…

    valid = somecondition.equals(satisfied);

  39. cherouvim Says:

    @Jon: Most are self explanatory, and in any case, the curious will discover the truth easily.

    @OtengiM: A bad workman always blames his tools. Most of the problems have been fixed and I’ve moved on :) The post is the result of 6 months of working on this project.

    @Rajesh: I’ve presented cases with an exact same snippet as yours at http://blog.cherouvim.com/bad-and-good-code-snippets/

  40. Harry Says:

    This is typically happens when there either is no lead at all, or the lead himself is a douche as well.

    Sadly, it also happens when the cowboy coders delivering the crap showcased here just have big egos and/or can’t take any form of (constructive) criticism. Put those coders in an organization where it’s deemed wrong to “hurt the feelings of others” and you have a recipe for disaster.

    I’ve seen organizations where people produced crap, other people and the lead pointed it out to them, but they were called back by management since it introduced ‘conflicts’ :|

  41. Harry Says:

    @Rajesh

    Yeah, that’s a typical example. Another variant on that theme:

    boolean isNull;
    if (someObject == null) {
    isNull = true;
    } else {
    isNull = false;
    }

    while you can just use someObject == null down the line. And if you -really- need the separate boolean isNull = someObject == null, but in a lot of code I found introducing the extra boolean is only extra noise and the test is much clearer.

  42. kenny Says:

    Thanks for sharing. I have worked with legacy code in the industry myself (as an consultant). Issues like these are pretty common – unfortunately. I could not take it any more, and after three years i quit my job. I figured coding should be fun – so I started my own business. Although, you can learn a lot from legacy code (including what not to do).

    PS! The ones that say that GWT requires use of concrete classes check out “rpc.blacklist”.

  43. gorlok Says:

    I had worked on legacy code many times. I feel your pain. Never rewrite from scratch. Refactor and isolation are your freinds.

  44. dootzky Says:

    the author of this article is such a douche, i totally fcking hate guys full of shit like him.
    hey, dude – fuck you! :P

  45. Tunnel rat Says:

    Looks like typical slumdog code. After decades of h-1b marble-mouthed retards flooding American IT departments, this is what we are left with. The good news is that American techies will have plenty of work cleaning up this mess.

  46. cherouvim Says:

    @dootzky: Keep it up :)

    @Tunnel rat: Judging people by their nationality, immigration status and accent is very old fashioned and utterly unrelated to software. Care to join the 21st century?

  47. Code quality matters to the customers. A lot. « The Holy Java Says:

    [...] Update: I’ve stumbled upon a blog documenting some of the abominations you can meet in a no-quality code: The worst codebase I’ve seen in my life [...]

  48. Em Gee Says:

    This code could be from one of my Ex-Team members …

    Anyway, if you got the chance to leave, then leave that project!

  49. Mike Says:

    OUTSOURCING ! You get nothing but crap like this is you outsource your projects. Companies try this, and it fails every time !

  50. Winfield Says:

    This smells like the horrors of outsourcing.

    Run, run for your life.

  51. cherouvim Says:

    @Mike, @Winfield: This is not outsourcing. It’s an EU funded project (these things usually last 12 to 24 months) undertaken by a contractor (EU based; proper company) and the result is this. Later on the contractor loses the contract and another contractor needs to take over all existing projects so the fun starts.

  52. Ryan Says:

    WTF#3 – Complaining about comments not being in English seemed off-base. If you don’t speak English, why would you (or how could you) put comments in English?

    Then again, the actual project isn’t named other than being “EU”, and without that background information, WTF#3 could be spot on.

    Was English documentation/comments an expectation for the project?

    Anyways, an interesting article. :)

  53. cherouvim Says:

    @Ryan: Comments in code were not a delivery requirement.

    On the other hand, since this was a long running (expensive) EU project with 100% English UI which required full source code delivery together with hundreds of pages of (word doc) project management and technical documentation, I find it completely problematic and disconnected to have non English comments in the hardest areas of the code. It wasn’t a very big deal (translators today are pretty usable) but definitely worth mentioning.

  54. Sebs Says:

    It’s so easy to bash the poor programmer(s) who had to do this project and in general I miss the notion of delivering solutions or fixes for the problems.
    “not as a rant, but as a case study of what is possible to be delivered in the software industry” … I do not believe this. If it would be really in this sense, there would be more qualitative Information on all the stuff.
    This is just a elaborated rant that will not help anybody. Although it is delivered faster than the knowledge that is required to fix THIS project.

  55. Gary Furash Says:

    If you have time at some point, I agree with several of the statements above that it would be terrific to add the anti-pattern issue, the correct pattern, and the refactored solution. While I know you’re under no obligation to do so, it would be very helpful to new-to-Java programmers or people that aren’t familiar with all of the sub-components you’ve identified. Thank you.

  56. cherouvim Says:

    Edited original post and added solutions to all issues. Thanks all for your comments.

    @Sebs: Allow me to say that the poorest of all programmers in this case is the one who takes over such codebase. Also, I think that the post did help a lot of people so it was worth the bashing.

    @Gary Furash: Done

  57. Tom Says:

    In a previous job, I had to work with outsourcing companies for these kind of things myself. In that case, Romanian and Indian ones. Let’s just say they could do a good job in typing in codes, but we only let them develop once – that did have a reason…
    Interesting to see though!

  58. Carolien Says:

    I have got the same experience as Tom: we also used to work with Indian outsourcing companies which was a drama! In my case, we worked with packagers, but the work was done terribly and we needed to have it re-done alltogether, which costs even more, of course…

  59. Blog Says:

    Very good.

Leave a Reply

You must be logged in to post a comment.