CodeSOD: The National Integration

Sergio works for a regional public administration. About a decade ago, the national government passed some laws or made some regulations, and then sent a software package to all the regional administrations. This software package was built to aggregate reports from the local databases into a single, unified, consistent interface on a national website.

Of course, each regional group did things their own way, so the national software package needed to be customized to work. Also, each regional administration had their own reporting package which already did some of this, and which they liked better, because they already knew how it worked. In the case of Sergio's employer, even more important: their organization's logo was prominently displayed.

Of course, there was also the plain old stubborness of an organization being told they have to do something when they really don't want to do that thing. In that situation, organizations have all the enthusiasm of a five year old being told to brush their teeth or eat their vegetables.

The end result was that the people tasked with doing the integration and customization didn't want to be doing that, and since the organization as a whole didn't want to do anything, they weren't exactly putting their top-tier resources on the project. The integration task was doled out to the people who literally couldn't be trusted to do anything else, but couldn't be fired.

Shockingly, national noticed a huge number of errors coming from their software, and after a few months of constant failures and outages, Sergio was finally tasked with cleaning up the mess.

 private ReportsWSStub stub = null;public ReportHelper() {try {if (null == stub) {// URL del WSString url = InicializacionInformes.URL_WS_CLIENTE_INFORMES;System.out.println("URL " + url);stub = (ReportsWSStub)new ReportsWSStub(url);log.info("Report's 'stub' has been initialized");log.info("URL for the Report's stub " + url);log.info("stub._getServiceClient() " + stub._getServiceClient());...}} catch (Exception e) {log.error(" Exception", e);System.out.println(" Exception" + e.getMessage());}}

Here, we have the constructor for the ReportHelper class. It might be better named as a wrapper, since its entire job is to wrap around the ReportsWSStub object. It's important to note that this object is useless without a valid instance of ReportsWSStub.

With that in mind, there's all sorts of little things which pop out. First, note the if (null == stub) check. That's a backwards way to write that, which sort of sets the tone for the whole block. More than just backwards- it's pointless. This is the constructor. The stub variable was initialized to null (also unnecessary). The variable can't be anything but null right now.

Then we go on and mix log calls with System.out.println calls, which this is a JEE component running on a webserver, so those printlns fly off to the void- they're clearly left over debugging lines which shouldn't have been checked in.

The key problem in this code, however, is that while it logs the exception it gets when trying to initialize the stub, it doesn't do anything else. This means that stub could still be null at the end of the constructor. But this object is useless without a stub, which means you now have an unusuable object. At least it hopefully gives you good errors later?

public Report getReport(String id , String request) throws IOException{...try {stub._getServiceClient().getOptions().setProperty(Constants.Configuration.ENABLE_MTOM,Constants.VALUE_TRUE);stub._getServiceClient().getOptions().setProperty(Constants.Configuration.CACHE_ATTACHMENTS,Constants.VALUE_TRUE);stub._getServiceClient().getOptions().setProperty(Constants.Configuration.FILE_SIZE_THRESHOLD, "10000");stub._getServiceClient().getOptions().setTimeOutInMilliSeconds(100000);stub._getServiceClient().getOptions().setProperty(org.apache.axis2.transport.http.HTTPConstants.CHUNKED,Boolean.FALSE);requestInforme.setGetReport(requestType);if(stub == null){log.info("##################### Stub parameters are null!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");}else{log.info("#####################Stub parameters are not null!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"); } responseInformes = stub.getReport(requestInforme); } catch (Exception e) { //... } }

As you can see, they've helpfully added a if (stub == null) check to see if stub is null, and log an appropriate message. And if stub isn't null, we have that else block to log a message stating that the parameters are not null, but with the same enthisuasm and exclamation points of the error. And of course, we're only trying to log that the stub is null, not, y'know, do anything different. "We know it's a problem, but we're doing absolutely nothing to change our behavior."

That's okay, though, because if stub is null, we'll never log that error anyway, because we'll get a NullReferenceException instead.

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!