One of the most common tasks that lands on my desk as a consultant is to review someone else’s code. Sometimes this comes in the form of a pull request (which is most often a small, feature or ticket based change) or an entire code base. When looking at a code base, the scope is massively different than an individual PR, given that we aren’t making a “small change.”
Looking at an entire codebase is something of an acquired skill just because your scope is… well… everything (even some things that you may not personally be an expert at).
Why is This Challenging?
One of the things you learn real quick as an architect is that despite best practices, documentation, and group-think, there’s a really high probability that you do something a little differently than other people. Or a lot differently. And the scary part is just because it’s “really different” doesn’t mean it’s “really wrong.”
One of the first challenges you have to overcome when you’re reviewing someone else’s work is understanding those differences. Is it a difference of preference? Or is it actually a big deal.
For instance, let’s imagine looking at a codebase that doesn’t have any automated code validation vs. a project that does different code validation than you’re accustomed to.
If the project isn’t doing any code validation at all, this is a violation of best practices and should be prioritized. I’ve written about this in the past, but the TLDR is that you absolutely should be making sure that code contributed to a project meets a certain standard, and you should prioritize this if it’s not present.
If the project is doing code validation, but perhaps it’s using a different tool (e.g. GrumPHP vs. PHPCS), this isn’t a hill I’m going to die on. If the standards being applied aren’t right (for instance, generic PHPCS rules vs. Drupal community rules from the coder project) I would certainly flag this, but it’s not going to be top of my list.
So, as you sort through the codebase, you’ll be faced with this scenario over and over again.
Are they doing a thing?
If so, are they doing it right?
If so, is it working?
At some point there’s also your personal preference to take into consideration: Are they doing it the way I would do it? Unfortunately, the way you want it to be may not always be the way it ends up being. Setting aside your ego / personal opinion to acknowledge that “yes this is different. but it works” is one of the hardest lessons to learn in this field.
The other quite significant challenge I want to acknowledge is that these reviews and the ensuing discussions can become immensely personal for people, very quickly. It’s easy as an “expert” in a field to look at the work done by an individual, team, organization, etc. and say “well this is just wrong.” What that can come off as is “well you don’t know what you’re doing.” And maybe they don’t know what they’re doing! But the take away here is that ripping apart an entire project and bringing down the hammer of best practices may have casualties. Which may impact your working relationship. Which may impact your ability to actually move forward with fixes. So be mindful of what you’re saying and how. You may well be 100% right about what you’re seeing and why you want it to change, but if everyone is pissed and hurt at your message, it may not matter that you’re “right.”
Why Do This Review?
There’s a lot of reasons you might be reviewing someone else’s code. Some of the most common ones for me:
We’ve been hired to do an audit (look for problems, tell them how to fix)
We’ve been hired to troubleshoot a problem (they know something is wrong with something in particular, but they aren’t sure what/why or how to fix it)
We’ve been hired to do development in an existing codebase (meaning someone else has defined what’s there vs. how my team would usually )
These three scenarios are a little different in practice.
The first two are very focused on the nitty gritty of how the application was developed / how it’s running. In these circumstances I am focused on finding a problem (or lots of problems) and trying to fix them. I will likely make recommendations on best practices (hey, you should be doing automated testing, you should be doing code validation, etc.) but these are just another piece of the audit! These go deep into custom code, application of that code, configuration, etc. I am actively kicking over rocks and looking for problems.
The third scenario has a very different lens, because in this model my team is actually going to be doing work in the codebase. Here, I actually care a little less about the other work that was done (specific modules, libraries, etc.) if they aren’t in my scope. So I may not be flipping over every rock to find specific issues. In this case I care a lot more about how folks work with the project, what the development / devops processes are, etc. I’m going to fight a lot harder for the best practices at the workflow level because they’re important to the way my team works.
So, the lens that you bring to a codebase based on the type of review you’re doing absolutely should be taken into consideration.
Things To Review
Obviously your miles will vary when reviewing projects depending on the technology and underlying framework(s) at play. But a few things that I think are general rules of thumb:
Is there a local environment? If so, is it configured properly and does it align with the CI/CD and cloud environments?
Is there a development workflow? If so, does it align with the CI/CD and deployment (DevOps) workflows?
Are the right things there and if so, are they setup properly? For instance, on a Drupal project is there a configuration management strategy? Is composer setup properly? Does the code and configuration validate and work scans?
Are there front end technologies like NPM there? Do they work?
Can you actually install the software (in the local) and run it / log into it and have it work?
Obviously this list is highly can be highly subjective based on the technology at hand. For a recent Drupal project, here’s the list I looked at:
q: is there a configuration management strategy
a: yes, config splitq: is there a configuration directory and is it in the right place and named correctly
a: yes there is a directory, but the sub directories weren’t named as I expectedq: are there standardized processes in place for managing config
a: no, the local, deployment, and ci/cd processes do not alignq: is there a local environment that makes sense?
a: yes, but it could be simplifiedq: is there CI/CD in place and does it do “all the things we want”?
a: yes there is CI/CD but it doesn’t do much (just installs front end dependencies)q: does the project use standard deployment mechanisms?
a: sort of, there are custom versions of the standard scripts vs. using the standard scriptsq: is code quality an issue and/or enforced?
a: yes it’s an issue (roughly 15k lines of output from PHPCS) and it’s not enforced. the majority (all but 500 lines) can be fixed with PHPCS automaticallyq: are there security vulnerabilities?
a: yes, there are multiple unsupported releases of dependencies on both the back and front endq: does the site install and run?
a: using normal methodologies, no. there were configuration validation issues that need to be resolved.q: do php dependencies resolve?
a: yes, although not updated to support composer 2q: do front end / npm dependencies resolve?
a: yes, although they won’t build without adding a rebuild sass
This (and other) information I gleaned from reviewing the codebase over the course of about an hour tells me that a lot of the basics are in place, but aren’t standardized or configured in the way we would usually configure them.
The next step is to communicate this material to the customer! Explain why these things are concerning, explain what we recommend to do about it, show examples of how we could do something about it, etc. The goal here is to standardize the workflow and resolve any issues that could in the long term increase the likelihood of the site having issues.
In Conclusion
I talk a lot on the blog about having established workflows and standardized approaches to development. This scenario of looking at someone else’s project is a prime example of why it’s important you do. You want to make sure that you know what you need, how to do it, why it works, what you should NOT do (and why), etc. And it’s important that you can articulate why it’s important. “Well, I just like it that way” isn’t super valid. “It should be that way because we want to make sure that when CI/CD runs it simulates the deployment and gives us a high degree of certainty that the build pass telegraphs a successful deployment.” One of these statements is compelling and based in fact.
Even with a basis in fact, it’s important to acknowledge that you are (potentially) throwing dirt on someone else’s hard work. Sure, it may be work that doesn’t follow best practices or align with what you usually do, but it’s still something that someone invested a great deal of blood, sweat, and tears into! Proceed with caution and be respectful. And hopefully you can all get to a good place.
Photo by Bruce Mars from StockSnap
Related Content
I recently made a stupid mistake cleaning up Git history on a project. Let’s talk about it, and how to avoid it.