Showing posts with label domain driven design. Show all posts
Showing posts with label domain driven design. Show all posts

Tuesday 17 January 2017

Writing a book - Secure by Design

Dear Junior

I'm sorry for not writing to you for quite some time. But, you see, I have been working on a book together with my two colleagues and friends Daniel Deogun and Daniel Sawano. Late this summer we decided to try to write down our collective experiences and insights in design and security as a book. We are far from finished, but by now the first few chapters are released from the publisher Manning as part of their "Manning Early Access Program" (MEAP).

The book is named Secure by Design and our main message is how good design and good code can help developers avoid security problems. We have found that lots of security vulnerabilities are due to bugs that could have been avoided if the design of the code had been different. 




You as well as I know that it is a lot easier to think about "good design" when developing features, that it is to think "security" at the same time.

Of course not all good design lead to avoiding security vulnerabilities. So we have gathered our experience on what design-tricks that are most effective in giving security as a side-effect. It's probably no big surprise to you that Domain-Driven Security is an important part of this, but there are many other parts as well.

We still have a lot of material to cover. But, what we have started with things that are close to the code, such as building Domain Primitives, how to structure validation, immutability etc. I guess you will find several of these ideas familiar, as we have discussed them earlier - but there are also some new insights I have not yet had the time to write to you about. Well, not to mention all the ideas from Daniel and Daniel which you and I haven't had possibility to reflect on earlier.

In the later parts there will be more material on integration, security benefits from cloud thinking. We also want to share our ideas around architecture such as security concerns for microservice architecture or how to handle legacy codebases.

We will continue to write during the spring and early summer. Hopefully we will see the book in print sometime during the fall this year.

Yours

   Dan

PS The early-access was released last night; you can check the book at https://www.manning.com/books/secure-by-design

Wednesday 19 November 2014

Make Concepts Explicit when Fixing Bugs

Dear Junior

To find implicit concepts and make them explicit is a very powerful way to improve code. But, sometimes it is hard to judge beforehand which concept is important enough to make explicit. We try to make good judgements, but sometimes we miss.

But, no reason to despair, we can take Time as our helpful aid. So, instead of everything perfect from start, we continuously perfect it whenever we find a reason. Two of the topmost reasons are either a bug appearing or further development of the code.

Let me for now fokus on the case of when a bug appears. If the code does not behave the way we intended it to do, then there is most often a place in the code that was to convoluted. In that convoluted state, subtle misunderstandings and simple mistakes could hide. The solution is to clarify the code until it is obvious whereof the mistake consists.

Things get clearer by examples, so let me clarify what I mean with a very minimal example, the story about a bug and its fix.

In real life, more or less a decade ago, I worked on a project where we did a batched import of records coming from an external source. The code looked roughly like this following in the class ImportCron.

public class ImportCron {

    private RecordImporter importer;
    private Logger logger;

    void runImport()  {
        List records = importer.listWaitingRecords();
        logger.log(Level.INFO, "Importing records: " + records.size());
        for ( Object rec : records) // imports each of the records in turn
   …
    }

Now, the import was run once a minute, but most of the time there where non records waiting.  Problem was the import log became filled with rows saying "Importing records: 0" so we had to grep out the non-zero lines each time we wanted to look at what was happening.

One of those days I got tired of this, fired up the editor, and wrapped the logging in an if statement to filter out all those zero-import writes.

    void runImport()  {
        List records = importer.listWaitingRecords();
        if (records.size() == 0)
            logger.log(Level.INFO, "Importing records: " + records.size());
        for ( Object rec : records) // imports each of the records in turn
   …
    }

A few days later we released during lunch, which was the best time for our users. The deploy went smooth, apps started up as expected, and soon it was time for the first import "tick".

Within a minute, import was run the first time, and the log line read:
<timestamp> Importing records: 0
While standing confused, a minute went by, and the log line read:
<timestamp> Importing records: 0
Another minute went by … and no log line.
While firing up my editor to check the code, another minute went by and a third log line appeared:
<timestamp> Importing records: 0
A colleague of mine looking in the database calmly reported "We just imported three records".
It only took me a split of a second to realise my mistake once I had the code in front of me.

