Software Development

Offensive programming

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 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 the 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 it 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.
 

Reference: Offensive programming from our JCG partner Johannes Brodwall at the Thinking Inside a Bigger Box blog.

Johannes Brodwall

Johannes works as a programmer, software architect and provocateur for Sopra Steria Norway. He loves writing code in Java, C# and JavaScript and making people think.
Subscribe
Notify of
guest

This site uses Akismet to reduce spam. Learn how your comment data is processed.

11 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Faisal Feroz
11 years ago

Hi,

I had come across a similar paranoid programming incidence. Here is the code example that I came across:

if (obj == null) {
System.Out.println( obj.getClass().getName() );
}

Here the developer went so mad that he went asking for the class name of a null object :D

Johannes Brodwall
11 years ago
Reply to  Faisal Feroz

Wow, that’s pretty neat!

At least that way, you can be pretty sure that the obj has never actually been null (or that the system has never actually been in production)

Raphael
Raphael
11 years ago

I don´t know if a line like this: people.add(job.getContact().getEmail().getText());

is a good option. If we get a nullPointerException, the stack will indicate that line but not point what variable is null.

Maybe a good option is to at least separate this line in multiple:

Contract c = job.getContact()
and so on..

Eclipse has a good keyword for this Ctrl + Alt + L (Extract local variable).

Johannes Brodwall
11 years ago
Reply to  Raphael

It’s meant as an example of what NOT to do. The “law of demeter” says why. Extracting a local variably doesn’t really help much.

A good shortcut in Eclipse when debugging is ctrl-shitt-d, which “Uses the Display view to show the result of evaluating the selected expression in the context of a stack frame or variable in that thread”. (From Eclipse help text)

Sky
Sky
11 years ago

I really enjoyed your article Johannes, but this comment of yours confused and annoyed me. I don’t understand how the Law of Demeter relates to Raphael’s suggestion (or why you decided to wrap it in quotes, lol). Can you explain your though process here? Regardless of the number of local variables you make, you still have to have the same amount of knowledge about the structure of the objects you’re accessing. It might save you a few import statements if you don’t make the local variables, but does that really buy you anything? To address your Eclipse debugging suggestion, often… Read more »

Johannes Brodwall
11 years ago
Reply to  Sky

I pride myself on being confusing and annoying, Sky. :-) The code “job.getContact().getEmail().getText()” is a violation of the Law of Demeter. Using local variables doesn’t really change this, In this sense, neither code is an improvement on the other. (The reason use quotes is that the law of the demeter isn’t really a law, and I’m willing to discuss whether it’s appropriate in this context) Personally, I often find that using lots of local variables hide the intent more than it reveals it. I find it easier to see “Okay, we get the contact’s email address text”, rather than, “we… Read more »

rr
rr
11 years ago
Reply to  Raphael

Simply write something like the following for getting the right lines instead of create useless one time variables. The number of lines maybe the same but you will not get the var declaration waste.
job
.getContact()
.getEmail()
.getText()

Christian
Christian
11 years ago

I have seen code like

User user = new User();
if(user == null) {
return;
}

Johannes Brodwall
11 years ago

Priceless!

Kousalik
Kousalik
11 years ago

Hi,
I find your “offensive programming” idea intersting, but…. The method called badlyImplementedGetData has nothing to do with defensive programing. I think, it’s not defensive at all. Why ? Because everything you wrote is bad on it. I don’t like the way you showed dark sites of being defensive on completely nondefensive code. What is defensive on ignorig exceptions ? It’s just very very ……. very creepy code.

Article’s idea would worth much more, when confronted with well written defensive code.

Nutharsh
10 years ago

Hi,
Nice Article!

One more point to add. Creating too many local variables in frequently ran code can create lot of overhead on automatic memory management process.

Back to top button