Release It – 2nd edition – part 2!

Chris talked again about Release It 2nd edition.

Last time, Chris talked about “Creating Stability” – things that can go wrong, and how to prevent that.

The next section “Living in Production” is about how a system works in production. Part of this is physical (networks, IPs, etc.). There can be clock problems particularly with VMs. It covers “12 factor apps” – which we’ve discussed before in the context of microservices, coming from the microservices ideas, this is all about making the app not depend on things on the box.

We discussed “Stucco apps” – where if you install subsequent versions 1, 2, 3 of an app on a box, then there will be bits of version 1 and 2 left over – so the app isn’t exactly any of those versions. Instead, you should rebuild from scratch each time (you could use Nix and NixOS for this…). We also discussed configuration – getting environment-appropriate configuration onto each box.

We had a digression about ambient sound from services – like putting microphones in the JET torus – so you can tell whether the system is running normally. Because humans are good at recognizing unusual noises or unusual changes in noise patterns, this can let you pick up on patterns of behaviour that aren’t otherwise obvious.

We talked about setting up logging to demonstrate that the high-level goals of the system are being met. For example, in some systems, it might be really important if page loads have become slow, or users cannot log in, or if the number of purchases per hour has significantly dropped; and these are the important business needs rather than just whether a box is up.

We briefly discussed the merits of the Unix command “uniq -n” for monitoring services; for example to find the counts of unique ID addresses. This is very useful for spotting patterns in your logs.

When upgrading data in SQL databases, the upgrade path is typically straightforward – you migrate it all via a migration, or the apps don’t work. In NoSQL databases, there is no schema, there may be multiple clients using the data imposing their own restrictions, and so it’s not so straightforward. The author suggests a “trickle then batch” approach of first converting the high priority items, then after a while, converting all the other items.

We talked about API changes, and contract tests created by consumers, and versioning of APIs.

The final part of the book is about systemic problems – a grab bag of issues that didn’t come up elsewhere. Load testing scripts can be overly polite and well behaved, and then sites break when hit with real users that aren’t well behaved – so the load testing scripts should be more impolite. We talked about chaos – we’ve discussed chaos monkeys before, but there are various refinements to this idea. For example, a default “opt-in” for chaos monkeys, with the ability to opt-out if your service cannot tolerate chaos. Also, a “zombie apocalypse” – you send home a bunch of people, and see whether any of them are indispensable or not.

Managing state with VueX

Tim talked about Flux, Redux and VueX, for managing state in JavaScript applications.

The big change between Flux and Redux was that in Flux there was one store per feature; in Redux the entire application state is stored in one central object. This makes it easy to serialize and debug. Changes to the store are made via a “reducer” – which is a function that takes the current state, and transforms it into a new state – similar to the way that state is managed in Elm. To help organize the state, then you can break it down into a set of smaller views of that state. Because you’re abstracting all the data into one place, then the components themselves can be simpler and dumber. This makes the components themselves easier to test.

VueX is a reimplementation of Redux, but focussed on Vue. While you can use Redux with Vue, it makes more sense to use VueX since it integrates better with Vue’s properties and events. The VueX dev tools have built in support for VueX, and they list the actions that are triggered at any point, showing when the state changes, and allowing you to step back before that. Also, there are libraries to serialize your entire state to local storage whenever there is a change, which makes it harder to lose state if you close your browser.

We discussed exactly what “state” means, and how much you want to have stored in the VueX state. It comes down to what is useful to you – you don’t necessarily need to store everything, just the things you care about.

Rich talked about a current project that’s using VueX; with a wizard that collects data from the client across multiple pages. This uses VueX to manage state.

We discussed when VueX was worth using. It adds flexibility, but it also adds complexity and means that you are distributing the code to manage state to more places. The recommendation from the Gitlab blog is to use VueX always. Tim doesn’t completely agree – it does add some extra boilerplate, but on the other hand it’s good discipline to make what your application does clearer. He’d use VueX for a new single page app, perhaps whatever the side. Rich on the other hand thought that it was more important to consider whether there is state on the client side that needs to be remembered between pages – so the wizard app needs that, but an app where the state is always synchronized with the server doesn’t need it.

Rich also mentioned a prefetch option in Vue Router, which can potentially make apps using the router perform more quickly.

Release It!