    void runImport()  {
        List records = importer.listWaitingRecords();
        if (records.size() == 0)
            logger.log(Level.INFO, "Importing records: " + records.size());
        for ( Object rec : records) // imports each of the records in turn

    }

Obviously my spine reflex for coding "check for zero" made me write the boolean expression the wrong way around. By the way, did you capture that mistake at first glance in the code on my first description? Confirmation bias is a nasty thing.

Now, I must say this kind of bugs are pretty uncommon, bugs that are typos or simply mispunching the keys. Most bugs in my opinion are rather that pieces of code subtly misunderstand each other.

Well there are at least two ways of fixing this code. The obvious, and fastest would be to quickly change "==" to "!=". However, humbled by the mistake I had done in the first place, I realised that that kind of hasty coding was what got me in the trouble in the first place.

One of my coding mantras since long have been "Code should mean something, not just do something". So, a better way out would be to make the code more meaning-ful. From Eric Evans I learned the phrase "Make implicit concepts explicit", which says the same thing in this context, but gives a better guiding direction forward.

I took to the challenge of fixing the bug by finding what implicit concepts had been missed, and making them explicit until it was blatantly obvious that the code was wrong. An added benefit would be to be able to make a test proving the code was wrong.

Extracting the boolean condition to a method of its own would force me to spell out the meaning of that piece of code.

    void runImport()  {
        List records = importer.listWaitingRecords();
        if (containsRecords(records))
            logger.log(Level.INFO, "Importing records: " + records.size());
        for ( Object rec : records) // imports each of the records in turn

    }

    static boolean containsRecords(List records) {
        return records.size() == 0;
    }

Granted, this is more code than I started with - but I think the code is more "to the point" (phrased inspired by Rickard Öberg, another of the great programmers).

At least this made me able to write a test

    @Test
    public void shouldConsiderEmptyListNotContainingRecords() {
       Assert.assertFalse(ImportCron.containsRecords(Collections.EMPTY_LIST));
    }

Well, at least this is what the test would look today - at the time JUnit looked slightly different.

Anyway, now I have a failing test. Claiming "the empty list does not contain records" is simply false - as proven by the test. We safely update the code to fix the bug.

    static boolean containsRecords(List records) {
        return records.size() != 0;
    }

Upon which the test switched to green as expected.

During this "refactor -> put under test -> fix" something interesting has happened. The concept "import list contains records" that hitherto had been implicitly represented by the technical "records.size() != 0" has now been made explicit and given a name "containsRecords".

It might be claimed that the main benefit in this example was derived from the drive to put code under test before making a change. Undeniably, that is a point, but I think it only tells half the tale. 

Simply putting things under test can be done in a myriad of ways, and I have seen several very technology-based efforts. I do not think those efforts have paid off handsomely. Mostly the code get cut along technical lines to push in tests in the gaps. But, the code does not get more comprehensible over time.

Focusing on "make implicit concepts explicit" have been a more productive course for me when working with code.

Yours

   Dan

ps My team-mates did not force me to skip lunch to fix the bug. We went out together, and when we came back satisfied and rested, I sat down and fixed the code. We released the fix during lunch-time the day thereafter.






Monday 17 November 2014

Make Implicit Concepts Explicit in Code

Dear Junior

In our letters on software development we have touched upon the idea several times, but I think it might be worth spelling it out - the idea to make implicit concepts explicit; something I see as one central message of Domain Driven Design applied to coding.

Let me pick that phrase apart for a moment. When coding we have a lot of concepts in mind, e g when writing code that handles people I might have a class namned Person. In this case the concept of person has an explicit representation in the code - i e an explicit concept.

A person might have a birth date, represented in code as a data-field Date dayofbirth in the Person class; another example of an explicit concept.

We might have business logic restrictions that are based on the age of the person, e g you have to be at least fifteen years to access some content. So there will be code calculating this.

