Pragmatic Code Evaluation

One thing we do rather frequently, especially when taking over a project from another team or when we engage in a rescue effort, is evaluate code that we didn't write. Our process for doing this isn't magic, it boils down to some tools that we use to gain a foothold and then a lot of manual investigation. This guidebook covers some of the techniques we use and serves as a general outline for the information we present in reports to customers who need a code review.

General Overview

It all starts with just opening up the project in an editor and poking around, gathering some basic understanding of the code.

Number of Source Files

In a lot of cases the shear size of a code base is telling, especially when compared to the relative complexity of the functionality. If an application is functionally simple but has a big code base (or vice versa) there's probably something worth investigating.

Lines of Code Per File

A high number of lines in a source file can be an indication that a class or module is doing too much, it's not a universal rule but can point to violations of the Single Responsibility Principle. Running some tools over a code base to give a line count per file helps us identify where the big blocks of functionality or complexity might live.

Length of Methods

Similar to a class or module doing too much, methods can do too much as well. If the previous developers used just a few methods to do a lot of work it's an indication that the system will be difficult to change. If however the system is composed of many small methods, well named and organized then change will be accomplished more easily.

Conventions and Idioms

Every platform (Rails, Spring, Ember.js) and every language (Ruby, Python, JavaScript) has a set of common conventions and idioms. These are things that are not enforced by the environment but are accepted in a given programming community. Following these conventions makes it easier for developers to move between projects and quickly ramp up. If code is not written in this conventional and idiomatic style it's a red flag that it will take new members of a team longer to accomplish tasks.

Static Analysis

Static analysis refers to analyzing source code without running it, just looking at the syntax and text of the source to draw conclusions. There are numerous tools that can help us do this faster and more efficiently than the human eye.

Repetition