This week, in our dev meeting, Chris talked about Release It! (second edition) by Michael Nygard. He’d read the original years ago, before “dev-ops” existed, and it was the first time he’d really thought about putting applications into production. It was an eye-opener 10 years ago, so he was excited by the second edition.

The book discusses stability patterns and antipatterns; then about things to consider in production; then about deployment; then about evolution of applications. So far, Chris has only read the first two parts of the book.

It discusses “Designing for Production” rather than “Designing for QA”; starting with a case study about how a small problem escalated into a huge problem. Some approaches are:

  • “Testing for Longevity” – identifying things that will fail after X continuous days running, which might not be picked up by standard CI approaches
  • limiting chain reactions between servers if there’s a failure causing domino effects (often by blocked threads building up)
  • self-denial attacks like sending out mass emails that trigger everyone to look at your site simultaneously
  • dog-piling, which is a bit like the thundering herd problem, e.g. if everything server is updated on a schedule exactly on the hour it causes peaks in load
  • automated tools going out of control, so applying limits to them is good;

Positive approaches to avoid these are:

  • timeouts
  • circuit breakers – isolate your system from the remote system when the remote system is down, providing an immediate “fail” response rather than blocking and running out of threads
  • bulkheads – e.g. reserving a few threads for the admin tool to fix the problems, or the operating system saving some disk space so the root user can fix out-of-space problems
  • steady state – e.g. log files shouldn’t become infinite in size, they should be tidied up; temporary files should be deleted
  • fail fast – do validation as soon as possible, if there are expensive operations that require data, then get that data upfront
  • for websites, distinguish between 40x errors and 50x errors – a user entering bad data shouldn’t trigger a circuit breaker
  • let it crash – if something goes wrong, let it crash and then restart it, but this requires other architectural choices like supervisors
  • handshaking – typical REST services don’t have any handshake involved; sometimes it would be useful if they could return an HTTP 503 to their caller to tell it to slow down
  • asynch approaches – a pull system can choose the rate at which it receives content
  • shedding load – dropping load if it can’t take any more
  • back pressure – telling your caller “back off, I can’t take any more work”
  • governor – applying limits to the automation tool, so it can’t fire up 10,000 boxes immediately – if it’s outside the standard range of what it does, then it will do it more slowly

When building a system using an Agile approach, then you need to decide when to start applying these techniques – since they may not be expressed directly in user stories, and they may require significant architectural choices that are hard to refactor into an existing application. We discussed that some of these patterns are directly supported in newer frameworks, such as Akka HTTP; which has direct support for back pressure, circuit breakers and asynch methods. This makes the decision about when to implement them easier.

Dev meeting – Data intensive applications; and Rust

At our dev meeting this week, Rich talked about ‘Designing Data Intensive Applications‘, that he’d been reading, and Inigo talked about ‘Talking to your TV using Rust‘.

‘Designing Data Intensive Applications’, from O’Reilly, describes various tools and approaches for storing and processing data. It’s split into four sections. Section 1 discusses the basics of databases and how they store content, for SQL databases, NoSQL databases, flat file stores. It covers in-memory stands, b-trees, indexing structures, and the best approaches to use in various situations. Section 2 talks about distributed databases – clustering, transactions, serializing commits at a point in time so shared logs can work, and so on. Section 3 discusses distributed batch processing, using tools like Spark. The first three sections were all very interesting and informative. The fourth section was not as good – it covered the future of data, and was a lot woollier than the previous sections. Rich found the book on the whole good.

To talk to his TV using Rust, Inigo used a Google Home Mini, an RM 3 Mini IR blaster, an Open Source library to interface with the IR blaster, and some Rust on a Raspberry Pi. The Google Home part was setting up a Google Home App via https://developers.google.com/actions/building-your-apps – you configure a set of actions and intents online, and then assign a ‘fulfillment’ to it, which is a web service you make available via HTTPS. This part is fairly straightforwards – it requires filling in various web forms, and it’s easy to publish a test version of an app that’s only available via your own Google account.

To actually use this for a home application, you need to have SSL, so you need a domain name and a certificate. Inigo used Amazon Route 53 for the domain name, and Lets Encrypt for the certificate. There are various other services that will provide a domain name for a dynamic IP address for free, although the original dyndns is no longer free.

THe ‘fulfillment’ part is a web service that Inigo wrote that accepts JSON from Google, and then calls out to a Python library that interacts with the IR blaster. He wrote the first version in Scala, to get something simple working fast, and then rewrote it in Rust. He learned from O’Reilly’s Programming Rust book, and used IDEA’s Rust plugin. The experience was positive, although it was a bit tricky to get some of the memory management working correctly as a first-timer using Rust. The code is on Github at https://github.com/inigo/googlehome-remotecontrol.

