Mapping between types can create some interesting challenges. Michal has one of those scenarios. The code comes to us heavily anonymized, but let’s see what we can do to understand the problem and the solution.

There is a type called ItemA. ItemA is a model object on one side of a boundary, and the consuming code doesn’t get to touch ItemA objects directly, it instead consumes one of two different types: ItemB, or SimpleItemB.

The key difference between ItemB and SimpleItemB is that they have different validation rules. It’s entirely possible that an instance of ItemA may be a valid SimpleItemB and an invalid ItemB. If an ItemA contains exactly five required fields importantDataPieces, and everything else is null, it should turn into a SimpleItemB. Otherwise, it should turn into an ItemB.

Michal adds: “Also noteworthy is the fact that ItemA is a class generated from XML schemas.”

The Java class was generated, but not the conversion method.

public ItemB doConvert(ItemA itemA) {
  final ItemA emptyItemA = new ItemA();
  emptyItemA.setId(itemA.getId());
  emptyItemA.setIndex(itemA.getIndex());
  emptyItemA.setOp(itemA.getOp());

  final String importantDataPiece1 = itemA.importantDataPiece1();
  final String importantDataPiece2 = itemA.importantDataPiece2();
  final String importantDataPiece3 = itemA.importantDataPiece3();
  final String importantDataPiece4 = itemA.importantDataPiece4();
  final String importantDataPiece5 = itemA.importantDataPiece5();

  itemA.withImportantDataPiece1(null)
          .withImportantDataPiece2(null)
          .withImportantDataPiece3(null)
          .withImportantDataPiece4(null)
          .withImportantDataPiece5(null);

  final boolean isSimpleItem = itemA.equals(emptyItemA) 
&& importantDataPiece1 != null && importantDataPiece2 != null && importantDataPiece3 != null && importantDataPiece4 != null;

  itemA.withimportantDataPiece1(importantDataPiece1)
          .withImportantDataPiece2(importantDataPiece2)
          .withImportantDataPiece3(importantDataPiece3)
          .withImportantDataPiece4(importantDataPiece4)
          .withImportantDataPiece5(importantDataPiece5);

  if (isSimpleItem) {
      return simpleItemConverter.convert(itemA);
  } else {
      return itemConverter.convert(itemA);
  }
}

We start by making a new instance of ItemA, emptyItemA, and copy a few values over to it. Then we clear out the five required fields (after caching the values in local variables). We rely on .equals, generated off that XML schema, to see if this newly created item is the same as our recently cleared out input item. If they are, and none of the required fields are null, we know this will be a SimpleItemB. We’ll put the required fields back into the input object, and then call the appropriate conversion methods.

Let’s restate the goal of this method, to understand how ugly it is: if an object has five required values and nothing else, it’s SimpleItemB, otherwise it’s a regular ItemB. The way this developer decided to perform this check wasn’t by examining the fields (which, in their defense, are being generated, so you might need reflection to do the inspection), but by this unusual choice of equality test. Create an empty object, copy a few ID related elements into it, and your default constructor should handle nulling out all the things which should be null, right?

Or, as Michal sums it up:

The intention of the above snippet appears to be checking whether itemA contains all fields mandatory for SimpleItemB and none of the other ones. Why the original author started by copying some fields to his ‘template item’ but switched to the ‘rip the innards of the original object, check if the ravaged carcass is equal to the barebones template, and then stuff the guts back in and pretend nothing ever happened’ approach halfway through? I hope I never find out what it’s like to be in a mental state when any part of this approach seems like a good idea.

Ugly, yes, but still, this code worked… until it didn’t. Specifically, a new nullable Boolean field was added to ItemA which was used by ItemB, but had no impact on SimpleItemB. This should have continued to work, except that the original developer defaulted the new field to false in the constructor, but didn’t update the doConvert method, so equals started deciding that our input item and our “empty” copy no longer matched. Downstream code started getting invalid ItemB objects when it should have been getting valid SimpleItemB objects, which triggered many hours of debugging to try and understand why this small change had such cascading effects.

Michal refactored this code, but was not the first person to touch it recently:

A cherry on top is the fact that importantDataPiece5 came about a few years after the original implementation. Someone saw this code, contributed to the monstrosity, and happily kept on trucking.

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