Recently I developed my new pet theory - a different (different from traditional TDD/BDD) way of coding that focuses on readability can be feasible and beneficial.
I am fully aware that this theory might be flawed, but I feel the need to document it here because I think although the software industry has made huge progress in software development processes (TDD, BDD, xP, etc), we still have a long way to go. We follow those processes/practices, but we still produce code that later becomes pain to ourselves. We need to keep the thinking of new coding strategies/processes going.
In this article I am going to explain this pet theory in three sections.
Section 1 is to discuss why readability is definable and can be set as #1 priority for coding.
Section 2 is about why readability should be the #1 priority in terms of code quality.
Section 3 is to introduce a difference way of coding (I call it code in units) that was built upon of the principle of extreme readability.
Section I - what is readability? what is extreme readability?
From a couple of recent large projects, I refreshed my appreciation of how import to have a readable code base. After some further thoughts and discussions with other ThoughtWorkers on this particular topic, I feel that readability should be defined clearly so that it can be used a main principle.
The first reaction many developers had when I discussed readability with them is that readability is subjective and hard to define.
To some degree I agree with this observation but I believe this problem can be tackled by taking smaller steps. First we can define some coordinates for readability, then we define the extremes on those coordinates. With such definitions, readability can be more objective to the team than subjective to each person.
It makes sense to me that we have the two readability coordinates defined as the following:
Coordinate I. Identifying logic for given code. Specifically, for a developer that is new to the project but reasonably familiar with the technology stack and the business domain, how hard will it be for him to understand the intention of a given unit of code (method or class) to the extent that he knows how to change that code to incorporate new attention.
Coordinate II. Identifying code for given logic. Specifically, for the developer described above, how hard will it be for him to locate the implementation code for a certain unit of business logic.
These two coordinates are defined based on the two main purposes for us to read the code on a daily basis.
I am not sure if it's possible to define the sad extreme for these coordinates. One way might be that it's so unreadable that no programmer can achieve these purposes before he quits.
Fortunately it's the happy extremes that interest us the most:
For Coordinate I, the happy extreme is that the developer can understand the intention of the code by browsing from the beginning to the end of that unit of code only once.
For coordinate #2, the happy extreme is that he should be able to locate that implementation through a single path of search, in another sentence, he should neither be misled to a wrong path or find out that the unit of logic is implemented in multiple places (compositely or repeatedly or both).
To be more specific, I would list here some implications of these two extremes:
For happy extreme on coordinate #1, the implication of being able to understand the code by reading it once are:
- the unit of code can't be large otherwise it will be hard to keep track of what's going on when you read it the first time. For a method, less than 3 statements will be easy for reader to understand in a glance, while 5 will be probably be a stretch. For a class, there should be no more than a handful of public methods so that the intention of the class can't be easily interpreted.
- there shall be no disruptive logic in this unit of code, it's difficult for reader to switch context when reading, which means that code should be cohesive.
- there should be no confusing naming (variables, parameters, methods or classes). Ideally name should reflect exactly the intention of the variable. Sometimes there is a tendency to name something more generic than it should be.
- the developer should not need to refer to document or tests to understand the code
- tests should be understandable without description (or long method name)
For happy extreme #2, being able to locate the implementation through a single search path have the following implications:
- for a certain unit of business logic, it should always be implemented only once in one place.
- responsibilities should be divided clearly so that sub-logic can be easily located.
- it should be avoided that some knowledge of project specific convention is required to locate the code.
If you are a believer of Extreme Programming like me, you might agree with me that we can pursue extreme readability. With the two coordinates are defined, it is rather straight forward, simply always keep asking yourself those two questions when writing the code to implement some logic: how easy will it be for other developer to find my code and how easy for him to understand my code. Make your development/design decisions on top of them.
Section II - why extreme readability can be a principle for coding
With extreme readability defined, I think it's possible to simplify the principles of coding as the following
- Principle #1 - write code that works, which is so obvious that I probably shouldn't even include it in the following discussion
- Principle #2 - write code that is easy to read
- Principle #3 - write code that is tested or at least testable
All other software principles, such as SOLID, could be forgotten (and often they are), but as long as these 3 easy principles are followed, the project should be sound and life should be good for every developers in the team.
There is plenty of discussion why #3 principle is important, here is why I think the #2 readability principle should have a much higher priority than other coding/design principles:
- Following some of the common design principles blindly could lead to over engineer and thus makes the development less agile. but certainly others might have different opinion which brings the second point.
- The readability principle is relatively easier to be agreed upon and followed than a bunch of design principles that often times very difficult for a big team to consistently follow simply because they are not unanimously agreed upon. And if a principle can't be consistently followed in a project, the majority of its value is often lost.
- Many of the design principles can be deemed as consequential to the readability principle. In another sentence, they can be driven by following the readability principle. For example, the "Single responsibility principle" could be a natural consequence if we organize code cohesively so they are easy to read. There is more explanation on this in section I. Even the principle of always automate can be deduced from the readability principle - manual processes, when documented, have a lot more readability issues than automated process due to pretty much the same reasons why code comment has more readability issues than readable code.
- Readability is more universal. Many of the other sets of design principles only applies to a certain programming paradigm, e.g. SOLID for object oriented, while the readability principles can be applied to a broader range of programming paradigms.
- Readability has more importance. Many deem tests as the #1 priority, but readability could be even more important than tests. I saw plenty of unreadable code that are fully tested but you still don't know how to change them. On the other hand, I rarely found any real readable but untested code that is hard to add test for.
- Readability always has value. A significant number of the design/development principles were developed to promote extensibility and thus their value can only materialize when the code is extended, not to mention that unnecessary extensibility is not only a waste but also can be a burden sometimes. On the other hand readability always have value, even when you are about to delete the code, you need to read it to make sure you delete the right one. In other words, focusing on readability is more agile than pursuing design principles that may or may not provide value.
Section III - introduce code by units
Now lets introduce a strategy, a new way of coding to pursue extreme readability defined in section I and follow the 3 easy principles in section II.
In one sentence, developers should always focus in a small unit of code (by "small unit" I mean 2-3 statements, no more than 5 ). In TDD, it means that developers are on a constant and quick rhythm of writing a small unit of test and then a small unit of implementation.
Here is simple example, presume that we are developing a website in which we need to render links to pdfs on external websites. We have the urls of the links but we are not sure the availability of the pdfs on those websites. Some urls will redirect visitor to a html page saying that the pdf is not ready yet (but it maybe in the future). Other urls will simply return a http error. We need a checker to check this so that we can avoid rendering links that do not work.
First let's see how this would be done in a traditional TDD fashion:
Step 1, first you do some research and found out that you can use the Net::HTTP to get the http response of the url, with the sample code, you write the following test
Note that you have to do this Net::HTTP library research before you can write the first test.
Then you write the following code to have it pass
Then you add a failing test
To have it pass simply replace the last return true statement in method with the following
Then another failing test to make sure you the content of the http response is a pdf file
This time replace the last return statement as the following and the implementation is pretty much done
Now the class and test looks
This code looks not bad right? Well, in normal development, I would probably leave them along, but if our goal is to achieve extreme readability there are a couple of issues here:
First
is actually testing the url parse and it's repeated 3 times. This makes the tests harder to read because of irrelevant information so maybe this should tested in a separate test.
Then you can remove the ".with("sample.com", 80)" from the other tests, which helps the readability, for example:
As you can see the new test helped readability, but it itself has some readability issue - it had to mock the response even when it's not tested at all.
Second, it didn't really test that you should use http GET to test the url. Actually these tests will still pass if you didn't write http.get(url.path) in the HTTP start block. That is definitely a fail. You probably need another test and break the two principles mentioned above.
Again in this test, you actually don't care about the response because it's already tested in other tests, but you still have to mock it.
Now you have 6 specs with some minor readability issues other than the two mentioned above such as 1) :code => "200" is duplicated twice, you will relie on the test description to understand that it means that the response is successful. You have the same problem in the implementation. 2) I would argue that you will need some knowledge of HTTP protocol to quickly understand the intention.
It's quite a journey to achieve this isn't it? You will have to review the code and test carefully to identify the problem and then loose some principles to reach this sub-optimal point.
Now let's see how we would do this in very different fashion, in which we only think in very small steps and code in very small units. And we only care about readability during coding. Here, the first small unit is that this check method should request for the url and return true if a pdf is responded. As for how to request and how to request and check if it's a pdf we don't care for now.
Here is the first test
Implementation
simple enough, now let's worry about how to do the request, this is the time we do a bit google to find out about the Net::HTTP library. Here the test:
implementation
Then we can worry about how to tell if the response is a pdf. First the response should be successful and then it's content should be a pdf. Again we don't worry about how to tell successful or content type yet.
Then another small step, how to tell if the response is successful
Finally, take care of how to tell if the content is pdf
Now the class and test look like the following
Note that I used a small trick here to test private methods, that detail is hidden here.
In this version, our code has better readability. The intention of the class can be quickly understand by reading the public methods. Implementation detail can be easily located in the private methods. Each private method is very easy to read. On the test side, there is no duplicated tests (no logic is tested more than once) or duplicated mock (no logic is mocked more than once). Every test can be easily understood without reading the description. We can say that this code satisfies the definitions of extreme readability. Interestingly, this kind of unit thinking resulted in a code style that is more functional than imperative.
The specs here are separated into two groups - the normal describe PdfChecker block tests the public interface. However, in our case, instead of testing the behavior, it's more like a description of what the code does than a test. That's actually due to the fact that now the code body of the public method is merely delegation to internal helper methods. So maybe we can replace this block with some more end to end functional tests ( which happen to be difficult in this case).
On the other hand, at least too very common practices were not followed: 1) do not mock the class you are testing and 2) do not test private methods. Also there is minimum level of integration tests. In order to do this thinking in small step and then coding in small unit fashion, we dramatically changed our way of testing. We need to examine if this new controversial way of testing is a price too high to pay. From now on, we are going to call this style of coding "code in units"
We can try do some preliminary examination by trying to add some extra features to the code and then some hypothetical bug fixing.
1, adding proxy support, 2, adding cache function, 3, changing HTTP Library.
Round 1, let's add proxy support to the class so that it can also work in an environment where proxy is needed to reach external websites.
Let's assume that we are going to store the proxy settings as instance variables when instantiating the checker and use them when doing the checker.
In the traditional coding, you will add another test like the following
It's mostly okay except that you are forced to mock the http response although you are not testing it. It will take reader a bit time to figure out that :code => nil is irrelevant to this test.
Then you modify the class code
All tests pass again. It might be a bit surprising for people who are not familiar with the net/http library, because in the code it's using Net::HTTP::Proxy.start while all the other tests are mocking Net::HTTP.start. It is passing because when proxy_addr is nil, Net::HTTP::Proxy(proxy_addr, proxy_port) simply returns Net::HTTP. Surprise is not a good thing when talking about readability. It also means now our tests actually depend on a library's implementation detail although from the tests, it looks like that we are mocking it. And from those tests it looks like the class is using Net::HTTP directly. To solve this problem, we will need to change all of our tests to mock Net::HTTP::Proxy instead. That is to replace "Net::HTTP.stub(:start)..." with the following
What about in code in units fashion? We change the existing test on the request method to the following
Then we add the same constructor in the pdf checker and change the request method as follows
That's it. There is no impact at all to other tests. All interactions between our class and the net/http library are captured in one private method and one test on that method.
Round 2, let's add a cache function so that the result of the check can be cached to prevent excessive http requests.
Let's assume that once a pdf becomes available it will always be available. So we want to provide another public method which also returns the availability of the pdf url. This method, however, will do the http request checking if the url is not http checked before or it's checked before but was not available at that time. If it is checked before and it was available, the result (true) will be returned without any http request.
First, traditional code style, we construct the tests based on the spec mentioned above.
The reason I wrote two tests at the same time rather than having the first test pass and then write the second one is that after I wrote the first one, I realized that it is easier to implement it in a way so that both tests pass. Here is the implementation:
All tests pass now. Three issues here. 1) I was forced to mock the http response code and content_type again. 2) HTTP protocol knowledge is required to understand the intention of the tests, you need to instantly translate the "code => '500'" to an available pdf and ":code => '200', :content_type => 'application/pdf' to an unavailable pdf. 3) In the second test, the intention is to test if the available method returns the result of the second check. However, we only tested the scenario when the pdf became available in the second check, we should've also checked the scenario where it's still unavailable, but the benefit of this such might be canceled out by the duplicated code introduced.
Alright, now let's try do this in the code in units fashion. The way to proceed this is different from the traditional way. First you read the code before writing the test. The only public method is check
It's extremely easy to see that it's doing a request. In the code by units testing fashion, the tests can be written to test if the new method calls this check method correctly.
Note how easy these two tests are to read comparing to the ones written in the traditional fashion. They avoided all the issues mentioned above in the traditional ones. In the second test, it also effortlessly tested that the available method should return the result from the latest check. Of course, it has its own issue, it didn't really explicitly test that the http request was not sent when not necessary. But I can hardly think of a scenario where someone change some code so that these tests still pass but fail on the intention. Maybe someone change the available? method so that it does its own http request checking too. Provide that the code is very readable and the idea is that other developers should read the implementation code (not just the tests), such chance should be low.
The implementation is the same as traditional way
Round 3, let's pretend that the HTTP library we are using was changed, which introduced a bug which we need to fix
Assume that we upgrade to a new HTTP library but we didn't know that the response code attribute in HttpResponse is changed from code to resp_code.
First of tests in both coding fashion will fail to discover this because they both mocked the response. The fix on the implementation side is quite easy, just change res.code to res.resp_code. However all the tests written in the traditional ways are broken, because they all mock this code. So all of them have to be changed. On the other hand, only one test written in code by units is broken because the resp.code is only mocked once.
In all these 3 rounds, the tests in the code in units fashion were less painful to work with, partly due to the fact the interaction between this class and the http library it uses. They also consistently provide a much better readability than the tests written in the traditional fashion. The only extra trade off we are taking here is the integration points between the private methods are not tested. There are two obvious risks associate with this: 1) existing hidden bug with these integration points not detected, and 2) future changes on these integration points will not be guarded. These are legitimate risks.
However two arguments can be made here. First, as a result of the code by units fashion, these internal integrations points are very simple, mostly just simple delegations. Actually most of such integration points(in another word, private methods) were introduced due to this code by units fashion. There shouldn't be much to test about them. Second, we often not test the integration between classes (by mocking one of the classes), and these class internal integrations are normally simpler and with definitely narrower scope. If we can take the risk of not testing the integration points between classes, why can't we take risk on these even "easier" integration points for the benefits?
There is another trade off here, that is, since we are no longer doing BDD, we lost the class specs that can tell developers the behavior of it. The idea here(as mentioned in Section I) is that the implementation code in the class itself should be readable enough so that the behavior can be interpreted easily without going to its test. This idea might need more test than other ideas in this theory though.
Conclusion
Congratulations!! if you reach here, your superior patience just won you a grand prize: a 25% discount on a two-hour personal boudoir package provided by the world famous photographer - Kai, checkout the detail out at Kaipic.com.
That aside, in conclusion, my pet theory is that first we can define readability, second we can use readability as a principle to guide coding activity and last the principle can be followed easier if we use a different way of coding, that is, code in small units. The trade off of this different way is that we no longer test the integration points inside a class, and my guess now is that it's gonna worth it.
I will continue testing this pet theory in my personal projects (I fully understand that only in a real sized project, the benefits/shortcoming/feasibility can be truly exposed,) and keep you posted.
Very good post. I am looking forward for more and I will try to use it myself.
ReplyDeleteThank you palove. If you have any thoughts after you try it, I will really appreciate it if you can post them here.
ReplyDeleteHad to point out the funny typo while reading that, just under Section II heading:
ReplyDelete•Principle #2 - write code that is easy to readable
Thanks Nate! corrected.
ReplyDeleteGreat post - I agree with your comments in the second section.
ReplyDeleteI'd suggest that to achieve this code is reviewed by a developer who is working on a different project and who is more experienced with different technologies. They (rather than someone expert in the domain) should be able to understand the logic from the code and communicate their understanding back to you.
If they can't do this or their understanding is incorrect, then the code should be rewritten until the code communicates its intent.
I agree that some design patterns tend to be overused, and the fashionable ones change over time. Simplicity and readability are more important.
Thanks Richard.
ReplyDeleteHaving code review from devs outside the team could be definitely helpful. But I am not 100% sure if it's alway feasible. My hope is that we can provide enough guidelines so that we can still control our readability without it.