How to read more interesting books

In our dev meeting, we talked about interesting development related books we had read, and how to read more interesting books. Various people each talked about a book that they had read recently or recommended.

Chris talked about “The Nature of Software Development“, by Ron Jeffries.

It’s by Ron Jeffries, one of the signers of the Agile Manifesto signer. It’s a good book to give to people who don’t know about Agile, and who might be scared of Agile jargon – it’s entirely jargon free. It explains the fundamentals, and the reasons why software development is better done Agile. It cuts through the cargo culting that is common with Agile implementations.

Tim talked about “What to look for in a code review“, by Trisha Gee.

The author is from Jetbrains, and there is a little undercover marketing for Jetbrains tools, but it’s not excessive. It’s written pragmatically (although not from the Pragmatic Press!). It’s very short, which is a plus, particularly if you have a young child and limited reading time. It is split into chapters based on things to look for in code reviews, like tests, security, performance, good principles. Although it is fairly Java focussed, it is still useful for any language. The tips are fairly obvious to most experienced developers, but there is some good stuff in there.

Pasquale talked about “Clean Coder” by Bob Martin.

This is about “saying no” and “saying yes” – the language used when committing to a task – you should say what you’re going to do and then do it. It recommends and explains Test Driven Development. It recommends coding practice, using katas. It discusses time management and managing your own time; and doing estimation. It is good for intermediate developers.

Loic talked about “Designing the user interface“.

This is a classic of Human-Computer Interaction. It’s primarily aimed at students. It covers a little of everything you need to know about HCI; usability, mice, collaboration, etc. It is useful for all developers to learn a bit about usability. It’s 20 years old, and a bit dated – but still relevant.

Reece talked about “The Design and Evaluation of C++“.

This book describes how the early C++ compiler was created, and how various features were designed, and the compromises made to overcome problems. It goes up to C++ 98. It is good for anyone interested in the history of programming languages and how they are created and evolve.

Simon talked about “My job went to India” (now renamed to “The Passionate Programmer“).

Simon read this about 10 years ago, and still recommends it. It is organized into 6 sections, with 52 tips across that. It is useful to think about how to do more than just what’s needed right now in the short-term. Some particular tips are: “Be a generalist” – don’t get bogged down in particular technologies; “Be the worst” – have better people around you; “Be a mind reader” – think about the software being developed and spike out future requirements that haven’t yet been considered, and think about what a customer might need, but they aren’t saying; “Be where you’re at” – do your job well; “How much are you worth” – was the work I did today worth what I am paid? The book could be read by anyone working in tech, but it’s aimed more at programming than other tech jobs. The ‘outsourcing’ part of the title is just a marketing vehicle.

Stephen talked about the “Mythical Man Month

It was written in 1975, but is still true, although a bit dated. Its core message is that a job that takes two months for two people, does not take one month for four people – because of communication, training, etc. It famously coined “Brooks’ Law” – adding more people to a late project makes it later. That’s still true, although perhaps less so nowadays since people it’s easier to split up tasks and make them independent. Sometimes you think “I could have done that in an hour” – but it actually takes a day: because it takes nine times longer to write good quality deployable shareable code, than it does to write something for yourself only. A modern insight on this is that reuse of code, better tools, and TDD can all help.

We had technology problems talking to Bart, but he passed on his notes on “Algorithms To Live By“.

  • It is a book on algorithms that either technical and non-technical people can get an insight on most important algorithms in computer science. Algorithms are presented in form of brief history and application in daily life. For people with computing background this could be a great supplement to already gained knowledge , i.e in the university. For non-computing people this would explain that computer science is not only about installing/fixing Windows. Some of the chapters most interesting to Bart are:
  • Optimal stopping – you will learn the most efficient methods to employ secretary, find a wife or parking space, or sell the house
  • Explore and exploit – you will learn how medical trials with unknown drugs are conducted, when to stop looking for a good restaurant, how the founder of Amazon explains Regret Optimism
  • Caching: How to store data, clean up the house, and human memory issues
  • Scheduling – importance of time, various methods to resolve problem with tasks dependency, how Pathfinder stop responding and was fixed while on Mars
  • Overfitting: how not to over-learn (self and machines)
  • Randomness – how to calculate PI by dropping a pin, how playing cards helped in building the atomic bomb, how to find gigantic prime numbers
  • Games theory – recursion in humans decisions, the “Prisoners’ dilemma” explained

