Code review

The hidden dimensions of code review(s) – part 2

Some dimensions will be treated in this blog post and on part 3 of this blog posts series(see part 1 here) , my friend, Ionut Albescu will help out with the dimensions and sub-dimensions which were left out.

The dimensions

■ Programming(Design, Clean Code, Refactoring, Unit Checking)

There are a lot of materials covering these aspects. Just search for books by James Coplien, Robert C. Martin, Martin Fowler, Michael Feathers and you will get an idea of each of these topics. Usually, in appearance, this is the dimension were code review is concentrated. I said in appearance because these topics cannot be mastered in a month or two, not at all. But is more, although management/customers/end users will expect programmers to excel in this… well is not quite like this…

Programming – Impact

This sub-dimension is to make the programmer aware of how the functionality she/he implemented will impact the final user. How many programmers think that because of their work an end user can be fired?

■ Testing

Here I am thinking about Context Driven Testing(1) school of thought, especially Rapid Software Testing(2). This testing has its roots in cognitive psychology and epistemology(3). Please read the work of James Bach, Michael Bolton, Cem Kaner, Jerry Weinberg.

If you are a developer and want to ignore this dimension than I advise you to think better. You will lose the chance to be an even better professional.

Testing – History

James Bach, some time ago, pointed me out about the book “Your Code as a Crime Scene: Use Forensic Techniques to Arrest Defects, Bottlenecks, and Bad Design in Your Programs”. I was thrilled to see how many things can be deduced just by looking in the source control history. I was able to see the weak points of code and where to concentrate the refactoring. It was there in front of my eyes the Paretto principle, helping me in prioritizing work, but also helping testers in guiding their test strategy.

Testing – Testability of the application

We need to give the application control points so the testers, programmers can exercise more easily certain scenarios. For example replacing more easily components. If we can’t do this, then is a red flag.

Testing – Risks

4 sub-dimensions listed here are from the wonderful paper “Risks Analysis Heuristics (for Digital Products)”(4): project factors, technology factors, specification factors, operational factors. Studying this paper made me realize that coding is not just about if, else, while… instructions.

The tester(s) have a different mindset, which should be viewed as a complementary to the one we already have, as a developer. Developers look at the code with the assumption that is right, but we need testers to look at the code with the assumption that is wrong(they are negative, they focus on failure(3)).

■ Business

All the code should be driven by the business need. Use cases/activities should drive the code and it’s structure. How do you know that a duplication is not just a temporary one which in the end will no longer exist? How do you know that you are applying ok the Single Responsibility Principle? Well guided by the business, the actors.

Business – Analysis

I remember we discussed the heuristic “Mary had a little lamb”(5). This heuristic help identify ambiguity statements which are in written form. The secret is to concentrate on each word one by one and then in combinations. Let’s take the word “Mary”. We concentrate on this word by posing questions: Why Mary? Why not Giovanni? Let’s take the word “had” from the sentence: Why it had? What happened that no longer has it? What about present, future? Will it have another one in the future?.

I love this technique. A lot of programmers want and expect fully written requirements without side effects. But is not possible because:

– it is a lot of tacit knowledge;

– is one thing for people to see the data(let’s not forget that even with the highest concentration we partially see/scan stuff), one is to pay attention to it and a different thing to act upon that data. (6)

Most of the time we go on the supposition that we know what customers/end users want. But is it like that? What if the end user does not know? What if the end user is unaware of certain things, on which we might help, which are possible and they may want it? Why to code a thing and put a lot of effort in abstractions, algorithms when … maybe … that is not the path to follow?

Is time for a lot of programmers to grow up and not ask for things just because it will be easier for them. Or search for excuses not to do work. They will code certain business things. This means they are directly responsible to understand and make sure they understand those business things. Christopher Alexander put it very nicely: “Please forgive me, I’m going to be very direct and blunt for a horrible second. It could be thought that the technical way in which you currently look at programming is almost as if you were willing to be ‘guns for hire.’In other words, you are the technicians. You know how to make the programs work.‘Tell us what to do,Daddy, and we’ll do it.’That is the worm in the apple.”(7)

Business – Impact

Here is about building products and delivering projects that make an impact, not just ship software.

There is a technique for doing this which is called Impact Mapping(8).

