CodeSOD: Switching Jobs

If you have an object, say a Job, which can be in multiple states, you'll frequently need to execute different code, depending on the state. Enter the ever useful switch statement, and all those problems are solved.

Or are they? Reva found this Java code. Enjoy your 112 line switch statement:

@Overridepublic void processEvent(Job job, Error reason) {switch (job.getState()) {case NEW:case WAITING:case STARTED:case ERROR:String message = null;switch (reason) {case ERROR: // release job (no break)case ERROR_OTHER_1:// set message to some valuecase ERROR_OTHER_2:SomeEntity someEntity = service.get(job.getSomeEntityId());if (!someEntity.getStateReference().getLocationReference().getLocationTypeReference().getName().equals(Constants.SOME_CONSTATNT)) {// Set Job and then breakbreak;}// set message (no break)case ERROR_OTHER_3:// set message (no break)case ERROR_OTHER_4:// set message (no break)case ERROR_OTHER_5:// set message inline if (no break)case ERROR_OTHER_6:if (null != message) {// Change state of someEntity}if (null != job.someReference()) {// set job to another reference} else {//cancel jobif (null != job.getSuccessor() && null != job.getSuccessor().someReference()) {SomeEntity otherSomeEntity = service.get(job.getSomeEntityId());if (!otherSomeEntity.getStateReference().getLocationReference().getLocationTypeReference().getName().equals(Constants.SOME_OTHER_CONSTANT)) {if (OtherConstants.BLOCKED.equals(job.getSuccessor().getState())) {// set state of successor reference}} else {job.setSuccessor(recursiveSetNewLoadOrCancel(job.getSuccessor(), reason));}}// set job to something elseif (new Integer(1).equals(job.getSource().getLength())) {// set temp Stringtry {// try something with tmp String} catch (Exception e) {log.warn("Exception", e);}}// Here an application event is firedtry {load = someService.get(job.getSomeEntityId());} catch (Exception e) {log.warn(e, e);} // remove some messagesif (load != null) {String text = // some text describing error;log.warn(text);}}break;case ERROR_OTHER_7:// save errorbreak;case ERROR_OTHER_8:// set job stateif (null != job.getSuccessor() && null != job.getSuccessor().getRequestPosition()) {if (// more checks) {if (// more checks) {// set job state to another state}} else {job.setSuccessor(recursiveSetNewUnitLoadOrCancel(job.getSuccessor(), reason));}}// get another job referenceif (new Integer(1).equals(job.getSource().getLength())) {// get a string based on jobtry {// remove a message and save} catch (Exception e) {log.warn("Exception", e);}}// Fire another event// save an error messagebreak;default:// nothing elselog.warn(String.format(" %s no behavior defined for %s", reason, job));}break;case BLOCKED:default:// nothing else}}

Now, Reva has stripped out a lot of the details, but we can definitely see the core flow, with the main notable feature being an absence of breaks. A lot of missing break statements. This means that the states of NEW, WAITING, and STARTED all go down the error handling path, though presumably reason won't have a value in those cases, so it just dumps to printing a warning in the log.

We have a switch in a switch, for handling errors, which is always a treat. And even within those switches, we have loads of conditional statements. Tracing through this code is nigh impossible.

If I had to pick a favorite line in this knot of code, though, it would have to be this one:

(new Integer(1).equals(job.getSource().getLength()))

It's not wrong, but nobody writes code this way. There's no reason to do this this way. It's just the most obtuse way to express this idea, and I assume that's intentional obfuscation.

And obfuscation is what Reva suspects was the point:

I don't know how. I don't know why. It seems a previous co-worker wanted to make himself in-expendable.

With any luck, that co-worked has been expended.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!