kengr

Posted by Remy Porter

https://thedailywtf.com/articles/the-map-to-your-confession

Today, Reginald approaches us for a confession.

He writes:

I've no idea where I "copied" this code from five years ago. The purpose of this code was to filter out Maps and Collections Maybe the intention was to avoid a recursive implementation by an endless loop? I am shocked that I wrote such code.

Well, that doesn't bode well, Reginald. Let's take a look at this Java snippet:

/**
 * 
 * @param input
 * @return
 */
protected Map rearrangeMap(Map input) {
	Map retMap = new HashMap();

	if (input != null && !input.isEmpty()) {

		Iterator it = input.keySet().iterator();
		while (true) {
			String key;
			Object obj;
			do {
				do {
					if (!it.hasNext()) {
					}
					key = (String) it.next();

				} while (input.get(key) instanceof Map);

				obj = input.get(key);

			} while (obj instanceof Boolean && ((Boolean) obj).equals(Boolean.FALSE));

			if (obj != null) {
				retMap.put(key, obj);
				return retMap;
			}
		}
	} else {
		return retMap;
	}
}

The first thing that leaps out is that this is a non-generic Map, which is always a code smell, but I suspect that's the least of our problems.

We start by verifying that the input Map exists and contains data. If the input is null or empty, we return it. In our main branch, we create an iterator across the keys, before ethering a while(true) loop. So far so bad

Then we enter a pair of nested do loops. Which definitely hints that we've gone off the edge of the map here. In the inner most loop, we do a check- if there isn't a next element in the iterator, we… do absolutely nothing. Whether there is or isn't an element, we advance to the next element, risking a NoSuchElementException. We do this while the key points to an instance of Map. As always, an instanceof check is a nauseating code stench.

Okay, so the inner loop skips across any keys that point to maps, and throws an exception when it gets to the end of the list.

The surrounding loop skips over every key that is a boolean value that is also false.

If we find anything which isn't a Map and isn't a false Boolean and isn't null, we put it in our retMap and return it.

This function finds the first key that points to a non-map, non-false value and creates a new map that contains only that key/value. Which it's a hard thing to understand why I'd want that, especially since some Map implementations make no guarantee about order. And even if I did want that, I definitely wouldn't want to do that this way. A single for loop could have solved this problem.

Reginald, I don't think there's any absolution for this. Instead, my advice would be to install a carbon monoxide detector in your office, because I have some serious concerns about whether or not your brain is getting enough oxygen.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!

https://thedailywtf.com/articles/the-map-to-your-confession