Rhys also wasn’t present, but passed on his recommendation for “Functional Programming in Scala

This discusses how to solve problems in a functional way, how to think about combining functions in algebraic way. It includes exercises too. Particularly when coming from a Java / procedural background, it’s quite a mind shift.

We also briefly discussed why those particular books:

  • Chris had seen it on PragProg.com, and read about it on the mailing list, and liked Ron Jeffries as an author.
  • Tim has a young child and wanted to read a short book.
  • Pasquale realized that human processes around coding are important, and had already read and liked Clean Code.
  • Loic studied HCI at university, and it’s a heavily recommended book from ACM curriculum.
  • Reece is interested in language design, and this was written by the creator of C++. It was interesting to see thought processes of language designers.
  • Simon was doing a lot of Ruby development at the time, and reading blog posts by the author of the book, and found his attitude of interest – and the book was unexpectedly good.
  • Stephen read Mythical Man Month because it was famous.

 

Dev meeting – Effective Java (again)

Stephen recently read Effective Java, by Josh Bloch, and we discussed things that he picked up from it, particularly approaching Java from a perspective of much more experience in C# than in Java.

  • When you have multiple overloaded constructors, instead use a static factory method on the class. We discussed the advantages and disadvantages of this.
  • We discussed the preference for interfaces rather than concrete classes in Java vs C#. We discussed how this was probably influenced by type inference and type erasure. It may also be related to Java’s history with frameworks like Hibernate and Spring, that initially used lots of interfaces to support proxies before bytecode rewriting became common.
  • Overriding equals and hashcode – it’s common in Java, but less common in C#. In Scala, case classes remove a lot of this complexity. We discussed what identity means for classes and objects, and how you express that in various languages.
  • The difference between Exceptions and RuntimeExceptions is surprising from a C# background. We discussed the merits of checked exceptions – we broadly agreed that in an ideal world, checked exceptions were a good idea, but in practice it depends on library authors being competent and library users being competent. Sadly, this doesn’t happen. Checked exceptions also cause issues for lambda functions. There was some discussion that the way Scala does this is better.
  • The use of external libraries – it’s a lot more common in Java, whereas .NET developers tend to use the CLR content much more frequently. We discussed the issues of transitive dependencies in Java bulking out your build. We also discussed how the ‘small number of large libraries’ assumption is no longer true in .NET Core – there are many different packages within .NET Core.

We discussed Effective Java a while ago in a previous dev meeting, too.

 

Dev meeting – AIs, interplanetary file systems and Nix

We had several lighting talks in the dev meeting this week.

Rich talked about WitAI – a language parsing web service that helps with building chatbots. He’s written a “drinksbot” in it, which we used via Slack before the dev meeting to order drinks. You provide it with a corpus of example sentences, and it learns from that.

Chris spoke on the Interplanetary file system. In typical direct file request, you might retrieve it synchronously from a single location. IPFS allows you to retrieve content from a set of distributed stores – essentially a peer-to-peer, torrent-like filesystem, that can be more robust and potentially faster than a direct file retrieval; because content is stored can be stored in a number of places.

Reece talked about antagonistic attacks on neural networks. Because neural networks (e.g. for image recognition) will typically work by perturb the input image to make the neural network recognize it as something entirely different – even though the change to the human might be imperceptible. For example – researchers recently made a model of a turtle (to human eyes) look like a gun (to neural network recognition) to illustrate this issue. This is a problem with neural networks generally – that the neural network may be using very specific things for its recognition, and it may be hard to identify these.

Stephen talked about teaching (very) young developers – like his daughter – to program. There is a lot of implicit knowledge and setup and background in a typical dev environment, and you want to get to working code as quickly as possible. HTML 5 and JavaScript are good for this. You should optimize for small victories, and concentrate on fast feedback loops.

Tim talked about Vue, the JS framework. You create reusable components that have an HTML component and some connected JavaScript that can control . It’s somewhere in the middle in terms of ease of use – it’s more complicated and powerful than Knockout, but simpler and more usable than React and Angular (and it can be used initially just by including script tags, and then extended to manage things like transpilation). We’re using it in a number of projects at present.

