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.


Sorting, and impatience – our dev meeting for 16th June

Chris talked about things that he has learned from the new 3rd edition of the “Scala for the Impatient” book… that we received several months after Amazon had initially told us it would be delivered. He was impatient.

Chris discussed:

  • String interpolation – while he knew about the s”” format for interpolating variables, he learned about the f”” prefix to do printf style interpolation, and that you can define your own string interpolation operators
  • Scala ARM – provides resource management in a similar way to the try/resources construct in Java
  • Renaming classes on import – while you might normally import individual files under different names, for example to deal with the clash between “Files” in Guava, and “Files” in Java, you can also import the whole of a package but block out an import with -> _ to avoid it conflicting.
  • Values classes – you can use the AnyVal class when you’re wrapping a simple type – like wrapping an integer inside a class to make it strongly typed – and the compiler will not create a class
  • Stream creation via functions – you can use the the #:: operator on streams to create a lazily evaluated stream based on a function. This is useful for all the times when you need to, for example, create an infinite sequence of Fibonacci numbers.
  • The @Elidable annotation allows you to strip out methods at compile-time at particular levels – which potentially allows you to remove things like calls to logging in a production environment.

Then, Rodney talked about generic sorting and discrimination – the problem of sorting a list of things, such as a pack of cards. He talked about recent research on how to improve the traditional ways of sorting. Normally you might implement something like “Comparable” or “Equals” on your “Card” class – but that only gives you one way of ordering, whereas you might want to group your cards by number or by suit. Using just an equals method means that finding unique values in a set is hard – it takes O(N^2) comparisons, as you compare every item against every other item.

To improve this, “Discrimination” generalizes partitioning and sorting, and defines a language for doing this.

Rodney demonstrated sorting a pack of cards, using the bucket sort. If you have 52 buckets, then you can put each card in the right place on a table in linear time. If you have 13 buckets and don’t care about suit, you can sort the cards into buckets purely by value, as a linear operation. If you’re sorting an arbitrarily large list of integers, you just need a potentially infinite amount of buckets, stored in a potentially infinite amount of memory, and you can get linear time sorting – no problem…

The problem comes if you don’t have enough memory for all the buckets you want. However, if you have 2^32 numbers to sort, then you can split each into a pair of words, and then sort them all initially by the high word and then by the low word – using multiple bucket sorts where you only need 2^16 buckets not 2^32.

The language of ordering is a DSL that defines various ways of ordering – natural ordering puts things in their natural order; product ordering sorts by one feature such as the suit then another such as the number; sum ordering sorts by type such as whether the card is a joker or not.

You can use this generically by defining a discriminator for your type, such as a card, in terms of the language of ordering, and then you can apply the rules of sorting those terms to split the ordering into an appropriate sequence of buckets for your type, and this can be significantly more efficient than using a single bucket sort.

In conclusion, using a DSL is an effective technique for simplifying and solving problems, and we should use them more widely.

The price of Some(everything), the value of None

This week, Loic talked about how to measure the value of software, and Reece talked about category theory.

Loic based his talk on an academic study of software value from Gio Wiederhold, published in ACM. He talked about how the value of software changes over time. Typically, revenue from a software product will rise until it reaches a peak, but the price of software is usually not expected to increase significantly over time. Maintenance costs, particular for keeping old versions available, can rise over time. In practice, 5% of code is deleted per year, and a new version of a product consists of less than 30% more code – with most effort spent on keeping everything running smoothly. Loic discussed rational design decisions about where to focus the effort in software development, and making sure there is sufficient investment in maintenance.

Everyone else was generally interested in the topic, but found the research it was based on somewhat dated. It’s very much based on the “large software company producing a big product” model, like sales of MS Office. However, the current market is much more about web delivery of systems that can be updated very frequently in small ways, and about apps, and about Open Source. We discussed how these sales models impacted software value.

Then, Reece talked about category theory, and its relevance to software development – in a talk called “Monads are not (always) Optional”. He talked about the value of using standard mathematical terminology for the concepts of programming, and defined “category”, “object”, and “morphism”. Then we discussed composition of morphisms, and how classes of numbers (e.g. integers, rational numbers, natural numbers, complex numbers) could be transformed into each other via various operations, but other operations (such as addition and multiplication) changed the values of numbers but kept them within the same category. He discussed the relevance to programming, and how category theory and its concepts provided a more unified mechanism for structuring solutions than other approaches such as design patterns.

We then discussed monads, and monads in C# via Linq and in Scala via sequences and optional. We discussed that the “Advanced Scala with Cats” had recently been made available for free, and several people planned to read it. No-one was clear of the difference between Scalaz and Cats – both seem to cover similar ground (some subsequent research seems to show that they’re similar, and Cats might generally be preferred).

