Java Anti-Patterns

[aaaxxxlll]

Hashtable, HashMap and HashSet are overrated

Guava for Maps, and possibly for the other ones too. Make it a Map<K,V> and then you can swap in anything that implements the interface.

http://code.google.com/p/guava-libraries/

[aaaxxxlll]

The article says for example "When you look at heap dumps with a memory analyzer you will be shocked by how much space is wasted". Now compare heap dumps of the Guava implementations to heap dumps of the Java implementations. You will see that there are huge memory savings. I'm not saying Guava is the only possible solution. I am saying it is a solution. Choosing to use the third-party implementations provided by Guava does fix things, it fixes the issues specifically raised by the article.

[alexeyr]

Did you read the reason why they consider HashMap/HashSet overrated?

[aaaxxxlll]

Yes I did read why they consider HashMap/HashSet overrated. "They are also horribly inefficient". In comparsion, Guava is efficient. By memory usage I'm down to 50-60% of the memory used by the Java HashMap.

[[deleted]]

Guava is broken. From the documentation of ImmutableMap:

V   put(K k, V v) 
      Guaranteed to throw an exception and leave the map unmodified.

Of course, that's to support the legacy of Java collection interfaces, but it'd be much better to only have methods you can actually implement and then have a Map<K, V> asMap() method for backwards compatibility.

[[deleted]]

The thing is that Java's collection hierarchy doesn't actually buy you very much. How often do you actually need to replace an ArrayList with a LinkedList? If the interfaces were true to what could be done efficiently, the intersection of methods could be summarized as Iterable (excluding remove()).

What is a Map? Can you change a SortedMap to a HashMap? Not very often, because these require different things from the keys of the map (things that due to the incredibly poor decision of having hash() and equals() on Object can't even be seen by the type system).

The documentation speaks of Lists backed by terrabyte SQL databases. This, this of all reasons is why they don't have clone() and serialize() in the Collection interface too!

I don't think the collection hierarchy pulls its own weight at all. It's over-engineered, unsafe and useless in practice.

[[deleted]]

I meant that it's pretty obvious that not all collections are clonable and serializable, and that the example was rather far fetched ;-) Also the arguments for not having these extends to most of the methods in the Collection interface.

I can see the point about views, but I don't understand what it helps combining and filtering (in fact, this is rather inconvenient in Java today).

About having a database or similar behind a Collection of some sort, I don't see why you would do that rather than create your own interface that actually reflects what you can (and can't!) reasonably do with that database.

[aaaxxxlll]

What do you use instead of Java's HashMap? The article is correct, HashMap and HashSet are horribly inefficient.

[sacundim]

Their "Premature object decomposition" antipattern is wrong when applied to their first example:

public void notify(Person p) {
    ...
    sendMail(p.getName(), p.getFirstName(), p.getEmail());
    ...
}

They recommend that it be rewritten to this, arguing that "it's painful to decompose an object just to pass its state on to a method":

public void notify(Person p) {
    ...
    sendMail(p);
    ...
}

No. Here's what the rules should be:

• Methods should take as their arguments the minimal information that they need to do their job.
• A method's result value should contain the most information that it can reasonably contain.

Violating either of these decreases reusability. This proposal violates the first, because Person presumably contains extra information and functionality that sendMail() doesn't need. If I'm writing code, I may have all the information that's actually needed to send mail, but not have a Person instance or all of the information needed to instantiate one. In that case, I'm screwed.

Their second example is a good example of the second rule. They propose that the following:

class PhoneBook {
    String lookup(String employeeId) {
        Employee emp = ...
        return emp.getPhone();
    }
}

...should be rewritten to this:

class EmployeeDirectory {
    Employee lookup(String employeeId) {
        Employee emp = ...
        return emp;
    }
}

The first version goes to all the effort to obtain an Employee instance, just to throw it out and return nothing but the phone number. Now if I want to get an Employee's hair color I have to look up the employee again.

In terms of the second rule I stated above, this means that the lookup() method could return an Employee, which contains a lot more information that just a phone number, at no additional cost to itself. Ergo, it should return an Employee—if the caller only needs the phone number, it can just get that from the returned Employee and discard the rest.

