When have you seen the 4 aspects done well or poorly?

I have yet to work in a codebase that satisfies the first of the rules. The majority of the projects I’ve worked on have partial automated testing coverage, with the rest being a manual process. This isn’t exactly bad; few projects have 100% coverage with automated tests, and it’s debatable that such a thing is even useful.

That said, spotting the code that should be tested and making sure it is pays dividends. Two key areas of note are comms code (in particular bindings) and database migrations. We have tests for both that frequently flag up issues in our code that we would otherwise miss.

However, I struggle to think of a time where these tests have influenced the design of the code as described in this chapter. Certainly, these tests grant assurance that critical parts of the application work as intended, but I don’t think they feed back into the overall design.

Perhaps what has been more useful is what I’ve learned from Clean Architecture⁵ about how to split up the world. Clean Architecture has led to more testable code, but I’m not convinced that the inverse is true: that writing testable code leads to Clean Architecture. I suspect it probably does lead to better code, and more testable code (obviously), but I think there’s more involved here.

In terms of code duplication, this is oddly enough one of the most common things I find myself refactoring. Not because people don’t see the duplication, but usually because the wrong approach has been taken to minimise it. This usually takes two forms:

• Inappropriate interfaces have been written that describe implementation details rather than abstractions.
• Badly designed inheritance hierarchies that either assume too much or too little about their subclasses.

In both cases of the above, refactoring as I go isn’t too hard.

In terms of expressive code, I have serious issues with the naming conventions used in some of the products that I work on. Some examples:

• Objects that send data, download data and perform multi-part comms operations (such as async calls or batched calls) are all named in a way that implies their job is to send data.
• There is no consistency in naming objects that are serialised to the database (using ORM) and objects that are serialized for comms operations. Sometimes the same object is used for both (a practice I do not support) meaning that our comms code and persistence code have shared knowledge of a class that is effectively a DTO.

Naming in general has always been considered one of the most difficult problems in programming. It’s the subject of numerous jokes.

Source

Jokes aside, naming is genuinely hard, and it’s an area that I am pushing to improve in my team. The methods I am using for this include:

• Specifically evaluating names at code review stage.
• The development of a Domain-Specific Language (DSL).
• Various formal naming conventions in our style guide documentation for specific types of variable.
• Informal naming convention guidance in our style guide for common patterns in the codebase.

Finally, minimising classes and methods. Ironically I often find myself doing the opposite of this when refactoring code. This is mainly because of what I mentioned above: inappropriate abstractions and hierarchies. Fixing this usually requires breaking down interfaces or adding additional layers of abstraction. I also often find myself breaking down longer methods, but in such cases it very rarely results in less code. It usually results in more methods and classes that are longer in terms of line-count. However, after this refactoring the classes are generally simpler and more expressive. It’s often the case that I’ve split off responsibility into another class, following the Single-Responsibility Principle. The author does state:

”…although it’s important to keep class and function count low, it’s more important to have tests, eliminate duplication, and express yourself.”

I agree with this to the point where I feel that this rule is not particularly useful at all. Classes and methods should be small, but this should arise out of things like the SRP and DRY, not the other way around. That said, long classes and methods are often a code smell, so maybe this rule does have merit.

When have you seen concurrency done well?

There is quite a lot of concurrent code in the applications that I work on, and we tend to use generalised threading utilities to simplify the process of running concurrent code. These utilities are all tested and used extensively throughout the application. They encourage scheduling “fire and forget” runnables into a shared thread pool along with named single-threaded executors for things like database operations that should be executed in-order. This approach has been very effective, but I wrote and designed these utilities, so it’s a bit rich to pat myself on the back and say “good job”.

So in terms of concurrency done well, I’d probably give credit to the Android libraries for making it easy to write concurrent code. Things like the WorkManager make it much easier to write concurrent code that can be scheduled in useful ways. We can natively schedule UI work onto an Activity or View allows it to be cleaned up along with the UI. We can use LiveData to write code that only runs if both the data changes, and the thing observing the data is “alive”. This is all provided along with the standard concurrent libraries. It gives you a lot of ways to approach concurrent development.

When have you seen concurrency cause or fail to solve a problem?

I’ve seen a lot of problems with concurrent code. I once had a Python script that took data from Ordnance Survey and processed it into a GIS database. It took all night to run, and as I was tweaking the parameters and re-running it a lot, I figured it made sense to parallelise the workload. The next time I ran it, it took twice as long! That was when I found out about Python’s Global Interpreter Lock.

Other problems I see often include abuse of the synchronized keyword in Java. When a method is declared as synchronized, it acquires a lock on the instance of the class. It’s a bit of a blunt approach to concurrency control, but it works pretty well in most cases. The problem is that:

• If a class’ main method of access control is the instance of the class, then there’s nothing stopping something else with access to the instance from also using it as a lock. This is unlikely, but has happened once in my career, and it (predictably) caused a deadlock.
• You might have 2 distinct “sets” of operations that don’t need to block each other. For example you might have something, let’s say a Consumer, with a consume() method. It makes sense to put the synchronized keyword on that method as a simple form of thread-safety. You might also want to have some listeners that monitor the overall state of the Consumer; so you might also have addListener() and removeListener() methods. Perhaps you need to make those thread-safe. If you use the synchronized keyword here, suddenly you can’t consume things while you’re adding/removing listeners and vice-versa.

The solution to both of the above problems is pretty simple: using a privately-held lock and a synchronized block rather than using the synchronized keyword.

Other common issues I’ve encountered with concurrency is performance. I’ve seen a lot of things that are queued to run on the main thread (often called the UI thread in Android) that don’t really need to. Offloading such work to a worker thread or single-threaded executor is usually pretty simple, but problems arise when the main thread is being used as a substitute for a single-threaded executor.

What do you find most challenging when thinking about concurrency?

The biggest issue I have with concurrency is ensuring that things run on the right thread. For example, we have background database operations that should all be run on a dedicated single-threaded executor. We have tools that allow us to reject attempts to run such code on the main thread, but we have no tools that allow us to enforce a specific thread executor.

At present, our only real solution for this is scrutiny of the code at code-review time. We can use annotations to denote that methods need to run on worker threads, etc. but these are of limited value. I would love to have compiler-enforcement or even just linter checks that will flag up code that is being run on the wrong thread.

I also think that sometimes there’s a lot of boilerplate around concurrency in Java that we can do away with. Why can’t InterruptedException be a RuntimeException? Wouldn’t it be nice if we could annotate methods with @WorkerThread, and then when called, they would automatically be dispatched to some arbitrary thread pool? Maybe that oversimplifies a complex problem.