EMACS org mode, tea and sprints – dev meeting for 5th May

Rodney discussed EMACS org mode – after some discussion of the relative merits of EMACS vs Vim vs inferior other editors. It essentially allows you to edit bullet points, but it can be used for calendars, todo lists, estimates, and so on. Via a keypress, you can add templates for a todo item, including schedules and other metadata, and include it in your todo list. This metadata is used in the display – e.g. you can display an agenda for the day, which will include all of the tasks scheduled for today. You can mark those tasks as done once complete. Tasks can also be split into a hierarchy of tasks, and the top task can display a percentage of the remaining tasks under it. It also has time tracking – once you start a task, you can start time tracking, and then it will be logged against that task.

Advantages of it are – it’s simple, it’s fast, it’s free, and it’s text based so other tools will work with it.

To achieve similar results, Chris uses todo.txt – which is essentially a text file in Dropbox, but it’s supplemented by command-line tools, and a phone app. Because it’s backed by a text file, then you can always access your data anywhere without any other tools. Reece uses a text file in Vim. Inigo used to use a text file in Vim, but now uses Trello.

We then discussed tea making. There was no consensus reached.

Stephen then talked about sprint planning. In various of our projects, we are doing sprint planning in different ways. The default Agile approach is to start with a demo of the work done in the previous sprint, then a retrospective of the previous sprint, then the project owner provides the things that they would like to be done in the sprint, and the project team discusses which ones can be done in the sprint. In one of our projects, we have a looser sprint planning, that doesn’t involve prior discussion of the statistics from previous sprints, and not much backlog grooming. We discussed the merits of estimation – even though the results of the estimation are not always useful, the process of doing estimation and giving developers a good idea of how long tasks are expected to take is useful. We also discussed prioritization, and how new tasks should be dealt with – whether they should be completely rejected until the end of the sprint, or whether they should be prioritized against exising tasks. We agreed that we would look at more explicit prioritization in some projects. We also agreed to combine some brief explicit estimation of tasks in the sprint planning meetings, with announcing on Slack a brief description of the task and how we would approach it and how long it would take.‰

Elm, and Punishment

Chris talked about Elm, which he subtitled “JavaScript for people who like Haskell not JavaScript”.

However, he immediately explained that Elm is not like Haskell, and isn’t intended to be compared to Haskell, and that it is aimed at JavaScript developers not Haskell developers, and avoids the community avoids using Haskell terminology. This is intended to help increase the uptake of the language.

However, it is a pure functional language, and does look quite Haskell-ish: for example, it supports pattern matching, immutability, and so on. It is event-driven.

One thing it concentrates on is having good error messages. It will even do things like identify references to variables that don’t exist, and suggest what you might have intended. It’s also helpful that there’s an Elm plugin for IntelliJ IDEA.

Like most modern JavaScript frameworks, it uses a virtual DOM, to make it faster.

There is a time-travelling debugger, which is a very cool feature, but Chris didn’t use it in practice.

It has automatic semantic versioning, so it updates the major version automatically when existing signatures change. However, the Elm language itself has changed significantly in minor versions, so you can’t depend on code for older Elm versions being compatible. This was frustrating for working in the language – finding older libraries that hadn’t been updated, and older examples that were out of date.

You can incrementally rewrite an existing JavaScript codebase to Elm, on a file-by-file basis. Chris wasn’t sure how compatible Elm output is with older browsers.

There are currently no books (although there will be later this year). There is a “try elm” site that is good.

It’s primarily aimed at interactive UIs, and games.

Chris’s sample Elm code is at http://nespera.github.io/elm-slide/. However, he hasn’t actually completed the game, because it’s too hard.

Then, Loic talked about Punishment Driven Development. He talked about reasons why companies punish people, and the effects of that punishment. He described the importance of understanding why people are behaving as they do, and that sometimes you may need to change your own behaviour to work with them in order to achieve your objectives. Then, he talked about the axioms of Punishment Driven Development, and contrasted them with People Driven Development.

Dev meeting 10th March – Laravel, Slack bots, and Android automation

At the dev meeting this week, we talked about the PHP framework Laravel, about Slack integrations, and about automating Android phones with Tasker.

Bart gave a presentation on Laravel. It’s an MVC framework for PHP, that bills itself as “The PHP Framework for Web Artisans”. It makes extensive use of “artisan”, a command-line code generation tool for generating models, view templates, etc. It supports various features familiar from MVC frameworks in other languages, such as database migrations, programmatic definition of routes, and ORM.

We’re not using Laravel, and we’re not likely to be using it for any client projects, but we are using PHP for some infrastructure things like our website.