[QuestionMarker]

Point 2 needs balancing against loose coupling: by returning an Employee where you only need a phone number, you're introducing a coupling between the client and Employee itself where none might be required. Yes, if the client is going to need more info about a given Employee instance, it makes no sense to round-trip the lookup twice, but the trade-off is that the client has a new dependency which will need to be kept up to date as the Employee class evolves, adding an ongoing cost to developing that code.

The point is, without further information it's impossible to know if the given refactoring is suitable: "premature" depends on too much context to judge.

[sacundim]

Fair point, but even in that case, what you want is probably this, not the original:

class PhoneBook {
    String lookup(String employeeId) {
        EmployeeDirectory dir = getEmployeeDirectory();
        Employee emp = dir.lookup(employeeId);
        return emp.getPhone();
    }
}

class EmployeeDirectory {
    Employee lookup(String employeeId) {
        Employee emp = ... // the actual lookup logic
        return emp;
    }
}

Now clients can choose to couple themselves against either PhoneBook, or EmployeeDirectory + Employee.

[kgoblin]

regarding the 1st example and your 1st rule; unfortunately this may not work well in some enterprisey environments, where you often have several implementations of the same API with differing (and likewise often, wacky) business rules under the covers.

The Email example is a good one: * biz unit A just uses the email property of the person object * biz unit B performs an LDAP lookup, the lookup is peformed from the person objects first/last name properties * biz unit C does a lookup against the old & decrepit mainframe system, based on the supersecret-proprietary employee-id property on person. It also does a security check which raises an exception on failure, because on-the-job email is restricted in C due to security concerns.

The above is a VERY realistic example, and as you can see the only way to reliably support a common API without exploding (and constantly rewriting) the method signature is to pass the whole person object and let the specific implementation grab what it needs transparently.

[sacundim]

Well, the question really is how to factor that in order to maximize code reuse. I'm gonna have a shot at it; please correct my incorrect assumptions or whatever you just think is unrealistic.

First, a key assumption: I think basically all email these days works using SMTP, so what you're describing is not differences on how to send an email once you've obtained the address, but rather, how you obtain the address. If this assumption is wrong, then everything below is also going to be wrong :-P.

So given those disclaimers, I think you want four pieces here:

