CodeSOD: The Map you Pay For

Soraya's company recently decided to expand the payment options that they support. This meant integrating "Inipay", Initech's API for handling payment processing globally. This particular API is open sourced, which means that Soraya was able to investigate exactly how the sausage was made.

Many of the classes were tagged as being @author auto create. In fact, there were about 2,000 such classes, all nearly identical aside from a few minor differences. What got Soraya's attention though was that each of them referred to InipayObject and InipayHashMap. Re-implementing standard classes is always a concern.

public class InipayHashMap extends HashMap<String, String>

That right there is a bit menacing. I'm actually a big fan of aliasing complex generic types into simpler classes, but a HashMap<String, String> tells me that we're looking at stringly typed code.

public String put(String key, Object value) {String strValue;if (value == null) {strValue = null;} else if (value instanceof String) {strValue = (String) value;} else if (value instanceof Integer) {strValue = ((Integer) value).toString();} else if (value instanceof Long) {strValue = ((Long) value).toString();} else if (value instanceof Float) {strValue = ((Float) value).toString();} else if (value instanceof Double) {strValue = ((Double) value).toString();} else if (value instanceof Boolean) {strValue = ((Boolean) value).toString();} else if (value instanceof Date) { DateFormat format = new SimpleDateFormat(InipayConstants.DATE_TIME_FORMAT); format.setTimeZone(TimeZone.getTimeZone(InipayConstants.DATE_TIMEZONE));strValue = format.format((Date) value);} else {strValue = value.toString();}return this.put(key, strValue);}

So, this is how they overload the put method. Fun fact: if you invoke toString on a String object in Java, it returns itself. Which means nearly all of the instanceof checks are 100% unnecessary. I say nearly, because while you could call Object.toString on pretty much everything, Dates do need some special care and handling. Still, I'm skeptical that this code belongs in your HashMap implementation.

Note, the last line calls the base class's put, but" no, not really. We overrode that, too.

public String put(String key, String value) { if (StringUtils.areNotEmpty(key, value)) {return super.put(key, value);} else {return null;}}

The real magic here is that if key and value don't meet whatever criteria areNotEmpty requires (presumably not null, and not empty strings), we silently ignore them. We don't fail. We don't raise an exception. We just silently ignore it. Worse, the put method in Java's Maps should return the old value, if there was one, and null otherwise, so this doesn't even fit the contract of the interface it's implementing, and thus doesn't behave like a HashMap should.

In practice, these InipayHashMap objects are used as part of every one of the 2,000 request objects, meant to store "user defined values". In other words, it's an uncapped pile of stringly typed data offered to end users to do whatever they please with it, making it maximally Enterprisey.

[Advertisement] ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!