Impact mapping is a planning technique which uses in a very inspired way guiding mind maps in helping achieve this by guiding/facilitating the process. It shifts the focus from just doing what the customer orders to focus on collaborating with all involved parties. The goal of the impact maps focuses on the question “Why are we doing this?” and gives a strong guidance on how to do this. This work collaboration between senior technical and business people is a must because the code changes based on this interaction. The abstractions, objects, architecture is strongly influenced by this. The business must be known by senior programmer – is a sine qua non condition for the seniority of the programmer.

Business – Demo

At the end people will see the result of the code, not the code itself. This result is the desired effect. This means that the end user, the Product Owner have to see it and the programmer to get an early feedback. So this aspect can help the developer to be ok/prepared when doing the Demo.

■ UX

Most often the work of the programmers is reflected in the UI. The final user does not see what is underneath. I really like how James Coplien makes the connection between object oriented programming and UX. Is normal because via UX we try to translate as much as we can the mental model of the user and then that mental model must be reflected in code.

■ Human

Very often we forget the social aspect of programming. We do not realize that architecture, for example, has a social aspect. This is very important.

Is not easy to integrate and make work two or more, different, personalities. I wrote before about this. There can be a conflict, a political stuff, sadness, ….All this can affect and will affect the quality of the code review.

In the mind map we put the word “Affinity” to be aware of all the subjectivity that can be present.

We put the word “Experience” because:

– of seeing bad examples. People who were considered seniors but they were affecting in a bad way how the review was done. Also the other team members, the juniors one, were influenced by these “seniors”;

– the experience plays a role in review because of the learning that can be done. Having an experienced developer that can share knowledge hands on;

– people with no programing experience , like some testers, were posing interesting questions. And the explanation towards them had to change a little bit;

About inattentional blindness, I wrote before about this aspect and code reviews.

Partial Conclusion

– Trygve Reenskaug had an interesting idea: the reviewer having the responsibility of the code being reviewed, not the programmer who wrote it. That for sure will change the dynamic of code reviews and it can make code review(s) more valuable/interesting not just a monoton approve step(9);

– After we (Alexandra Vasile, Marius Jantea, Ionuț Albescu and I) created the mind map and thought together at those dimensions and explanations I realized that in my subconsciousness I might have been influenced by the wonderful book named “Handbook of Technical Reviews” written by Jerry Weinberg(10);

– Seeing how code reviews are being done/treated gave me a clue regarding the dynamic of the team and possible sociological/management/organizational problems that might be there. I say this because a characteristic of complex systems is fractality;

– Do you feel to modify the mind map, to add more dimensions and sub-dimensions? Good;

On Part 3 of this blog post my friend Ionuț Albescu will continue in clarifying the other dimensions which were left out and also will give his personal touch on this subject of code reviews.

(1) Context Driven Testing, http://context-driven-testing.com/

(2) Rapid Software Testing Methodology, https://www.satisfice.com/rapid-testing-methodology

(3) James Bach, “Lessons Learned in Software Testing: A Context-Driven Approach”, https://www.amazon.com/Lessons-Learned-Software-Testing-Context-Driven/dp/0471081124#

(4) James Bach, Michael Bolton, “Risks Analysis Heuristics (for Digital Products)”, http://www.developsense.com/resources/RiskHeuristics.pdf

(5) Jerry Weinberg, “Exploring Requirements” https://leanpub.com/b/requirements#bundle-page-exploringrequirementsone

(6) Dave Snowden, “See-Attend-Act”, https://cognitive-edge.com/blog/see-attend-act1/

(7) Christopher Alexander, “The Origins of Pattern Theory: The Future of the Theory, and the Generation of a Living World”, https://pdfs.semanticscholar.org/7157/ee5a77c6fd64f8b8b2282225f113205370e9.pdf

(8) Gojko Adzic, “Impact mapping”, https://leanpub.com/impact-mapping

(9) Trygve Reenskaug, “DCI: Re-thinking the foundations of object orientation and of programming”, https://vimeo.com/8235394

(10) Jerry Weinberg, “Handbook of Technical Reviews”, https://leanpub.com/handbookoftechnicalreviewsfourthedition

The hidden dimensions of code review(s) – part 1

Context

The title image was created last year in December with the help of three friends (Alexandra Vasile, Marius Jantea and Ionuț Albescu) over lunch. Since then, I try to write about this subject. The trigger point was when, again, I saw a collocated team using exclusively a tool for code reviews and no personal discussion. It was clear to me that I was seeing some effects of bad things that happened there before and people were hiding themselves behind tools – is so easy to do this.