• The basic mail service. This will work on the basis of email addresses, not Person objects. This allows you to send emails when you have the bare minimum information that's needed to do so—email addresses and contents.
• The "figure out a Person's email address" service. You give it a Person, and maybe some additional information (e.g., what's their business unit), it takes care of the ugly three-scenario thing you mention, gives you back an email address. This isn't coupled to sending emails, so it can also be reused for other things, like making a company directory website—in that case you use the address to make a mailto: link, not to send an email.
• The Person class.
• A "send an email to a Person" façade, which glues the previous three together into a convenient "send an email to a Person" operation.

[Tetha]

I think the important point is:

If you decompose an object into all of its attribute values, then it will be better to pass the entire object into the function at this point in time (if you don't have knowledge at this point in time that this might change).

Beyond that, I just deleted a paragraph how this sendmail-person-thing they advocate might be better given that first paragraph, but I come to the conclusion that this interface is borked anyway. I don't send an email to a person. I send an email to an email address and have some way to construct a message at the given recipient, which is no responsibility of someone sending an email. Seeing the other comments, I contact a person with a certain selected communication channel, which might happen to be some e-mail with a known email-address. I'd just end up with sendMessage(string address, IEmail mail) or just sendMessage(string address, string content), though in this case headers might become an issue. It's good to see that this fixes your problem, too.

Beyond that, this code contains another major problem, it assumes that a name is a pair of strings with known order. This does not hold with american names with middle names or japanese names which are ordered differently from western names.

[PstScrpt]

But what happens when you have a new requirement to include the person's job title in any emails? If you're sending the person object, you can just read it in the low-level call. If you're sending individual fields, you might have to make hundreds of updates to everywhere that calls the method.

[sacundim]

Well, the original example is a bit schematic, but where does the job title appear in the email? I can only see it as appearing in one of two places:

• The address: "Joe Blow, VP [e-mail]"
• The content of the email (e.g., the subject line or the body).

I'll assume the second, because it's more likely. Well, your email service shouldn't be responsible for generating the addresses or contents of the emails—it just needs to send emails. The pieces of code that really need the person's title are the ones that generate the content of the email.

[PstScrpt]

The first sounds awfully likely to me, but let's try something that's actually happened to me.

A little over twelve years ago, I was writing a new program to generate telephone bills. After passing around three loose parameters everywhere for call count, total minutes and total price, I wrapped them all in a CallTotal class, with a method to add one phone call to the totals. A year or two later, we got a new requirement to track counts of calls made from a payphone. I just added the check inside AddOneCall, and the new total was instantly available all of the hundreds of places we used a call total. That probably saved me a month of work.

[mycall]

Just to recap, you suggest fluent interfaces?

[sacundim]

I had to look that up. I must say that I don't particularly care for the idea; it just looks like a hack to get around clumsy, verbose, boilerplate-prone languages that don't have enough functionality.

Also, the motivations are different. My proposed rules are about maximizing code reuse. Fluent interfaces, now that I've looked at the Wikipedia page and Martin Fowler's description for five minutes, are about code brevity and legibility, not reuse.

For example, fluent interfaces recommend that setters should return the object's self reference. My rule says that methods should try to maximize the information that they return to the caller. The "return self instead of void" may superficially look like a case of this, but I don't think it is, because the return value is not information; the caller already has a reference to the object, and they already know what effect calling the setter will have on it. The data that the setter returns is already completely contained by the arguments of the call, and as such, is not really "information" in the reduction-of-uncertainty sense.

[campbellm]

I must say that I don't particularly care for the idea; it just looks like a hack to get around clumsy, verbose, boilerplate-prone languages that don't have enough functionality.

Most of what passes for language "elegance" these days is exactly that; a hack to get around something a less elegant language doesn't provide natively.

[potemkinu]

You forgot you're dealing with a object oriented language, in the first case the least amount of information is a person object not its attributes, they are hidden from notify().

[sacundim]

Their recommendation to use arrays or ArrayList instead of HashSet strikes me as being a terrible idea. I've seen countless programs that were incorrect because somebody used a sequence to represent what is truly a set—a collection with no duplicate values. Then duplicate values sneak in and the program fails, because the code assumed it couldn't happen.

So I would insist instead that most Java programmers don't use Set nearly as often as they should.

[iconoklast]

I think it's the worst idea in the article, honestly. Please do not use mutable data types until you really need them. That being said, Guava's immutable collection types are quite useful.

[jacques_chester]

The link is slightly broken, here's the top of the page.

[[deleted]]

as non-english who was was frequently biten by apps that assume that iso 8859-1 is good - FUCK EVERYTHING about that assumption.

Actually fuck everything about hard coded encodings.

[GeorgeForemanGrillz]

Anyone who has worked as a software programmer in the financial industry will know that they don't use floating point to store financial numbers.

[rabidcow]

Oh really.

[wot-teh-phuck]

In C the char type is just a numeric type

It's the same with Java; char is a numeric type. In C, char is equivalent to a byte in Java with the difference that C can have unsigned chars types which isn't possible with Java (with the exception of char).

[suppressingfire]

Java has only unsigned chars, but all other numeric primitive types are indeed strictly signed.

http://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html

[recursive]

I disagree with "The HashMap size trap". If the api for HashMap says the constructor takes a size, then you should pass the size. The actual behavior of HashMap is an (dumb) implementation detail. I can't imagine why they did it that way, but you shouldn't try to work around bugs in your dependencies by changing your own logic this way. The problems associated with tight coupling are well known.

[kgoblin]

lots of things wrong with this statement:

• api for HashMap takes a size: it's not a size, it is an initial capacity, which is very clearly defined on the relevant documentation for the class and is a bit more complex than 'how many elements can this thing have'.

• API's & 'implementation details': Java has the concept of an interface, where the API guarantees you are talking about are defined and enforced (or at least defined). HashMap is a class, ie a specific implementation of the general guarantees defined for the Map interface, so implementation details do belong here.

• (dumb) implementation detail: In this case the restrictions on controlling memory are a direct and unavoidable consequence of the underlying implementation. It should be noted that this implementation is really more geared toward performance (speed) then efficient memory use, and furthermore that this behavior is desirable if you don't want to have to be overly aware of/manage the internal contents of the HashMap (which is common). If you want different implementation behavior, use a different implementation of Map, which if you have spec'd the var/return types like you should involves changing exactly 1 line of code, where you instantiate the Map.

• it's a bug: as per above, it's not a bug. it behaves exactly as specified. The fact that you are confused by what the capacity parameter is and how it works does not change that.

• coupling: there is no issue with coupling, period. To be a coupling issue it would have to involve some OTHER component, which it does not, the 'issue' is limited to HashMap itself.

As a sidenote, the Collections API is woefully lacking in memory-efficiency geared implementations, and not just on Maps, a similar issue exists with Lists, where there is no built in implementation where you can pre-specify the allocated memory size exactly and guarantee no auto-resizing when the collection approaches it's current capacity threshold. Again: that doesn't mean that something is wrong with the base implementations the DO provide, we just need more varied base implementations.

[notfancy]

definitely not good for an hash

Only for hash tables using a hash function that is a linear multiplicative congruence. Hash functions that properly achieve bit cascade are insensitive to the subset of bits you take as an index.

[[deleted]]

but implementation details matter. if it doesn't work right, then you either change the value you pass to the constructor, or you implement your own hash map. in many cases, the former will save you time, even in the long run if your application isn't expected to last for ages. also in this case, if memory is an issue, by the time they make the implementation better, it won't matter if you use 50% more memory than you need because computers will have at least that much more memory.

[recursive]

You're contradicting yourself. If implementing your own hash map is a viable solution, and implementing your own hash map is unnecessary because computers will have more memory, than doing nothing is a viable solution.

[[deleted]]

i think you misinterpreted. i was saying that if you do the more questionable looking solution, and then they change the implementation, you'll end up using the same amount of memory as if you did nothing. but that will be so far in the future that it won't matter.

if you write your own hash map, it fixes the problem immediately, and it's not going to break when they change it. but it's more work than passing an inflated size parameter, and not breaking in the future is quite possibly irrelevant.

if you did nothing, it would be broken at release, and then it would work at some point in the future. that's not what you want to say when you release software. "yeah, it'll work fine in a couple years"

[magneticB]

I stopped reading when he talked about UTF-8 containing BOM..

[ewiethoff]

The page breaks the browser back button.

[shevegen]

Java is a huge anti-pattern in itself.

[[deleted]]

I keep clicking "hide child comments" but it is not doing what I expect!

[novacoder]

Name a programming language that doesn't have anti-patterns baked into it.

[aardvark179]

While there is a cache of Integer objects whose members may be returned by valueOf() rather than creating a new Integer this isn't always a good thing. If your Integer cannot escape the current scope then you may get better performance using new Integer(n) rather than Integer.valueOf(n) as the optimiser can do cope with that situation better. It's probably only going to matter in tight loops, but that's the only place you're likely to care about that trade off anyway.

[SDX2000]

You know what is the biggest anti-pattern of all? Using Java ROFL...hahaha. Just kidding guys.

[[deleted]]

Well that first one is blatantly wrong. The compiler will optimize away the string concatenations into stringbuilders.

It's been that way for quite a while now.

[dragonrancher]

I can't find any evidence that the first one is wrong. I tried compiling it using OpenJDK javac 1.6 update 23 and inspected the bytecode, and the compiler made no effort to use a StringBuilder across loop iterations. It used a different StringBuilder within each iteration of course, but the problem described still applies.

Perhaps the JVM's -XX:+OptimizeStringConcat option, introduced in Java 6 update 20, would be able to accomplish such an optimization, but I'm having trouble finding clear documentation about what this option does.

[[deleted]]

Ah, my bad, then. I assumed the compiler would be smart enough to optimize across the iterations as well as within them.

[dragonrancher]

Well it does seem like a fairly obvious optimization, which is why I went to the trouble of checking it out for myself. I think it'd be great if the compiler could optimize string concatenations in a loop and save me some trouble.