Repetition is generally bad in code, and by repetition we mean the representation of a single concept in more than one place. We call this kind of repetition a violation of DRY (Don't Repeat Yourself). Keeping a project "dry", as we say, is an indication that there is only one place in the code that models a concept and therefore changes to that concept can be made more universally. If a concept is repeated in many places, when you need to change that concept, you have to do it multiple times.

Security

Security is a huge topic, and reviews of application security can be entire projects in and of themselves. But, as part of a code evaluation we do like to throw some basic tools at the code to see if there are glaring security issues such as SQL Injection, Cross-site Scripting, or Request Forgery. These common security problems, if present, probably indicate other issues as well.

Code Quality

Code quality is subjective, but in recent years some tools have emerged that can provide some basic grading to the quality of a code base. We like to use these if they're available for a given language just to have a more blanket look at the code…forest rather than trees.

Cyclomatic Complexity

Cyclomatic Complexity is a measure of the number of branching paths through a method or series of methods in code. Higher numbers mean that there are more scenarios modeled in the code, more conditional logic, more case statements, etc. More branching means there is more to test and more for a developer to understand before modifying code.

Dead Code

As a system evolves it's inevitable that some parts become dead because they are longer needed. It's important to remove dead code because it's not immediately apparent to future developers that this code really is unnecessary. The amount of dead code in a system is an indicator of how diligent the team has historically been at keep a clean code base.

Churn

An analysis on code churn can be very revealing. Churn is a measure of which parts of an application change frequently and is done by using some tools against the source control history. If certain files are changed very frequently it may point to hot spots in the code that need refactoring. Why are these areas so volatile, are they doing too much, too little, or the wrong thing entirely?

Dependency Analysis

Dependencies are things that the code base in question relies on to function. They could be open source libraries, third-party systems, or servers and operating system resources. The more dependencies a system has the more moving pieces need to be coordinated, updated, and curated.

Unused Libraries

Libraries (Ruby gems, Node packages, etc) that are defined in a systems dependencies but are not actually used by the system should be pruned.

Outdated Libraries

Libraries that are out of date (i.e. newer versions exist) should be updated where possible, especially if they are core to the system. Newer versions are likely to be improved over previous versions in terms of performance, security, and functionality…not always, but usually. Severely out of date dependencies force major upgrades, it's much less expensive to upgrade incrementally.

Systems and Servers

Dependencies aren't just in code, sometimes they're in the systems and servers that an application uses. For example, most applications have a primary database (Postgres, SQL Server, Oracle) and will use some secondary system for ephemeral data (Redis, Memcached). Some systems have much more complex requirements and need connections to GIS servers, or printing hardware, or reporting systems and data warehouses.

Third-party Integrations

Integrations with external, third-party systems is one of the key factors in the complexity of a system. Many errors and issues tend to collect around the areas of a code base where two systems collide. For example, systems that pull information from may outside APIs tend to have a lot of layers and import/normalization code. As external systems change, the code base breaks and bugs form. How these bugs are mitigated or dealt with is telling.

Automated Testing

Automated testing should be the life-blood of any development process, it's a foundational aspect to building quality software and we don't write code without writing tests…period. When we look at external source code we look very closely at the tests, or lack thereof.

Test Coverage

Coverage measures the percentage of the production code that is "covered" by tests, how much of the code is exercised by the automated test suite. This number doesn't tell us whether the right things are tested, or if they're tested well, but it does give us some indication of how much is tested. Low coverage (say, below 60-70%) could be an indication of an inadequate test suite.

Isolation / Test Types

There are arguably three types of tests: unit, integration, and functional. Unit tests are very isolated, integration tests compose several pieces of code, and functional tests drive the system from the outside (usually the browser). Applications that don't take advantage of all three in some way are usually lacking. We like to see the lions share of unit tests, a fair amount of integration tests, and a smaller number of high-value functional tests.

Test Quality

Just like quality needs to remain high for production code, it must remain high for tests as well. The test suite is the first line of defense for validating the system and must be constantly updated and pruned as the requirements and production code change. We love reading through tests to see how much effort the previous developers put into this valuable part of the software process.

Tooling and Automation

As developers we like to automate the repetitive stuff, no sense wasting human time on something a computer can do better. Looking at what and how other teams have automated is evidence of of how easy it will be to deploy and manage a production system.

Continuous Integration

When you have many developers working on the same set of code they will all have a slightly different version of the source code on their local development machines. The continuous integration server is responsible for running the automated test suite against the combined code. It may also be configured to automatically deploy passing builds to a staging or test server.

Deployment Environments

Having multiple environments (development, test, staging, production) allows versions of the code at various stages of the development process to be viewed by users. Successful builds from CI may go to test and then are manually tested before being promoted to staging for user acceptance and then on to production. It's poor practice to just push code directly to production without an intermediary step…unless you have an extremely refined process.

Provisioning and Automation

In larger scale systems, where many servers are required to run in production, it's critical to automate the provisioning (creation and configuration) of those servers. Having to hand create and configure servers would take far too long and be very error prone. Using tools like Chef to automate provisioning or at least have a repeatable/scripted process is a good sign.

Project Management

Another indicator of the state of a project is how well and fastidiously it has been managed.

Defect/Issue Queue

For our projects here at DevMynd we like to keep track of all the defects and issues that are reported by customers. We hope there aren't many, but when they do show up they're very informative as to where issues tend to crop up in our code. We log them, record the steps to reproduce, build new automated tests to prove they exist, and then fix them with those tests in place to protect against regression. For existing projects, reviewing the defect queue can be enlightening as to where there are hot spots within the code base.

Product Management Tools

Another great way to get context on a project is to take a look at the product/project management tools (Trello, Pivotal Tracker, Basecamp) and get a sense for how requirements have been constructed. If a product is built on shaky requirements it can shed light on some of the things we may have seen in the code.

Source Control History

Lastly, digging through the commit history of the source control system (Git, SVN) is always helpful. Seeing how well factored individual commits are, do they have descriptive names, are they small and atomic or were developers committing huge change sets. All of these demonstrate the level of care and quality that has gone into a project.

Developer Productivity

Many of the things we've talked about so far in this guidebook affect developer productivity. In clean code bases that have been built with good techniques and from a solid product backlog, we see high productivity. When we see productivity slipping it tends to just be a symptom of other issues. There are a few latent indicators of productivity that we like to look for.

Efficiency in Development Environment Setup

It's not every day that a team takes on a new member and has to ramp them up on the development environment. But, when it happens, it can bring up all kinds of problems that aren't apparent day-to-day. How long does it take to get all the dependencies installed? Can a developer go from downloading the code to running the tests in less than 30 minutes? Is the setup process documented? Is the environment so highly customized that system level changes are required to run? Is there a virtual machine that packages up the dev environment?

Performance of Application in Development Mode

Performance review, like security, is really a whole separate effort. But, we do like to spin up the applications in a development environment if possible and get a sense for how quickly in performs locally.

Quality of Development/Test Data

Test data is another area that teams tend to ignore until it becomes painful, even though having good test data is key to developer productivity. We like to look at how a team creates test data. Do they just pull down a backup from production? Is there a script that can create realistic data on the fly?

Documentation

No developer likes to write documentation, but no developer likes to hunt around for hours trying to solve a problem when a simple WIKI page could have helped. We don't like to see reams of documentation, but some form a documentation is a good sign. This is especially true if there are complex configurations that need to be addressed, or specific processes required to complete tasks, or know bugs/issues in 3rd-party dependencies. We also like to assess how up-to-date any documentation may be.

Ability to Run Offline

Finally, can the application and test suite be run locally without any external systems being present? If a connection to the internet is required to connect to outside systems for running the test suite it's a bad thing and can slow down development time considerably.