When I asked them why they did it in this way, they had a perfect excuse: audit. Smart things can be done so that an audit will not generate stupid behaviors. I saw that in our industry, at least here in my area, this audit is a good excuse for bad behaviors/recipes. I think there is a gross misinterpretation of the audit intention and on what should be done.

So in this case, as in many others, it was about abnormal/bad work environment, lack of understanding subjects/topics involving our day to day work(audits, code review(s), testing,… ).

The good thing is that it forced me to think very seriously on code reviews and what it meant for my friends and I. I have to say that when discussing with my friends, I had in mind the way how James Bach and Michael Bolton analyses certain things(1).

Why is so important

I continuously saw how this subject, in my opinion, was badly treated. But why it irritated me so much? Well, among other things, because:

– this is how I learned a lot of programming stuff;

– it is so obvious the connection with testing and how helpful is to find bugs;

-it helped me see the connection with the idea of complexity(those things which are named boundaries and how the system will adapt/shape/modelate based on those) (2) ;

– the serious analysis which is being(should be) done can help us arrive at profound stuff;

Checklists

There was a time in my professional life when it was enough to say the words “checklist” and “tool”, when a problem popped out, and everything would have been OK in the eyes of certain managers. I think that those managers wanted simplicity, but most often we deal with an essential complexity. Somehow I felt that, for those managers, the situation awareness always could have been reduced to some simple/obvious things in a very fast way. Everything was reduced to recipes.

Context is everything. This means situations can have different dynamic. I am afraid that for those managers all the situations were all simple/obvious( in Cynefin terms)(3). So for them it was assuring that having that checklist would have acted as a guard with, almost, ad litteram actions described in that checklist – and it makes sense for the Obvious domain in Cynefin, but we are not always there, not at all.

What if a checklist in complex domain, or liminal zone between complex and complicated, has a different form/significance? What if it means something else? I think that at this level we speak about compressed knowledge, cognitive activations, heuristics. They can’t be used ad litteram, is a lot of uncertainty. I understood this when I had the Rapid Software Testing course with James Bach. James had a list of different words regarding testing on a slide, it was a huge slide full with words. Someone could have transformed that list in a checklist but…not ok. Each word from there could have been described in a lot of text and lots of actions dependent on context.

This is how I realized that a professional expert might use a checklist in a different way than a junior would use it.

Maybe also with checklists, in these kind of contexts, the secret is … checklists “all the way down”(4) – and this is a whole different story, for sure not a simple one. James Bach said it so nicely: “Heuristics are often presented as a checklist of open-ended questions, suggestions, or guidewords. A heuristic checklist is not the same as a checklist of actions such as you might include as “steps to reproduce” in a bug report. It’s purpose is not to control your actions, but help you consider more possibilities and interesting aspects of the problem.”(5)