Rodney talked about “Where I’ve been” – specifically about the Nix conference that he recently attended. There is a new Nix tool for handling packaging and building in the Nix system, which has a simpler (command-line) UI. Another talk was about the security developments to the Nix project, such as having specific security experts who can getting security notifications from other projects early on; about automated scanning tools for scanning deployed Nix implementations for security holes. Nix in production was being discussed – e.g. for a company that’s doing cycle dock management in the Netherlands. Using Nix throughout an entire environment, from development through to production, brings many advantages. At Tumblr, Nix is used for testing of live SQL instance replication. Then there was a Hackfest – with spontaneous collaboration, and good work done on Nix cross-compilation.

Tell me more about talking about chatbots

In our dev meeting this week, Alex talked about chatbots. Bots are a massive shift in how we interact with computers – but to some extent they’re still a solution looking for a problem. Bots are conversation driven – and developers typically have never dealt with conversation! There’s a lot of existing support for English language bots that comprehend language reasonably – but there’s far less support for non-English bots. There are libraries and services to help with this – for example, Microsoft Cognitive Services provides endpoints that pick out from the users input what the user is asking, and even what their emotions are. However, these tools are probabilistic – they will say “it’s 90% likely that the user wants this”, rather than “the user definitely wants this”. Bots can get confused – because the bot will typically maintain state so it can refer back to earlier elements of the conversation, it may sometimes lose track of where it is – and needs to be reset. However – this is adding complexity to what is supposedly an intuitive interface; and users may not understand the underlying complexity.

You need to think about the platform that you’re deploying to, in terms of how you develop your bot and what capabilities you expect it to have. Slack bots, bots responding to SMS, Facebook bots – all have different characteristics based on the capabilities of the platform they’re running against. This also shapes what users expect in terms of level of communication – no-one expects lengthy complex sentences from an SMS bot, but they might do from a Slack bot. Discord bots have an expectation that they’re used for gaming, because of the assumptions of the Discord culture. Facebook bots have more in the way of graphics and buttons – so they’re moving to “Monkey Island” style user interfaces, whereas other bots are more similar to an old-style text adventure user interface or a Twine (and Twine is good for developing bots).

For bots, uptime is critical – so the bot should always be available. However, it’s not essential that they respond immediately at machine speeds – in some ways its better to pause a few seconds, because it may feel more natural, as well as helping deal with load. Cloud hosting is useful for bot development to help with this availability and coping with load. Also tracking user responses is important.

Given these complexities – why bots? It offers a high engagement rate, because it’s the same way of communicating as people are used to using with their friends. Bots can be proactive – if a user looks up an event like a concert, then the bot can prompt that the user might want to buy a ticket for it. Bots can be more engaging for users who are unfamiliar with technology. Bots are new and exciting – it’s immature and in flux, and there are no great settled ways of doing things, so it’s good for experimentation. There’s lots of space for exploration – like creating a Twitter bot that offers to sell wine to people based on their tweets of wine bottles. They are a new space – and we should think about ‘how can a bot benefit you’ – like using a bot to collect orders to buy drinks at the bar. Go forth, and make the next Flappy Bird of bots!

Elm, ScalaJS and GHCJS – a “better JavaScript” shootout

At our dev meeting this week, we talked about three different alternatives to JavaScript – Elm, ScalaJS and GHCJS. All three allow you to write code in a modern, statically-typed functional language, and then produce output in JavaScript that can be run in a browser.

Chris had previously talked about Elm a few weeks ago, in “Elm and Punishment”. Elm is a Haskell-inspired language, designed specifically as a web language that compiles to JavaScript, HTML and CSS. Chris has written a sliding blocks puzzle (source) in Elm as a learning exercise.

Inigo was inspired by Chris’s work in Elm to look at ScalaJS. ScalaJS is a plugin for Scala/SBT that allows standard Scala code to be compiled to JavaScript. It covers the vast majority of the Scala standard library (although it excludes a few areas like reflection and reading files from disk). It doesn’t convert JVM bytecode, only Scala sourcecode, so Java code first needs to be converted to Scala, and it’s not possible to immediately use standard Java libraries. Inigo also wrote a sliding blocks puzzle (sourcecode) and a minesweeper clone (source).

Rodney took a different approach – he implemented Flattris – a flat version of Tetris (source). Rather than using Elm, which is inspired by Haskell, he used GHC JS – which actually is Haskell (the Glasgow Haskell Compiler’s JavaScript compiler).

