CodeSOD: The Part Version

Once upon a time, there was a project. Like most projects, it was understaffed, under-budgeted, under-estimated, and under the gun. Death marches ensued, and 80 hour weeks became the norm. The attrition rate was so high that no one who was there at the start of the project was there at the end of the project. Like the Ship of Theseus, each person was replaced at least once, but it was still the same team.

Eric wasn't on that team. He was, however, a consultant. When the project ended and nothing worked, Eric got called in to fix it. And then called back to fix it some more. And then called back to implement new features. And called back"

While diagnosing one problem, Eric stumbled across the method getPartVersions. A part number was always something like "123456-1", where the first group of numbers were the part number itself, and the portion after the "-" was the version of that part.

So, getPartVersions, then, should be something like:

String getPartVersions(String part) { //sanity checks omitted return part.split("-")[1];}

The first hint that things weren't implemented in a sane way was the method's signature:

 private List<Integer> getPartVersions(final String searchString)

Why was it returning a list? The calling code always used the first element in the list, and the list was always one element long.

 private List<Integer> getPartVersions(final String searchString) { final List<Integer> partVersions = new ArrayList<>(); if (StringUtils.indexOfAny(searchString, DELIMITER) != -1) { final String[] splitString = StringUtils.split(searchString, DELIMITER); if (splitString != null && splitString.length > 1) { //this is the partIdentifier, we make it empty it so it will not be parsed as a version splitString[0] = ""; for (String s : splitString) { s = s.trim(); try { if (s.length() <= 2) { partVersions.add(Integer.parseInt(s)); } } catch (final NumberFormatException ignored) { //Do nothing probably not an partVersion } } } } return partVersions; }

A part number is always in the form "{PART}-{VERSION}". That is what the variable searchString should contain. So, they do their basic sanity checks- is there a dash there, does it split into two pieces, etc. Even these sanity checks hint at a WTF, as StringUtils obviously is just wrappers around built-in string functions.

Things get really odd, though, with this:

 splitString[0] = ""; for (String s : splitString) //"

Throw away the part number, then iterate across the entire series of strings we made by splitting. Check the length- if it's less than or equal to two, it must be the part version. Parse it into an integer and put it in the list. The real "genius" element of this code is that since the first entry in the splitString array is set to an empty string, Integer.parseInt will throw an exception, thus ensuring we don't accidentally put the part number in our list.

I've personally written methods that have this sort of tortured logic, and given what Eric tells us about the history of the project, I suspect I know what happened here. This method was written before the requirement it fulfilled was finalized. No one, including the business users, actually knew the exact format or structure of a part number. The developer got five different explanations, which turned out to be wrong in 15 different ways, and implemented a compromise that just kept getting tweaked until someone looked at the results and said, "Yes, that's right." The dev then closed out the requirement and moved onto the next one.

Eric left the method alone: he wasn't being paid to refactor things, and too much downstream code depended on the method signature returning a List<Integer>.