So, to come back to checklists as we know them which are used, most often as I saw it, for process compliance, just tick in the box – which for certain people, not professional who play good politics, can use to cover their ass :(.

Pull requests

Instead of hearing the words “code review”, I often hear the words “pull request”. And of course the link of that “pull request” was sent by chat to the collocated colleagues. I would like to say that this is something new for me, but it is not. But what is even stranger, is those collocated people decide to do exclusively the code review via the tool. This reminds me of the team who was doing daily meetings via Skype although they were in the same office…

I know there can be good scenarios to make code review via the tool. But to have it exclusively, no matter what, via the tool for collocated colleagues is rather problematic.

The justification for pull request(s) most often is, again, the audit.

Why dimensions

I noticed that code review has only a narrow perspective in the eyes/actions of many people. Some aspects, intentionally or unintentionally, are not considered, like: the environment, the context of where that code lives, what generated that code, the possible unintended consequences of the code,…

Is not enough to concentrate on those lines of code only. I say this because of 2 reasons:

– if something is not seen/observed does not mean it does not exist.

– I think that code has a complex aspect in it. This means that we have to see the connections also. Because in complexity is all about connections.

The title image from this blog post was created to show the possible dimensions worth considering, at a certain moment, when doing a code review. It is a guideline.

Depending on the moment when code review is being done, certain dimensions(see the image below) can be ignored while others will matter more. For example, in the image below, I want to concentrate on seeing how SOLID(6) principles were applied guided by the business need:

In the part 2 and part 3 of this series we (Ionut Albescu and I) will tackle these dimensions.

(1) James Bach, Michael Bolton, ”A Transpection Session: Inputs and Expected Results”, https://www.developsense.com/blog/2010/05/a-transpection-session-inputs-and-expected-results/

(2) Dave Snowden, “Boundaries”, https://cognitive-edge.com/blog/boundaries/

(3) Dave Snowden, “Cynefin Framework Introduction”, https://cognitive-edge.com/videos/cynefin-framework-introduction/

(4)”Turtles all the way down – is an expression of the problem of infinite regress” , https://en.m.wikipedia.org/wiki/Turtles_all_the_way_down

(5) James Bach, “Heuristic Risk-Based Testing”, http://www.testingeducation.org/course_notes/bach_james/cm_2002_rapidsoftwaretesting/rapidsoftwaretesting_13_heuristic_risk_based_testing.pdf

(6) Robert C. Martin, “The Principles of OOD”, http://butunclebob.com/ArticleS.UncleBob.PrinciplesOfOod

Code review discussion

Ri: Do you make code reviews?(1)

Shu: Yes. Every time we make a commit we have to ask for two reviews. And reviewers must put a comment.

Ri:You are explaining to me the formal process, which should not be confused with the quality of the actual review. Why are you doing that?

Shu: aaaa, because we must do it. It is mandatory. No commits should be made without code reviews.

Ri: So, for example, you use it for knowledge sharing?

Shu: Yes yes,exactly

Ri: Do you trust the reviews?

Shu: Of course, I can show you no commit was done without them.

Ri: That’s not what I asked you.

Shu: Yes, I trust them

Ri: What level of experience do your team members have?

Shu: hmm.ughhh…confirmed(2-3 years of experience)

Ri: And they do the review between themselves?

Shu: Yes, of course.

Ri: Hmm, how can you trust those reviews and the technical quality of that code when, actually, the persons who made them doesn’t have that much experience in writing code?

(1) Alistair Cockburn, “Shu Ha Ri”: http://alistair.cockburn.us/Shu+Ha+Ri

Inattentional blindness and code reviews

I already spoke about inattentional blindness in the context of testing in a previous post: Inattentional blindness and test scripts. But there are implications also for code reviews.

So, “We asked 24 radiologists to perform a familiar lung-nodule detection task. A gorilla, 48 times the size of the average nodule, was inserted in the last case that was presented. Eighty-three percent of the radiologists did not see the gorilla. Eye tracking revealed that the majority of those who missed the gorilla looked directly at its location.”(1)

But how can the effects of inattentional blindness can be reduced?

”A more effective strategy would be for a second radiologist, unfamiliar with the case and the tentative diagnosis, to examine the images and to look for secondary problems that might not have been noticed the first time through.”(2)

So, it does make sense to do a code review because things might have gone unnoticed. But with complex systems the important thing is the granularity. This means that we have to understand who makes the review and see possible implications.

For example, let’s say a novice has written some code and another novice will make a code review. The junior reviewer, possibly, will understand where and why that code was written like this. But is it enough? I would say no. A senior/advanced developer have to review the code because:

– can share experience;

– has deep technical knowledge(oop, architecture, clean code, properly deal with legacy code, TDD, unit tests, algorithms);

– is capable to have more perspectives.

Is this still enough? I would say no. A tester can take a look at the code with the intent to help him/her with the testing.

There are some lessons here:

– a novice can participate to a review because is a good occasion for him/her to learn. It will also offer a good occasion to be aware of the context and give a feedback;

– is not ok when the code review is not made by a senior because slowly slowly things will get in such a bad shape that it will be difficult to repair;

– a senior developer might miss certain details;

– a manager should not feel comfortable just because the process does not allow a commit of code in source control without reviews. Even if that process might say something like this: “a review must be made by at least one senior developer”;

– it is related to testing in the sense that the tester is searching for information which will be used in designing the tests;(3)

– this should not be used by developers as an excuse for a lack of professionalism.

This is praxis(4)….

(1) Trafton Drew, Melissa L.-H. Võ, and Jeremy M. Wolfe, Psychological Science September 2013;24(9):1848-1853, “The Invisible Gorilla Strikes Again: Sustained Inattentional Blindness in Expert Observers”: http://search.bwh.harvard.edu/new/pubs/DrewVoWolfe13.pdf

(2) Daniel Simons Christopher Chabris, “The Invisible Gorilla” https://www.amazon.com/gp/aw/d/B009QTTYCQ

(3) James Bach, Rapid Software Testing class discussion [2017]

(4) Dave Snowden, “Of practical wisdom”: http://cognitive-edge.com/blog/of-practical-wisdom/