Then we discussed Slack integrations that we are finding useful, and Slack integrations that we wish we did have. We have a webhook-based Gitlab integration, which sends a message whenever code is pushed to a Gitlab repo – this is useful for low-traffic repositories like document repos, but less useful for code repos. We have a Jenkins CI integration set up as part of our standard Jenkins script, which sends messages on success/fail of the build. This is more convenient than the email notifications we also have set up. For the client projects that use it, we have a Bamboo integration that works similarly. We have also tried the Trello bot, but didn’t find it useful.

Other Slack integrations that we thought might be useful were:

  • A wiki search bot, for searching our internal wiki
  • A JIRA slack bot for creating and tracking JIRA tasks
  • A cake bot, for working out whose responsibility it is to provide cake or cheese for our Cheese/cake Mondays
  • An uptimerobot bot, so downtime notifications can be sent to Slack as well as email
  • A password bot, for reminding us of passwords for our infrastructure

We also discussed the usage of Hubot at Github, which they use to automate all of their infrastructure. There is an Open Source version of Hubot available.

Joe talked about Tasker, an automation app for Android. Joe originally installed this after receiving a parking ticket in the Park and Ride. His use case was to detect when his car had stopped moving, so he could then automatically launch the Park and Ride app to pay for parking.

Tasker allows you to write small scripts for managing Android, without having to write a full Android app. For example, “I can see this cell tower” therefore “trigger these actions”. “I am plugged into power, and can see the car’s bluetooth” so “launch SatNav and turn off wifi” – and then when moving out of that state, “check the GPS and launch the Park and Ride app if at the car park”. “On receiving a text message with a particular text”, “turn volume up to maximum and play music”, in case the phone is lost. We also briefly discussed Llama, which is a similar Android automation app, although less sophisticated.

‘Jumble sale’ developer meeting

We had another “jumble sale” developer meeting this week, in which a number of people talked briefly about topics that interested them.

Rhys, Reece and Richard H talked about integrating with third-party authentication and authorization systems. For some of our projects, we have specified a simple API that the third-party authentication provider needs to support. This has been an effective approach, and we’ve found it particularly useful that we wrote a complete test suite to begin with against a dummy provider – that meant that we could very easily tell if the third-party had followed the specification, and potentially they could have run the tests themselves (although they didn’t). For another project, we have used SAML to support single-sign-on to integrate with existing sign-on systems. We’ve used pac4j from Scala to do this integration, and combined it with MarkLogic authentication to allow login to our systems.

Rodney talked about TypeScript, and how it improves JavaScript by adding better typing. It’s a strict superset of JavaScript, so all valid JavaScript code is valid TypeScript, and it compiles to JavaScript. However, we haven’t yet looked into how we can transpile TypeScript using the frameworks we typically work with.

Inigo talked about his investigation of why an AWS-hosted MarkLogic cluster was somehow transferring many terabytes of data per month. The possible causes of this were:

  • the communication between end users and Play
  • … between Play and MarkLogic
  • … between the hosts in the ML cluster,
  • … and from the servers to their S3 backups.

He used stats from the AWS load balancer combined with Firefox’s network tab to analyze page weight to rule out the communication with end users. By enabling logging on the load balancer used between Play and MarkLogic, he was able to rule out the second cause. Storing data from AWS in S3 is free as long as it’s all in the same region – which it was. Running iftop on the cluster nodes showed which sockets the data was flowing between and when – which pinned it down to the MarkLogic intra-cluster communication, in regular bursts. Looking at what the system was doing, there was a regular background task running that queried all of the documents. This took about 30s, but we hadn’t previously been concerned about the inefficiency since it didn’t affect users. However, because it wasn’t using indices, it needed to do a lot of data transfer between the cluster nodes to check the content – which led to our data transfer volume. Rewriting the query to use indices made it much faster, but also prevented it from having to do any data transfer, significantly dropping our AWS data transfer bill. The lesson of this is that when running a cluster on AWS, we care about different sorts of inefficiency than we would do running it on local machines.

Richard B talked about Effective Scala Futures – how to use Scala futures without using global variables that make testing hard, and how to bulkhead separate threadpools to manage different tasks. His slides are here: Effective Scala Futures dev talk

NixOS and MarkLogic 9

This week, Rodney talked about Nix – the NixOS Linux distro and NixOps tooling, and Simon talked about MarkLogic 9 Early Access that we have been working with for a few months.

Nix Expression Language is a declarative functional language, with aspects that are similar to Haskell, Scheme, and YAML. It’s a domain-specific language that can perform tasks similar to Ansible for configuration, and similar to make for building packages and projects while keeping track of dependencies and when they change.

