Offensive programming

September 25, 2013

How to make your code more concise and well-behaved at the same time

Have you ever had an application that just behaved plain weird? You know, you click a button and nothing happens. Or the screen all the sudden turns blank. Or the application get into a “strange state” and you have to restart it for things to start working again.

If you’ve experienced this, you have probably been the victim of a particular form of defensive programming which I would like to call “paranoid programming”. A defensive person is guarded and reasoned. A paranoid person is afraid and acts in strange ways. In this article, I will offer an alternative approach: “Offensive” programming.

The cautious reader

What may such paranoid programming look like? Here’s a typical example in Java:

public String badlyImplementedGetData(String urlAsString) {
	// Convert the string URL into a real URL
	URL url = null;
	try {
		url = new URL(urlAsString);
	} catch (MalformedURLException e) {
		logger.error("Malformed URL", e);
	}

	// Open the connection to the server
	HttpURLConnection connection = null;
	try {
		connection = (HttpURLConnection) url.openConnection();
	} catch (IOException e) {
		logger.error("Could not connect to " + url, e);
	}

	// Read the data from the connection
	StringBuilder builder = new StringBuilder();
	try (BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
		String line;
		while ((line = reader.readLine()) != null) {
			builder.append(line);
		}
	} catch (Exception e) {
		logger.error("Failed to read data from " + url, e);
	}
	return builder.toString();
}

This code simply reads the contents of a URL as a string. A surprising amount of code to do a very simple task, but such is Java.

What’s wrong with this code? The code seems to handle all the possible errors that may occur, but it does so in a horrible way: It simply ignores them and continues. This practice is implicitly encouraged by Java’s checked exceptions (a profoundly bad invention), but other languages see similar behavior.

What happens if something goes wrong:

If the URL that’s passed in is an invalid URL (e.g. “http//..” instead of “http://…”), the following line runs into a NullPointerException: connection = (HttpURLConnection) url.openConnection();. At this point in time, the poor developer who gets the error report has lost all the context of the original error and we don’t even know which URL caused the problem.
If the web site in question doesn’t exist, the situation is much, much worse: The method will return an empty string. Why? The result of StringBuilder builder = new StringBuilder(); will still be returned from the method.

Some developers argue that code like this is good, because our application won’t crash. I would argue that there are worse things that could happen than our application crashing. In this case, the error will simply cause wrong behavior without any explanation. The screen may be blank, for example, but the application reports no error.

Let’s look at the code rewritten in an offensive way:

public String getData(String url) throws IOException {
	HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();

	// Read the data from the connection
	StringBuilder builder = new StringBuilder();
	try (BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
		String line;
		while ((line = reader.readLine()) != null) {
			builder.append(line);
		}
	}
	return builder.toString();
}

The throws IOException statement (necessary in Java, but no other language I know of) indicates that this method can fail and that the calling method must be prepared to handle this.

This code is more concise and if there is an error, the user and log will (presumably) get a proper error message.

Lesson #1: Don’t handle exceptions locally.

The protective thread

So how should this sort of error be handled? In order to do good error handling, we have to consider the whole architecture of our application. Let’s say we have an application that periodically updates the UI with the content of some URL.

public static void startTimer() {
	Timer timer = new Timer();
	timer.scheduleAtFixedRate(timerTask(SERVER\_URL), 0, 1000);
}

private static TimerTask timerTask(final String url) {
	return new TimerTask() {
		@Override
		public void run() {
			try {
				String data = getData(url);
				updateUi(data);
			} catch (Exception e) {
				logger.error("Failed to execute task", e);
			}
		}
	};
}

This is the kind of thinking that we want! Most unexpected errors are unrecoverable, but we don’t want our timer to stop because of it, do we?

What would happen if we did?

First, a common practice is to wrap Java’s (broken) checked exceptions in RuntimeExceptions:

public static String getData(String urlAsString) {
	try {
		URL url = new URL(urlAsString);
		HttpURLConnection connection = (HttpURLConnection) url.openConnection();

		// Read the data from the connection
		StringBuilder builder = new StringBuilder();
		try (BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
			String line;
			while ((line = reader.readLine()) != null) {
				builder.append(line);
			}
		}
		return builder.toString();
	} catch (IOException e) {
		throw new RuntimeException(e.getMessage(), e);
	}
}

As a matter of fact, whole libraries have been written with little more value than hiding this ugly feature of the Java language.

Now, we could simplify our timer:

public static void startTimer() {
	Timer timer = new Timer();
	timer.scheduleAtFixedRate(timerTask(SERVER\_URL), 0, 1000);
}

private static TimerTask timerTask(final String url) {
	return new TimerTask() {
		@Override
		public void run() {
			updateUi(getData(url));
		}
	};
}

If we run this code with an erroneous URL (or the server is down), things go quite bad: We get an error message to standard error and our timer dies.

At this point of time, one thing should be apparent: This code retries whether there’s a bug that causes a NullPointerException or whether a server happens to be down right now.

While the second situation is good, the first one may not be: A bug that causes our code to fail every time will now be puking out error messages in our log. Perhaps we’re better off just killing the timer?

public static void startTimer() // ...

public static String getData(String urlAsString) // ...

private static TimerTask timerTask(final String url) {
	return new TimerTask() {
		@Override
		public void run() {
			try {
				String data = getData(url);
				updateUi(data);
			} catch (IOException e) {
				logger.error("Failed to execute task", e);
			}
		}
	};
}

Lesson #2: Recovery isn’t always a good thing: You have to consider errors are caused by the environment, such as a network problem, and what problems are caused by bugs that won’t go away until someone updates the program.

Are you really there?

Let’s say we have WorkOrders which has tasks on them. Each task is performed by some person. We want to collect the people who’re involved in a WorkOrder. You may have come across code like this:

public static Set findWorkers(WorkOrder workOrder) {
	Set people = new HashSet();
	
	Jobs jobs = workOrder.getJobs();
	if (jobs != null) {
		List jobList = jobs.getJobs();
		if (jobList != null) {
			for (Job job : jobList) {
				Contact contact = job.getContact();
				if (contact != null) {
					Email email = contact.getEmail();
					if (email != null) {
						people.add(email.getText());
					}
				}
			}
		}
	}
	return people;
}

In this code, we don’t trust what’s going on much, do we? Let’s say that we were fed some rotten data. In that case, the code would happily chew over the data and return an empty set. We wouldn’t actually detect that the data didn’t adhere to our expectations.

Let’s clean it up:

public static Set findWorkers(WorkOrder workOrder) {
	Set people = new HashSet();
	for (Job job : workOrder.getJobs().getJobs()) {
		people.add(job.getContact().getEmail().getText());
	}
	return people;
}

Whoa! Where did all the code go? All of the sudden, it’s easy to reason about and understand the code again. And if there is a problem with the structure of the work order we’re processing, our code will give us a nice crash to tell us!

Null checking is one of the most insidious sources of paranoid programming, and they breed very quickly. Image you got a bug report from production - the code just crashed with a NullPointerException (NullReferenceException for you C#-heads out there) in this code:

public String getCustomerName() {
	return customer.getName();
}

People are stressed! What do you do? Of course, you add another null check:

public String getCustomerName() {
        if (customer == null) return null;
	return customer.getName();
}

You compile the code and ship it. A little later, you get another report: There’s a null pointer exception in the following code:

public String getOrderDescription() {
   return getOrderDate() + " " + getCustomerName().substring(0,10) + "...";
}

And so it begins, the spread of the null checks through the code. Just nip the problem at the beginning and be done with it: Don’t accept nulls.

By the way, if you wonder if we could make the parsing code accepting of null references and still simple, we can. Let’s say that the example with the work order came from an XML file. In that case, my favorite way of solving it would be something like this:

public static Set findWorkers(XmlElement workOrder) {
	Set people = new HashSet();
	for (XmlElement email : workOrder.findRequiredChildren("jobs", "job", "contact", "email")) {
		people.add(email.text());
	}
	return people;
}

Of course, this requires a more decent library than Java has been blessed with so far.

Lesson #3: Null checks hide errors and breed more null checks.

Conclusion

When trying to be defensive, programmers often end up being paranoid - that is, desperately pounding at the problems where they see them, instead of dealing with the root cause. An offensive strategy of letting your code crash and fixing it at the source will make your code cleaner and less error prone.

Hiding errors lets bugs breed. Blowing up the application in your face forces you to fix the real problem.

Comments:

[Martin Moene] - Sep 26, 2013

I’d say ‘checked examples’ is a good idea (http://www.eld.leidenuniv.nl/~moene/Home/papers/accu/cvu216-cc60/ ;).

s/errors that may occurs/errors that may occur/

Johannes Brodwall - Sep 26, 2013

Thanks for the correction, Martin

Checked examples sounds interesting, but as the link you sent didn’t have any code on it, I was unable to understand the concept. ;-)

[Martin Moene] - Sep 27, 2013

Rephrased as a less implicit, more conventional issue report:

In: “This practice is implicitly encouraged by Javaâ€™s checked examples (a profoundly bad invention), but other languages see similar behavior.”

s/examples/exceptions/

[Simon Parker] - Sep 27, 2013

Well said! Three of my favourite gripes, expressed clearly and concisely.

Johannes Brodwall - Sep 28, 2013

… oh!

[Erik Brandstadmoen] - Oct 1, 2013

People keep harassing checked exceptions in Java. I have never really understood why. The problem is not checked exceptions per se, the problem is that people keep catching exceptions they can’t handle. The problem is exactly the same in C#, which doesn’t have checked exceptions. What I hate about .NET, is that I’m not able to, in an easy manner, to find out which exceptions can occur in e.g. a library method. And, if I don’t know which exceptions can occur, I can’t handle them properly. In Java, however, if used properly, checked exceptions are a bliss. Consider the following change to your example:

https://gist.github.com/anonymous/2463daf6bd6620241776

So, I don’t agree with your bashing of checked exceptions. Otherwise, I think you have some really good points.

Johannes Brodwall - Oct 1, 2013

My gripes with checked exceptions:

Non-essential: False positives. From the totally wacko CloneNotSupportedException - which will NEVER happen or ALWAYS happen for a given class; to the bad judgement of UnsupportedEncodingException thrown from URLEncoder.encode(foo, “utf-8”) and MalformedURLException, to the unfortunate: StringWriter.write doesn’t throw IOException, but it’s usually called through Writer.
Suspicious: Hard to use correctly. There are very many examples in the JDK of poor choice of checked exceptions. If Sun (still in denial) can’t get it right, how can we expect most developers to get it right?
Critical: Non-locality: Unless you know really well what you’re doing, most exceptions should be dealt with at the application entry point so that the handling is uniform. This means that a simple IOException from a backend will bubble all the way through your method call hierarchy. In short, this tempts programmers to sin.

[Erik Brandstadmoen] - Oct 1, 2013

OK, I see your points, but, I don’t totally agree.

CloneNotSupportedException - if you get an object of type Object from, say, an old-school Java collection, and you try to clone it, it makes sense. You could always try to cast it to a Cloneable first to see if it is really cloneable, but, whatever. UnsupportedEncodingException is totally acceptable, as URLEncoder.decode and .encode accept strings to represent encodings, and there’s no way of knowing that someone provides valid strings here. and, as Java is supposed to be cross-platform, it makes sense. You probaly don’t have e.g. Windows-1252 encoding on a *nix box. StringWriter.write does throw a IOException, according to the docs: http://docs.oracle.com/javase/7/docs/api/java/io/Writer.html#write(char[])
You are probably right, but it’s hard to comment on this without concrete examples.
I don’t agree. The problem is that people generally aren’t very good at exceptions. You should always treat exceptions as an as important part of your domain objects as others. Of course an IOEception shouldn’t be allowed to bubble all the way up the call tree to the applicaiton entry point. It should be caught, dealt with if possible, if not, you should throw a more application-specific exception, possibly with the IOException as ‘cause’, say, a “FileSaveException”, a “DatabaseUnavailableException”, an “UnableToProcessOrderException”, or whatever is appropriate. In the same way as you create Customers, Orders, Shipments, etc, with properties on it, instead of passing strings and ints and Streams around all the way up to the part of your app facing the client (user/service, etc), you need the correct level of abstractions on your exceptions. A Stream should throw an IOException, an OrderRepository an “UnableToProcessOrderException”, etc.

I work with .NET in daytime, and I definitely know that having to think about “silly” checked exceptions, and actually acting on them correctly, would have saved us many runtime errors. Take the Stream class, as an example. It’s impossible to know which exception it will throw on e.g. a network failure, an encoding error, etc, by using only the compiler (as would be the case with checked exceptions), or even the docs: http://msdn.microsoft.com/en-us/library/system.io.stream.write.aspx.

Johannes Brodwall - Oct 2, 2013

My point about these exceptions is that in reasonably written code, they will never, ever occur in runtime. As a minimum, I despise the effect in reduced code coverage. As a more practical consequence, you’ll see people reinventing cloning and url-encoding because of this. Reflection is similarly broken by checked exceptions. Java developers are nervous about reflection, but the only hard thing about it is the huge catch-blocks caused by the checked exceptions.
I see you use the word “should”. “People should … throw a more application-specific exception”. I have heard this statement many times, but never observed the prescribed behavior. Let’s leave moralizing about what people “should” do to religions and instead act based on how people actually end up behaving. :-)

[Piotr Zientara] - Dec 22, 2017

Thanks for a very good post! You can make 2 bug fixes: “to stop because it it” should be ““to stop because it is” and “This code is more concise and if the is an error” should be “This code is more concise and if there is an error”. All the best!