All three of the languages turned out to be very similar in approach and in results. All of them worked very well. The core languages (Elm, Scala and Haskell) are all more powerful languages than JavaScript. The generated JavaScript is bulkier than hand-written JavaScript would be (from 150kb for ScalaJS, up to about 1MB for GHC JS), but this still isn’t particularly huge compared to the size of modern web pages. All three implementations used a pure functional approach (this is standard in Elm and Haskell, and recommended in Scala), and the sourcecode for the two different sliding blocks implementations ended up remarkably similar. ScalaJS and Elm both emit sourcemaps that allow the generated JavaScript to be debugged e.g. by stepping through the Scala source directly in the browser, or via IDEA. All three were enjoyable to program.

We’re not planning to embrace any of the three as a core language we use at 67 Bricks yet, but we were pleasantly surprised by how well they all worked, and we’re certainly considering them for the future.

 

 

Effective code reviews

At our last dev meeting, we discussed code reviews. We do code reviews for all code on all projects, before code is merged to the master branch, and everyone is familiar with doing them, but we can always improve how we do things. So, we had a general discussion on what good code reviews are, and how we can do them best.

We initially discussed what one should look for in a code review. We agreed on some questions a code reviewer should ask themselves:

  • Do you understand the change being made, and more importantly why it’s being made?
  • Is the change itself good? Is it clear, are things well-named? Is it testable; and does it have tests? Does it introduce any non-functional issues such as security or performance problems?
  • One should look at what’s been done, but also the context of the change. Is there code elsewhere that should be reused? Are there similar changes to make elsewhere? Is there now obsolete code that can be removed?
  • Even if the change itself is fine, has the codebase as a whole become less clearer? Has a file or method become too big?

We talked about some of the benefits we have had from good code reviews, such as:

  • Finding out better ways of doing things – such as being able to change a large chunk of procedural code into much more concise and clear functional code, and new ways of doing legacy things
  • Improving consistency throughout the system, using common approaches such as use of optionals, and putting code in the correct classes, clustering similar methods close to each other
  • Getting the benefit of other peoples’ wider experience – e.g. new people have new perspectives and approaches that they have from previous companies, that we can learn from
  • Sometimes it makes sense for the reviewer to make changes and improvements to the code, rather than just handing it straight back to the original developer
  • Identifying areas that are unclear or confusing in the existing system as a whole, and that need refactoring
  • Helping standardize our practices within and across teams, so we can improve generally

As a reviewer, you are aiming to do a good code review, but as a coder, you also have a responsibility to help make the code review useful as well. These include:

  • Asking for a design review before starting the task at all
  • Making useful, atomic commits, with meaningful commit messages, rather than a big bulk commit of everything followed by a few “fixing this” commits
  • Making tests clear and readable, and avoiding repetition in them, just as we aim to make the main code clear and readable
  • Review your code yourself first, before you commit – stepping back from the moment-by-moment coding often helps you spot issues. This incudes the basics – check that the code compiles and relevant tests pass
  • Asking for partial review of long tasks, rather than waiting until the whole task is complete. Frequent review and merge makes the process simpler
  • For a long branch, being ready to fix issues on that branch quickly if a reviewer find any, e.g. by reviewing other peoples code or by doing short tasks, rather than jumping into another long task

Ultimately, it is the reviewer that is doing the review, and we discussed the ways that a reviewer can improve the quality of their reviews:

  • Consider whether you are the right person to review the code – usually it’s good to spread reviews around the team, but sometimes you might want an expert in a particular area to review the code
  • Being able to start the review quickly – so prioritizing code reviewing over starting on new tasks
  • Providing all the feedback from a review at once, rather than in dribs and drabs
  • Pointing out what’s good, as well as what’s wrong in the code – this is good for morale, and also encourages good practices
  • Providing a rationale for feedback – never just “do this”, but “this is a better way because…”
  • There can be multiple issues with a piece of code – sometimes you’ll be initially distracted by minor points, but might need to revisit the review once those are resolved to spot fundamental problems of approach
  • Pointing out alternative approaches that might be useful in future, even though a change isn’t needed in the current review
  • Avoid giving feedback that is impossible to apply – as a reviewer, think about how you might solve a problem yourself, and discuss the issues with the reviewee

And after the review, for all the items raised, each should be discussed and either resolved, or agreed to be not needed now, or a ticket raised for them to be fixed soon.