NixOS is a Linux distribution that uses Nix EL to describe the configuration of a machine, built on the Nix package manager. It has atomic upgrades and rollbacks of packages.

NixOps again uses Nix EL, but at a higher level to describe how to provision a set of machines in a cloud environment like AWS. Unlike other tools like Puppet or Ansible, NixOps describes the complete desired state of the machine – so if you manually install a package that NixOps isn’t expecting, it will uninstall it again.

A Nix file can be NixOps will provision machines based on a description in Nix EL. However, Nix builds are run in an isolated envi

We’re not currently using Nix for deployment (we generally use Ansible), but Rodney is using it on his desktop, and thinks it is a good set of tools.

We have been using MarkLogic 9 Early Access for a few months. Simon talked about the new features around data integration, security, and manageability, and the improvements to the Query Console.

The ‘template driven extraction’ feature allows us to define templates that convert XML or CSV files to row data or triples on insert. It is best on ‘row like’ data, such as XML with a set of repeated components, and allows you to define anchor elements via XPath, and then a set of relative XPaths that define particular data fields to be extracted from that content. You can then query this data via a select, or in conjunction with other data via the Optic API.

The ‘Optic API’ allows joins and aggregates over documents, row data and triples – for example, combining a SPARQL query of graph data with a SQL query of relational data. This is valuable because different data is often represented and queried in a form that is particular to the style of data – but needs to be combined with other data to be useful. For example, purchase data might be best represented as a relational database style set of rows, but then processed by aggregating it and combining it with metadata on what is being purchased that is best represented as nodes in a graph database.

The ‘data movement SDK’ extends capabilities that were previously available via tools such as mlcp and corb, and exposes them via a Java API. This allows bulk ingestion of documents, and their transformation.

The ‘entity services’ is a modelling framework for data stored in MarkLogic. It makes it easier to harmonize data from different sources into a consistent model, using the ‘envelope pattern’ to wrap and preserve the original data. It’s not doing anything that couldn’t have been done manually in previous versions of MarkLogic, but simplifies the code for this.

The most interesting new security features allows permissions for XML content to be assigned at the element level, rather than at the document level. So, hiding elements within a document based on the role of the user, or making elements read-only by role. This is interesting for some of our applications that make particular parts of documents available based on the user.

One of the small but important improvements to the query console is displaying different results for each query tab. This makes working with it a lot better. It also displays type-ahead autocomplete suggestions for function names – this is also useful, although initially disconcerting.

In all, MarkLogic 9 offers a number of interesting new features. We are currently using the Early Access version of it. We’ve encountered and raised a few issues with it, which have been fixed, and we’re looking forward to it being launched.


Fantastic Lambdas and How to Deploy Them

As mentioned previously, we are using terraform to spin up resources in AWS in an automated and repeatable fashion. Mostly it just works, but now and again things get tricky. We hit such a situation when automating the deployment of AWS Lambdas. We were using terraform to create AWS resources and then continuously deploying with ansible. So if the lambda source code changed, ansible would deploy the new version, while privileges and other plumbing were taken care of by terraform. It all seemed to work well, but trouble was lurking.

The problem was that when setting up the initial version of the lambda in terraform we were effectively creating it empty and leaving it up to ansible to deploy the actual code. This is fine up to the point you need to run your terraform script once again. Terraform defines its resources declaratively, so if additional resources or changes are needed you simply run the script again and everything is brought up to date. But when it came to the lambda it would say to itself “This lambda is declared as being empty, but it isn’t. I’ll fix that!”. So running the terraform script would wipe the source code. Oops.

We got around this by storing the lambda source code in s3 and always deploying from there. The terraform script ensures that the bucket and source zip exists and creates the lambda using that source:

resource "aws_s3_bucket" "source_bucket" {
  bucket = "my-bucket-for-source"

resource "aws_s3_bucket_object" "lambda_source" {
  bucket = "${aws_s3_bucket.source_bucket.bucket}"
  key = "source.zip"
  source = "initial_empty_lambda.zip"

resource "aws_lambda_function" "my_lambda" {
  function_name = "my_lambda_function"
  s3_bucket = "${aws_s3_bucket.source_bucket.bucket}"
  s3_key = "source.zip"
  runtime = "nodejs4.3"
  environment {
    variables = {
      foo = "bar"
      bez = "baz"

Note that creating the zip in the way specified (without using the etag attribute) means that terraform only checks if the file exists in s3. Importantly it won’t overwrite an updated zip with the empty one later on…

Meanwhile, the ansible playbook uploads the latest zip to the s3 bucket and updates the lambda source using that. So now running terraform will not break the lambda, sanity restored.