Agile Zone is brought to you in partnership with:

Programmer, solution architect, user group and conference organizer, conference speaker and traveling fun code evangelist. Johannes tries to apply Agile principles to large software projects, but what he's really passionate about is sharing the experience of more fun programming with other coders around the world. Johannes is a DZone MVB and is not an employee of DZone and has posted 35 posts at DZone. You can read more from them at their website. View Full User Profile

Offensive Programming

09.27.2013
| 9143 views |
  • submit to reddit

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 of 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 might 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 examples (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, do we?

What would happen if it 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 in 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 that are caused by the environment, such as a network problems, 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. 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 breeds 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.



Published at DZone with permission of Johannes Brodwall, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Robert Saulnier replied on Fri, 2013/09/27 - 9:29am

Ok, here are just a few issues I have with this article.

Your typical example is not typical.

Lesson 1 is completely wrong. Handle exceptions as soon as you can, but not sooner. The getData method could have returned null, or an empty String, or "No data", or whatever, or could throw an exception, but it's possible to handle the exception locally if that's the behavior you want from getData.

Java checked exceptions is not broken just because it's used improperly, as you demonstrate in this article.

Who uses Timer anymore? If your code passed a valid URL instead of a String to getData, then getData could return an empty String or "No data" when there is a connection issue. And you don't have to handle any IOExceptions in your TimerTask.

If a method is documented to possibly return null values and you don't check if the value is null, then that's not offensive programming, that's stupid programming.

Returning nulls breeds errors. You should think carefully if your method should return null, and if it can, it should be documented.

Lund Wolfe replied on Sun, 2013/09/29 - 3:15pm

I don't like defensive programming either.  Exception handling should be done in the context of the application as a whole (who catches the exception and where it is reported), but that is a lot more difficult than just catching it and logging it wherever.

If the error isn't expected, then just fail it up front rather than leave a gazillion potential error points that will be hard to track down in the code bloat later.  Concentrate on making the code simple and fixable.

Martin Vaněk replied on Mon, 2013/09/30 - 11:47am in response to: Robert Saulnier

I agree that checked Exception are useful when used in sane way, but returning "No data" while swallowing ioexception is pure evil

Robert Saulnier replied on Mon, 2013/09/30 - 12:09pm in response to: Martin Vaněk

I agree that swallowing exceptions is evil (in almost all cases), that's why I didn't state that the exception should be swallowed, I only stated what the method could return. Minimally, the exception should be logged.

The example used a Timer to probably get data at some regular interval, so the method should always work. I just gave some possible return values if the data could not be retrieved.

Robert Lauer replied on Fri, 2013/10/04 - 4:51am

Is there something missing in the 6th example? I don't really get the difference to the above examples.

Johannes Brodwall replied on Fri, 2013/10/04 - 8:08am in response to: Robert Lauer

The sixth example (if I count correctly) catches IOException.

Robert Lauer replied on Fri, 2013/10/04 - 8:44am in response to: Johannes Brodwall

... I was just wondering: The line before the sixth example says " Perhaps we’re better off just killing the timer?", but the example code doesn't kill the timer, rather it swallows the exception because it cannot handle it.

So the point is not killing the timer but going on because there's nothing else you can do. Or maybe I did not get the point yet.

Btw I agree with the general point that defensive error handling gets you into problems (I remember seeing a presentation by Bill Pugh (the findbugs author) saying the same thing).

Raging Infernoz replied on Fri, 2013/10/04 - 8:37pm

That sure is Offensive Programming, to a Professional Developer like myself, I would reject most of your 'corrections' in code review as bad for logging, dated, or plain anti-patterns:

1. Is obvious nonsense, anyone coding like that should be sacked!

2. May work, provide you don't loose important value information; if you will, catch the exceptions and wrap them with a useful message and the original exceptions as cause, otherwise your log file will be of little use!

3. Should be done with a java.util.concurrent.ScheduledExecutorService, so you could then poll the ScheduledFuture to see if it had finished, then grab it's exception for analysis, to decide if it should be rescheduled etc.

7. Would be better using an XPath search approach.

8. The no message NullPointerExceptions will reveal you as a crap coder, and (expensive) Exceptions should not be used for flow control!

12. As stated in 7, use an XPath search approach.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.