    void contentRequest() {
        ... 
        boolean access = ((new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear) >= 15;

Again we have a concept represented in code, this time age. However, this time the representation is not explicit - there is nothing in the code saying "age". Age is an implicit concept

There is nothing strange with having implicit concepts. In the short code snippet we have several implicit concepts: current point of time, point of time of customer's birth, and age-limit for content are just three obvious. This is not a problem. The programmer reading the code will intuitively re-construct the relevant concepts.

However, the design guideline from Domain Driven Design advice us to keep an eye open for important concepts - and if we find the represented implicitly, then make that implicit concept explicit

Let us look at the code snippet again.

        boolean access = ((new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear) >= 15;

Of the implicit concepts dwelling in this like, which of them seem important enough to make explicit? 

The concept of "current point of time"? Probably not.

The concept of "milliseconds since birth"? Nah, not that either.

The concept of "milliseconds since birth expressed as years, rounded downwards"? Hey! I know this! We already have a word for it - we call it "age"! And we talk about it all the time!

Well, that sounds important enough. Let us start with giving that value a name. 

    void contentRequest() {
        ... 
        long age = (new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear;
        boolean access = age >= 15;

By the way: this is where I really like the IntelliJ shortcuts "cmd-w" to widen the selection until the expression is selected, then "cmd-alt-v" for the refactoring to extract the selection as a variable - naming it "age" in the pop-up. It literally takes less than 10 seconds.

Now, we actually have made the implicit concept age into an explicit concept. Mission completed.

Well, not completely. We need to find the concept a good home where it can lead a good life. Right now it is stranded in the middle of some access computation. At least we can give it a method of its own.

Applying another of my favourite refactoring "Replace Temp with Query" yields this code.

    void contentRequest() {
        ... 
        boolean access = customerAge(customer) >= 15;

    private long customerAge(Person customer) {
        return (new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear;
    }

Better, but can be improved to make the concept clearer. If we talk about the "age of the customer" it will vary from time to time, and it might cause confusion: the age when the customer requested access to the content, the age when content was first accessed, when it was last accessed, the age now? Better to clarify.

Of course we could rename the method to "customerAgeNow". However, I do not feel comfortable having my concepts depend implicitly on external things like the system clock. I prefer to make those dependencies explicit. So we make the time-in-question a parameter. 

    void contentRequest() {
        ... 
        boolean access = customerAgeAt(customer, new Date()) >= 15;

    private long customerAgeAt(Person customer, Date timepoint) {
        return (timepoint.getTime() - customer.dayofbirth.getTime()) / msPerYear;
    }
 
This design also improves testability drastically. The tests can now use explicit test-data, and need not rely on checking, or changing, the system clock.

Kudos once again to the lovely refactoring support of modern IDEs. Another less-than-10-seconds-refactoring: two cmd-W to select "new Date()", cmd-alt-P to extract as parameter, change name to "timepoint" in pop-up. Enter. Done.

By now it is pretty obvious that the method "customerAgeAt" suffers from feature envy. The most relevant data it operates upon is the Customer, but it resides somewhere else. We should consider moving it.

Should we move it, the Customer class would get a method "ageAt", taking the liberty to rename it slightly. That method certainly makes sense in this context, but does it so in other contexts? As we do  not have the rest of the codebase at hand, we will just have to pretend that "ageAt" makes sense in other context - and might actually be useful there as well.

This is really one of my favourite refactoring: move method. Again extremely smooth thanks to modern IDEs. Literally two keystrokes (F6, Enter - in IntelliJ) for the move, and a few for renaming. 

    void contentRequest() {
        // ...
        boolean access = customer.ageAt(new Date()) >= 15;

class Person {
    Date dayofbirth;
    // ...
    long ageAt(Date timepoint) {
        return (timepoint.getTime() - dayofbirth.getTime()) / TimeUtil.msPerYear;
    }
}

You might not believe me, but I remember the days of yore when you actually had to do all the involved steps manually; cut-n-paste the method body and header, change the list of parameters, update the code to use the fields of "this" instead of the argument, change all client calls (which might be several).

Now, we have finally ended up with a version I feel comfortable with. The concept "person" has now an conceptual attribute "age at" which has an explicit representation in code. In code we have "enhanced" our language of what we can talk about directly. So, when we discuss the age of a customer with business people, it is likely that we consistently mean the same thing - even if we for sure still can misunderstand each other.

As a side effect the code checking the access has become a lot clearer.

        boolean access = customer.ageAt(new Date()) >= 15;

This is a line of code that can be shown some person from the business side and explained by reading it out "access is granted if the customer's age at 'now' is at least fifteen years". Given that support, they can see by themselves - something that really builds trust. 

The time invested was not huge. Coding-wise all refactorings took less then a minute together. The time to slowly realise that "age" is an important concept in this particular domain is not counted. However, that insight will have to dawn on the programmer sooner or later, and when that happens, that extra minute is well invested.

To make concepts explicit does not only apply to code, it can be fruitfully applied to other areas as well, e g capturing and writing requirements/specifications. 

As we all stand on shoulders of gigants, I also must give credit where credit is due. The phrase "Make Implicit Concepts Explicit" I have picked up from Eric Evans, the thought leader of Domain Driven Design. 

In conclusion: to make implicit concepts explicit help us to over time close the gap between the code and the understanding of the business domain. It is not uncommon to in the process find bugs or crucial misunderstandings, that then can be adressed in a proactive manner instead of popping up as nasty surprises later on. 

Yours 

   Dan





Saturday 25 May 2013

Days of Domain-Driven Security

Dear Junior

It has been a truly interesting and challenging DDD Summit, where I have had the chance to present the subject of Domain-Driven Security and discuss it with several of the world's most prominent Domain-Driven Design experts.

There seems to be a general agreement that the way John Wilander, Erland Oftedal and I phrased the subject rings well with the insights of those experts, and that they agree on the means of using doman modelling and context mapping as tools to understand indata validation and vulnerability attacks as Cross-Site Scripting and the various forms of Injection.

I had the honor of being interviewed by Daniel Gackle on Domain-Driven Security as part of a series of DDD videos that Vladimir Gitlevich is producing, and I will surely let you know when it is online.

Daniel amongst others has also given me excellent feedback on my assorted ideas about Domain-Driven Security to give it some structure that is more easily accessible for someone that has not been deeply involved from the start. Hopefully there are some articles or other pieces of writing on their way. Not promising to much, but I will let you know.

You know well that one of my favorite phrases is "nothing is just a String" pointing out that a phone-number is more than any random string, that the same goes for a person's name, for an order number etc. This basically provides the case for typed value objects, typically with validation wrapped in the constructor.

John Wilander has brought this one step further. If it is questionable to ever have just a String, then should not String be an abstract class? Besides making an excellent point in a very well written blogpost, he also provides an example of injection besides the usual SQL Injection which often is easily dismissed as "a solved problem, use prepared statements". I really recommend reading his posting.

It seems to me like this particular week, Domain-Driven Security as a field is experiencing an unusual high level of activity.

Yours

   Dan

Ps. John Wilander recently defended his PhD thesis on software security. Whereas the full text is of course pretty long, he also included a "popular brief" in Swedish which in a few pages summarises his work. Well worth reading, and a good example I think more researchers should follow.

Tuesday 21 May 2013

Domain Driven Design to Speed us Up by Opening Options


Dear Junior

At the DDD Summit a year ago there was an interesting discussion about how we motivate doing Domain Driven Design. We noted that one of the classical argument have been "proper domain modelling will make maintenance and further development easier", an argument which I have used myself. 

However, this is really a bogus argument; it more or less boils down to saying "spending a lot of time now will save us time later", which is a non-argument for anyone buying into the agile view of creating software. We need a better argument - If we are doing DDD it should be because it saves us time now.

The general agreement last year was that domain modelling actually do speed us up now, so doing domain modelling saves time in the short term, e g the stories at hand in the current sprint if you are doing scrum. However, we did not much discuss the mechanisms that make it so.

I have had this discussion in the back of my head during the last year, trying to keep my eyes open for how this could be the case - how doing more work actually could result in less work.

At the DDD Summit this year I had a really rewarding discussion about this with Steve Bohlen, sharing our experiences from the passed year. 

If you do not do domain modelling, you will look at the story to implement from a technical perspective. Doing so there is probably an "obvious" way to do it - "run that job, select the data from the database with a new where-clause, fetch it via XyzService, and push it to presentation". The approach will include activities A, B, C, D, and E - so write down those as five tasks, post them as stickies on the sprint board and start working. How could extra domain modelling save us time? Will it really make us work faster?

In order to make sense, let us think about "saving time" in the opposite direction. Efficiency can be seen as working fast. However, it can also be view as "maximising the work not done", which is how it is phrased in the Agile Manifesto as the principle of simplicity.

One of the important mechanisms of domain modelling (if done right) is that it opens up options. In a domain modelling session you challenge yourself and the team to think about the problem in different ways, and a good modelling session might well result in three different ways of thinking about the problem - and thus generating three different solution. 

Evaluating three different solutions might give you that first include activity A, C, E, and F. Second include B, F, and G. Third include A, B, D, F, H, and I. Now of these some might be worse than the "technically obvious solution". In this case third solution is probably worse; six activities instead of five. 

Finding solutions that are worse than the one you had (the "technical")? Surely that must be a true waste of time!

However, you do not only find solutions worse, you most probably find some that is better. And, you only have to implement one of them - the best one. In this case, the simplest solution is the second, only requiring three activities. 

It is a little bit like throwing dice trying to throw an as low number as possible. Throwing one die will give 1 to 6 equal probability, so your average low will be 3.5. If you throw two dice, each die will give 1 to 6 equal. However, for 6 to be the minimum you will have to throw [6, 6] which is 1 chance out of 36. For five to be minimum you have to throw [5, 5], [5, 6], or [6, 5] - 3 out of 36. For three it will be 7 out of 36. For one it is 13/36. Throwing one extra die heavily skews the odds for throwing a lower throw ("finding a simpler solution").

Back to software creation, the simplest solution found during the domain modelling is most probably easier that the solution that was obvious from the technical perspective. A small investment in opening options was well outweighed by finding a easier solution. So, we save time not by working faster, but by working smarter.

And just to be clear - we are talking about a small investment in domain modelling, like a few hours or a day. It is not about a "phase of the project" were you spend weeks in day-in-day-out workshops and discussions.

During the year my colleague Daniel Deogun had a wonderful example of this happening at our common client. At the sprint planning the team was convinced that they knew how to implement the story at hand. However, just to open up options, Daniel kidnapped the team for a two-hour domain modelling session. During that time they found new ways of looking at what they were building. So they ended up not touching the code they had envisioned to change. Instead they changed somewhere completely different and the amount of work shrank from being a dominating part of a sprint to be a few half-days of work for a few team members. And that insight was driven by asking questions about the domain - trying to phrase the answer in different ways.

So, doing more work to save time might seem very unintuitive. However thinking about it as finding options to "maximise work not done", I think it makes sense. In a way, what we are avoiding is the sadness of a really fast boat sailing at high speed in the wrong direction.

Yours

   Dan

PS BTW, Steve and I had more interesting discussion around this topic, but that will need to be the topic of another letter.

PSS Using the metaphor of orienteering what DDD modelling do is to spend a while to study the map, exploring different routes. As an orienteering runner this is definitely a smart move to make us faster.


Tuesday 18 December 2012

Technical Details as Part of Domain Model

Dear Junior

A classical questions in Domain-Driven Design is whether technical details should be part of the domain model. The usual answer is "no, there should only be business concepts", but I would like to refine that a bit.

One of the drives of Domain Driven Design is obviously to drive development from a design based on the domain - hence the name. So, concepts as "database", "table", "network" and "connection" should typically not be part of the domain and its language.

This kind of technical details does not make sense per se in the domain - they add nothing do the direct understanding. However, databases and network and other technical details might have indirect effects that that we need to understand. The tricky part is to find appropriate abstractions for that understanding.

Let me take "buying a laptop" as an example. Recently I bought a new laptop from the Apple site. This is the "laptop to buy" model, and when doing this I filled out an order form, which is what constitutes the "laptop to buy" domain model.

I know that a laptop is a pretty complex thing, so the "laptop to buy" domain is a really rich one. Some of my hardware freak frieds could talk to lengths about it. As I am not much of a hardware freak myself, I prefer if the "laptop to buy" domain model is as simple as possible - without "technical details" that I am not interested in.

For example, the laptop I buy have a hard-drive, which is made by a manufacturer. I happen to know there are numerous subtle details that differ between hard-drive brands - but none that I care of. It is an uninteresting technical detail, and I do not want to see the choice of hard-drive brand in the domain of "laptop to buy".

Luckily for me, Apple agree, and there is no option for choosing hard-drive brand that makes the order form more complex and harder to understand.

Another technical detail is that the hard-drive have a lot of sectors and blocks, and the number of sectors and blocks differ from hard-drive to hard-drive. This is also something that I do not wish to care about - I just want to stuff my documents, photos, and film clips onto the laptop and it should just work. I do not want this to be part of the "laptop to buy" domain model either.

Unfortunately, Apple disagrees here. They think I need to be aware of that the hard-drive consists of storage cells, and that these actually are limited. Otherwise, I would live in the illusion that I could put things on my laptop indefinitely. That illusion would break down when there suddenly are "no space left on device" - an event which would leave me utterly confused.

In order to make me understand the nature of how it is to work with a hard-drive, I need to be aware of the finite number of sectors and blocks - although it is a "technical detail". But, unfortunately, it is a technical detail I need to be aware of.

Now comes the hunt for the appropriate abstraction to make the blocks and sectors understandable to non-hardware-technical me. "Blocks and sectors" does probably not work very well, but the concept of "size" does. I get that we are not talking about physical size, but some kind of analogy to the size of a suitcase. A small suitcase will become full fast, but you can put more stuff into a larger suitcase before it gets full. The size is still a "technical detail", but it is a detail that cannot be ignored, and it is put in a form I can understand.

Thus Apple has chosen to have options in the order form for how big a hard-drive I want, making it a part of the "laptop to buy" domain model. 

And I can agree with Apple.

When considering whether a concept should be in the domain or not, it is really not interesting whether it is a "technical detail" or not. The interesting discussion is whether this is a concept which the user (or other stakeholder) needs to be aware of. We cannot take away concepts that will make their understanding "break down". And for those technical details, we have to find an appropriate abstraction.

This reminds me of an Einstein quote: "Things should be made as simple as possible, but not simpler" which I think is an excellent way to put Occam's Razor.

So, technical details should not be part of the domain if they are irrelevant to the business. However, it might be that the technical detail is a concept that you need to understand to comprehend how things work. And in that case, it should be part of the domain, even if it stems it origin from the technical side.

Yours

   Dan

Wednesday 14 March 2012

DRY and False Negatives

Dear Junior

In earlier letters we have talked about the DRY-principle, how looking for duplicated code can help finding violations of DRY, but also about the false positives - the rare locations where unrelated concepts by coincidence happen to be represented by similar code. Using Domain-Driven Design terminology it is where two different concepts are modelled, but where the models happens to be represented the same way.

But the hardest part of DRY is the false negatives.

A "false positive" is when some kind of test raises a signal but does so wrongly. E g a medical lab test might say that you have a disease which you does not have. If you get a false positive you have to do further investigations before you realise that it was just a false alarm. In our case the false positives are where code that was alarmed as duplication, but actually was jus a coincident and no violation of DRY.

A "false negative" on the other hand, is when the test does not raise a signal but does so wrongly. E g a medical lab test says you are healthy, but you actually have the disease. Now, false negatives are a lot harder to handle as the few mistakes (wrong classification by the test) hide within an enormous pile of "perfectly normal cases" (i e the true negatives).

This is the reason why medical tests most often are designed to avoid false negatives even if that design means more false positives. In the medical field this is natural. The cost in money and suffering in a missed AIDS-diagnosis totally overshadows the costs and anxiety of having to run a few extra tests in case the initial test wrongly showed "infected".

Talking about to DRY and code: what are our false negatives? Well, it is those DRY-violations that does not show up as code duplicates. Using DDD terminology we have exactly the same concept being modelled, but it is represented in different places in the model, and the representations differ from each other. Code-wise, we have pieces of code that fulfills the same purpose in the solution, but are written in a different way.

We return to our online sales system as an example to make that clear.

In the customer management there might be a method that validate that zip-codes are five-digit strings. Some programmer wrote the regexp as part of an if-condition and at some later point of time some other programmer extracted it to a metod.

class CustomerManagement {
    private boolean validateZip(String zip) {
        return zip.matches("[1-9][0-9]{4}");
    }
}

Totally unaware of the private method in CustomerManager, some third programmer was working on the shipping logic. This programmer wanted to avoid printing invalid zip codes on packages so he looked for some feasible method in the standard APIs for a minute or two before giving up and whipping together his own helper method. Also, this programmer was not so well versed in regexps so he found another way of validating.

class Shipping {
    public static boolean isValidZipCode(String zipcode) {
        try {
            int i = Integer.parseInt(zipcode);
            return Integer.toString(i).length() == 5;
        } catch (NumberFormatException e) {
            return false;
        }
    }
}

(BTW, these examples are inspired be real code-bases)

This is where we suddenly have a tricky instance of DRY-violation. We express the exact same idea - the criteria for how a valid zip code is formatted. But we do so in completely different way - so different that no code-duplicate detector will find them.

It would be far from tasteful to compare the direct human suffering of failing medical tests with the effects of DRY-violation. Nevertheless, in medical tests the false negatives are far more troublesome than the false positives. And the same holds for violations of DRY. The false positives we can find and discuss, but the false negatives will just silently create implicit couplings between different parts of the code - couplings that are hard to find and might break when the concept change but only a few of the representations are updated. Also, it leads to our code getting bloated.

In summary I dare to make an analogy to medicine. When looking for the DRY-disease, we will get a lot of help from testing for code-duplication. There will be some false positives that we need to keep our eyes open for so that we do not try to cure where there is no illness. However it is the false negatives that the easy test does not find that will cause us the most trouble in the long run.

Yours

Dan

Ps My colleague Anders Helgesson phrased this as "Man ska inte upprepa sig och säga samma sak, om och om igen, flera gånger" wich in translation is "You should not repeat yourself and say the same thing, over and over again, many times" - a wonderful example of a DRY violation where searching for duplicated text gives a false negative.


Wednesday 7 March 2012

DRY and Duplicated Code

Dear Junior

In my last letter I wrote about how the DRY-principle (Don't Repeat Yourself) talks primarily talks about the number of times an idea is expressed in the code-base, not about text-repetition as such.

If nothing else, the intention of DRY becomes clear when you go back the source that caused the DRY-meme to spread within the programming community: the Pragmatic Programmer by Andrew Hunt and David Thomas, published in 1999.

"Every piece of knowledge must have a single, unambiguous authoritative representation within a system" 

(Needless to say, when reading this book I found its core ideas to be extremely well aligned with my own ideas about programming - and the rest of the book have had profound impact on my view of the craftsmanship aspects of programming)

Now, to put my last letter in perspective it must be pointed out that the absolutely easiest way to find violations of the DRY principle is to search your codebase for duplicated text. It is very likely to find multiple places where the same idea is expressed over an over again using the same technical construct.

If for example you have an order system, there might be an attribute "payment" on the order telling when the order was paid.

class Order {
   Date payment = null;
   Date payment() { return this.payment; }
}

Most probably orders are not allowed to be sent for shipping unless they are paid. So, very probably there will be code somewhere checking this condition.

if(order.payment() != null) // do shipping
else // do not allow shipping

It is also probable that the same check is done in a lot of other places throughout the codebase. What we have ended up with is that the same technical construction "order.payment() != null" is used to express the same idea "order is payed" in a lot of places - a clear violation of DRY. Most probably we will also find a lot of places checking the negation "order.payment == null".

Of course the codebase would benefit from encapsulating the interpretation of a nulled payment-field and putting that encapsulated interpretation in one place - preferably the Order class.

class Order {
   Date payment = null;
   Date payment() { return this.payment; }
   boolean isPaid { return order.payment() != null; }
}

The benefits are several. To start with it is now possible to change the technical representation of an unpaid order without having to run around the code-base. We actually want all those places in the code to change at the same time - because they conceptually related things (actually they are conceptually the same thing).

An even more important benefit is that the API of the Order class becomes more expressive, making the client code more lucent.

if(order.isPaid()) // do shipping
else // do not allow shipping

Obviously, searching for duplicated text is an excellent way of finding candidate violations of DRY. If the code looks exactly the same, then those places will need to change at the same time - i e we have implicit couplings in the codebase that we risk breaking when we change.

There are excellent tools out there to detect duplicated code - some bundled in code-quality tools like Sonar, some available directly in your IDE. Most of these even have support for "fuzzying" where they detect that code is duplicated "apart from different naming of variables" for example.

Using these tools on an "average codebase of the street" will probably yield hundreds of places with duplicated code. Of these candidates, the vast majority of them will be violations of the DRY principle - especially the short code-snippets like "order.payment() == null" which are so easy to "just write" instead of looking for a method that does the job.

However, be aware that within that huge pile of DRY-violations there might also be a few "false positives". Those are the code-places that by accident happen to look the same way, but actually represent different and unrelated ideas. To "reduce the common code" in that case would only cause a irrelevant coupling between unrelated parts of the solution.

In summary, looking for duplicated code is an excellent "screening test" to find loads of candidates where the DRY-principle has been violated. However, the DRY principle is not about duplication of text, but about duplication of ideas. So, the two should be kept apart.


Even if duplicate code most often is a sign of DRY-violation, there are valid reasons to have the same code-text in two different places. 


Jérémie Chassai phrased it pretty well on Twitter: "You should repeat yourself if you say the same thing for two different reasons".

Get those tools, start searching for duplicates, and good DRY-hunting

Yours

Dan

Thursday 1 March 2012

DRY is about Ideas, not Text

Dear Junior

Ever since the Pragmatic Programmer, the principle of DRY (Don't Repeat Yourself) has been one of the central pieces of advice for good programming. From a private perspective, it has many a time helped me to write code that is better than had I not had that meme in the back of my head. However, I have also seen it misused and (in my understanding) misunderstood several times - sometimes with disastrous result.

What I think is central about DRY is that it refers to ideas. Slightly rephrased DRY can be expressed that "each idea about your software solution should be expressed once and only once in your code". With "idea" I mean stuff like "an order number is a six-digit string not beginning with 0 or 1", or "an order that is cancelled after shipping will still be debited the costs that we cannot recover".

Unfortunately, a common misuse is to let DRY to talk about code as text. Let me make it a little bit more clear though an example. 

Say that we have some kind of order system where order numbers are five-digit numbers. Somewhere we might find this code that checks whether a string is a valid order number or not.

       public static boolean isValidOrderNumber(String orderNumber) {
            return orderNumber.matches("[1-9][0-9]{4}");
        }

The code might be in the Order class, or even inside an OrderNumber value object, and possibly being called by the constructor.

As it is an order system, there will also be a part that talks about shipping, to get the goods to some specified address. The address consists of a lot of things, among those a zip code that in this country is a digit string of length five (initial digit must be non-zero). So, somewhere we can expect to find the code doing this validation.

       public static boolean isValidZipCode(String zip) {
            return zip.matches("[1-9][0-9]{4}");
        }

This code might be in the Address class, or inside the ZipCode value object - possibly called by the constructor.

Now, this is where some DRY-zealots shout "duplication, duplication" because they have pattern-matched the string matching in isValidOrderNumber with the string matching in isValidZipCode and noticed that they consist of exactly the same text (barring a rename of a variable).

I think this is a mistake, and those zealots focus on the wrong thing. 

In the eyes of the zealots, the textual duplication is bad. Instead one method should call the other. This obviously becomes bizarre code if you try it. For example it would create a completely irrelevant coupling between the zip code and the order number - two concepts that are not related at all.

Alternatively, they claim, the common code should be broken out to a separate method called from both places. Now, that might make more sense, but you still introduce a coupling between the un-related concepts order number and zip code: they will now covariant - change one and the other will change.

For example, should this company decide to use letters in their order numbers, then they need to regression test all their use of addresses as well.  

Do this kind of coupling on a massive scale, and you will end up with a system where every change is full of surprises - conceptually totally unrelated concepts start behaving different just because they had some code-snippet in common.

What to do instead?

We should not primarily look at the code as a mass of text. Instead we should look at it as representation of a set of ideas. This is of course the perspective of Domain Driven Design, where we see the code as the encoding of a distilled domain model - a model that captures our selected way of looking at the problem domain.

Now, seen from that perspective the "repeated code text" in the two validation methods is completely unproblematic. It is just two separate concepts that happens to be represented using the same technical mechanism. They have nothing to do with each other - the duplication is a pure coincidence and the two concepts should be able to change totally uncoupled from each other.

The principle of Don't Repeat Yourself is about ideas, it does not apply to text.

Yours

   Dan


Friday 16 September 2011

Public final and data encapsulation


Dear Junior

We were discussing immutable value objects and using "public final" data-fields for their representation. In that discussion I started off mixing up immutability and encapsulation. Now that we have covered immutability, let us return to encapsulation.

Obviously, declaring a field as public will break data encapsulation. You lose your freedom to change data representation without having to bother the clients.

Let us have a look at a name-class with some actual usage.

public class Name {
    public final String fullname;

    public Name(String fullname) {
        if(!fullname.matches("[a-zA-Z\\ ]+"))
            throw new IllegalArgumentException();
        this.fullname = fullname;
    }

    public String[] names() {
        return fullname.split(" ");
    }
}

The public API of this class consists of three parts: the construction of a name from a string, the attribute “fullname” (accessible through property data field), and the attribute “names (accessible through accessor method).

An example of what client code looks like we can find in the tests.

public class NameTest {

    private final String danbjson = "Dan Bergh Johnsson";

    @Test
    public void shouldHaveFullNameAsAttribute() {
      Assert.assertEquals(danbjson, new Name(danbjson).fullname);
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldNotAllowWeiredCharsInName() {
      new Name("#€%&/");
    }

    @Test
    public void shouldSplitIntoNamesAtSpaces()  {
      Assert.assertEquals(
        new String[] {"Dan", "Bergh", "Johnsson"},
        new Name(danbjson).names());
    }

}

Imaging that we want to change the internal data representation to using a char-array instead. Doing so will break all the clients as they rely on having that public data-field “fullname”. Thus, it is a bad design. Or?

I would argue that it is still a good design. Using modern tools it is easy to add the encapsulation when needed. Applying "Encapsulate Field" in e g IntelliJ yields.

public class Name {
    private final String fullname;

    public Name(String fullname) {
        if(!fullname.matches("[a-zA-Z\\ ]+"))
            throw new IllegalArgumentException();
        this.fullname = fullname;
    }

    public String[] names() {
        return fullname.split(" ");
    }

    public String fullname() {
        return fullname;
    }
}

And of course the client code has been changed accordingly.

    @Test
    public void shouldHaveFullNameAsAttribute() {
        Assert.assertEquals(danbjson, new Name(danbjson).fullname());
    }


Now, I could have chosen "getFullname" as the method name for the new method. However, I have always found that naming convention a little bit awkward, the property is “full name” and adding a boilerplate “get” does not add any value in my opinion. By the way, JavaBeans is just one naming convention in Java, the naming convention for CORBA predates JavaBeans. In the CORBA convention if you have a property “fullname” of type String, then the way to access it was to call a method “String fullname()” and the way to change the property was to call a method  “void fullname(String)”. So the convention I use is not new to Java at all.

In some languages there is no distinction in syntax between accessing a field and calling a no-arg method. Had the code been written in Eiffel or Scala, the syntax would have been the same and there would be no change to the client code at all.

Now that we have encapsulated the usage via “fullname()” we can change the internal representation to a char array.

public class Name {
    private final char[] chars;

    public Name(String name) {
        if(!name.matches("[a-zA-Z\\ ]+"))
            throw new IllegalArgumentException();
        this.chars = name.toCharArray();
    }

    public String[] names() {
        return new String(chars).split(" ");
    }

    public String fullname() {
        return new String(chars);
    }
}

Just for reference, in Scala the corresponding change would start with this "final public" representation – here represented by the keyword “val”.

class Name(val fullname: String)
{
  if(!fullname.matches("[a-zA-Z\\ ]+")) throw new IllegalArgumentException();

  def names = fullname.split(' ')
}

The change would take us to the slightly more verbose char array representation. Here we have no “val” in external class declaration, but a private val-field hidden inside the class-block instead.

class Name(name: String)
{
  if(!name.matches("[a-zA-Z\\ ]+")) throw new IllegalArgumentException();
  private val chars = name.toCharArray

  def fullname = chars.toString

  def names = fullname.split(' ')
}


In conclusion: As the public final field is accessible by the clients, it is also a part of the API for Name: This breaks data encapsulation. If you want to change data representation, you will need to create an accessor method to which you direct the client.

In Scala or Eiffel this is a no-issue, as the new accessor could transparently have the same name as the old datafield ("fullname") and be accessed with exactly the same syntax ("name.fullname"). However, in Java you have to include an empty pair of parenthesis to call the accessor method - thus the client code must be changed.

Now, I consider this a small issue, as there is excellent refactoring support in modern IDEs that eliminate the change to a fully automated four-click, one-minute exercise. Thus, there is no point in designing for that change up front.

So, even if we *do* break data encapsulation, I would say that it is not much of a problem.

Now, there are other kinds of encapsulations that are more interesting than data encapsulation, but that analysis will have to wait for some other letter.

Yours

   Dan