For some time now, I have been sowing the seeds of better code reviews wherever I may be working. It is perhaps my foremost passion and quite the uphill battle.
It is a challenge to overcome the inertia of "throw the code out there and see what sticks" that seemingly works well for many developers, because it takes less work.
But, aye, that's the fallacy! It might take less up-front work, not thoroughly documenting what is in your proposed set of code changes, but it is surreptitiously adding to technical debt.
If a code author conveys the context they have to reviewers, those reviewers will be able to provide better feedback and do it faster. Better feedback leads to a higher quality end product, which could be in the form of better user experience, less bugs, easier maintainability, and more. Which means much less work being "kicked down the road" in the form of hidden technical debt.
So how do I evangelize doing better code reviews? I do it both overtly (giving talks, suggestions, hints) and--I think most effectively--by example. Here are some of my examples.
Any non-trivial code base is not an island--it is supported by hoardes of libraries. Taking advantage of importable libraries saves often large development efforts but not without a modest cost. Taken as a whole, those libraries provide a constant stream of updates and your code base needs to stay in sync. Explaining how to do that for Chef Automate is half of this PR. The other half is the related, but distinct, DevOps maintenance aspects of maintaining the continuous integration workflows (merges, releases, and so forth). This PR explains how to monitor for anomalies and exceptions in Chef's many such pipelines.
For sometime I have been wanting the ability to both search for and validate semantic language constructions, in the vein of Resharper's "structural search" easily available in the .NET world.
Beyond helping the individual engineer with refactoring and checking code patterns, I knew it would be valuable in enforcing company-wide conventions and patterns. I came across the nascent semgrep project which delivered just what I wanted, and I started wiring it up in our CI system to do code enforcement on things beyond what the compiler or linter could do. Ultimately this will save me and others time in doing code reviews, too, not having to comment upon issues that tend to recur frequently.
I like a clean code base; I will always use the "treat warnings as errors" flag when available. For this project, neither the unit test runner nor the linter had that option, so I added it using a bit of shell sleight-of-hand for linting in this PR (having previously done so for unit tests in PR 3424).
Where the work lends itself to it, I like to itemize the contract I am implementing--i.e., the low-level requirements--materialized as unit tests.
This PR shows all the unit tests for the affected components, highlighting the new ones.
PR 3091 is another such example.
Also, in PR 245 I tried a variation of the traditional red-green-refactor approach, because this was a reasonably well-defined problem. I first itemized all the test names then, after making sure the stakeholders were satisfied, went back and implemented them. (When you make sure you are testing behavior rather than implementation, this is a quite reasonable approach.)
This PR brings awareness to potential performance issues with the Angular digest cycle whenever a function call (explicit or implicit) is used to read a value in an Angular template.
I identified two practical approaches for mitigating the issue.
The declared property approach, illustrated in this PR, replaces a function call with a property, where the function setting that property is now invoked only when things affecting it change, rather than on every digest cycle
The pure pipe approach (an example is in my PR 3876) let's you apply a pure function properly, having no side effects by design allows it to be called only when the associated property changes.
This PR was particularly fun in that while I was ostensibly adding some new functionality to a component, it presented an opportunity to apply not just one principle but three of the five SOLID principles: SRP, OCP, and LSP.
I needed to genericize a component to go from handling a list of a single type of object to handling lists of two similar types of objects.
No, I do not mean with vehemence and vulgarity, but rather literally, with graphics. :-) The right illustrations really do convey information much more easily than lengthy prose.
The "Definition of Done" section of this PR, is one example.
In this PR I made a one-line change--actually just a small fragment of a line. But if I just threw the code up there and said "this fixes it", there is very little chance anyone would just "know" the why and wherefore of what I had done and know that I arrived at the correct solution--so I explained what I did in about 3 or 4 screenfuls (not all text; that includes a couple pictures, too).
This is a combination of the last two items after a fashion: also just a one-line change, but this time explained with illustrations.
One day, with a gap in my workload, I opened up the the database to review the schema and found an inconsistency. The fix was just adding a uniqueness constraint but only those closest to the code would have sufficient context to vet the change. I wanted a wider audience to be able to, so I showed what I changed with "before" and "after" illustrations.
The heart and soul of good PR writing is effective communication. The better the PR author can explain what the problem is and how it was fixed, the better reviewers will understand the entire context and be able to provide more useful feedback. This PR provides one representative example of that. PR 1644 is an even more elaborate "storyboard" style of explication, while PR 3949 shows a visual example of a variety of use cases.
Sometimes one just writes sloppy code. I and my teammates had been adding modal dialogs to our system since its early days, and they just grew like weeds, untended and untamed, having profligate parameters making them unwieldy and certainly non-DRY when they needed to be reused here and there. So in this PR I undertook to break that mold and introduce the "self-contained modal" pattern. Thus, instead of 5 or 6 or 7 inputs and outputs, it needs just one: a signal to open and the modal manages itself. The Single Responsibility Principle also came into play here, letting the modal do things it should be doing rather than the handler invoking it.
In the ChefAutomate code base I adopted the role of dependency maintainer because, well, it needed to be done. I did some of the back-end (there were others helping out there, too), but I did almost all of the front-end. Front-end tends to be more involved, in some ways, as there are versions of node, then versions of framework (we use Angular), then all the other dependencies... lots and lots of them.
And, as I am wont to do, I did not just post the list of changes but in each case tried to provide details of what was updated and what needed to be pinned--and why. In PR 785 and PR 2579, for example, I bumped the node version, showing why the particular version I selected was appropriate. PR 1707 and PR 3082 are typical examples of Angular version bumps.
As we were developing out the front-end, it bothered me that the Save (or Update, etc.) button was always enabled as soon as you touched an input value--even if you put it back the way you found it. Why waste the user's time to let them hit Save if there is nothing to save? Forms should just "know" that if the back-end operation will be a no-op, it should not be allowed in the first place. So in this PR (and the follow-up PR 3141) I gave our forms those "smarts".
It almost goes without saying that user permissions should be part and parcel of any enterprise-grade dashboard. This PR introduces "permission-based routing" for the ChefAutomate top navigation bar. That is, not only will the user see just the tabs they have permission for, but the system will notice which is the first visible tab due to permissions and then land the user there upon login.
Because of the pre-existing modular design, this was almost a "no-code" implementation. I had only to create a new component that subclassed an existing component with one small callback function.
(Unfortunately, I did violate the SRP with respect to the PR itself--I bundled in an assortment of semi-related cleanup changes as well, ballooning this PR to touch 28 files! Sigh.)
Throughout my PRs I frequently use diagrams and illustrations. Just wanted to highlight a couple here: this PR adds some UML sequence diagrams for the authorization flow with projects.
Another one (PR 1020) shows a thoroughly annotated screenshot along with "before" and "after" shots. Often times folks will include a screenshot of what they have fixed (the "after" shot). But you usually get much more helpful context by also showing the "before".
I had been quietly keeping notes on anomalies and other seemingly unexpected TypeScript aberrations and put together this PR to address some of these, as well as created a slide deck to more easily disseminate my findings to the rest of the engineering team.
As developers on any given project, our responsibility is not just to churn out code; it is to deliver a delightful user experience. That must include keeping your eyes open for things that could make the product work better, faster, or smarter for the end user. Often this will get funneled into the product pipeline, triaged, and eventually put on the roadmap. But sometimes it is faster to just fix it rather than write it up. This PR provides one such example, creating a shell helper function that lets a user check all permissions for a given user. I actually posted this first on the official Chef Blog but added it to the code repository for better maintainability.
In a complex code base tooling to aid the 3D's (development, debugging, and diagnostics) is always important. In my casual reading I came across rxjs-spy that supports some diagnostics for the rxjs package; this PR makes it available to developers while taking care not to impact system performance.
