Caseflow code quality talk

caseflow_code_quality.md

Wisdom Wednesday on Code Quality, March 6, 2019

Moncef's Background:

• Developed interest and focus on code quality and refactoring over past 4 years

• Helped start 18F Development Guide

• Spent lots of time reading and learning

Recommended courses, books, tutorials, newsletters

• Practical Object-Oriented Design, An Agile Primer Using Ruby
• Ruby Science
• Refactoring Rails
• Ruby Weekly
• Ruby Tapas
• Destroy All Software
• Master the Object-Oriented Mindset in Ruby and Rails
• Upcase

Problems

• Onboarding is slow and different for each person
• Different teams use different design patterns
• High churn in files with high complexity

Recommended articles and talks on churn, complexity, code quality

• https://www.sandimetz.com/blog/2017/9/13/breaking-up-the-behemoth

Complicated code that rarely changes appears in the upper left quadrant of this graph. We abhor complication, but if the code never changes, it's not costing us money. Pretend the code is a cabinet overstuffed with teetering Tupperware: just quietly press the door closed and sneak away. Ignore code in this quadrant until it starts to churn.

Simple code that changes a lot falls into the lower right quadrant. If code is simple enough (think configuration file) it will be cheap to change. Change this code as often as necessary, as long as it remains simple.

The lower left quadrant contains things that aren't very complicated and that don't change much, so this code is already cost-effective and can also be ignored.

The top right quadrant reflects complicated code that changes often. By definition, code like this will be hard to understand and difficult to change. We'd prefer this quadrant to be empty, so code that slithers into it should be refactored right back out of it.

If important classes in your domain change often, get bigger every time they change, and are accumulating conditionals, stop adding to them right now. Use every new request as an opportunity to do a bit of design. Implement the change using small, well-designed classes that collaborate with the existing object.

A 5,000 line class exerts a gravitational pull that makes it hard to imagine creating a set of 10 line helper classes to meet a new requirement. Make new classes anyway. The way to get the outliers back on the green line where they belong is to resist putting more code in objects that are already too large. Make small objects, and over time, the big ones will disappear.

• https://www.sandimetz.com/blog/2017/6/1/the-half-life-of-code

Code is read many more times than it is written. Writing code costs something, but over time the cost of reading is often higher. Anyone who ever looks at a piece of code has to invest brain-power into figuring out what it does. [...] It follows that you can reduce overall costs by optimizing code for reading rather than writing.

Here's where the concept of half-life matters. You can lower your costs by reducing the half-life of your least stable code.

The parts of your applications that change the most also cost the most. These dynamic parts are often fundamental to your business. They are the result of lengthy, evolutionary development, and suffer from never ending churn. You depend on them the most, yet they are hard to understand, and only getting worse.

Vast other swaths of your application are likely equally unpleasant, but relatively stable. This stability essentially makes them free. It's not ugly code that costs money--it's change. Ugly code just exacerbates costs.

Time is in appallingly short supply. The good news, however, is that you're not obligated to fix things that aren't costing you money. The most efficient way to improve long-lived applications is to focus on the code that churns--this is where your efforts most pay off. For maximum effect, commit to crafting solutions that are both frugal and easily replaceable.

• Sarah Mei - Livable Code

• Twitter thread by Sarah Mei

• Katrina Owen - Here Be Dragons

• Sandi Metz - Polly Want A Message

• 18F - Choose Design Over Architecture

• Ben Orenstein - Refactoring from Good to Great

Script to measure churn in any Git repo

https://github.com/garybernhardt/dotfiles/blob/master/bin/git-churn

These files have changed more than 100 times in the Caseflow repo:

 102 client/app/queue/QueueApp.jsx
 106 spec/feature/intake/appeal_spec.rb
 107 app/models/form8.rb
 107 client/app/queue/utils.js
 110 app/models/hearing.rb
 110 app/models/request_issue.rb
 116 app/controllers/certifications_controller.rb
 118 spec/feature/intake/supplemental_claim_spec.rb
 123 client/app/containers/EstablishClaimPage/EstablishClaim.jsx
 129 spec/rails_helper.rb
 135 app/models/user.rb
 138 app/controllers/application_controller.rb
 140 spec/feature/intake/higher_level_review_spec.rb
 153 spec/feature/establish_claim_spec.rb
 166 lib/fakes/appeal_repository.rb
 171 app/controllers/tasks_controller.rb
 195 spec/models/appeal_spec.rb
 207 app/services/appeal_repository.rb
 223 app/models/task.rb
 380 app/models/appeal.rb

The ones at the bottom are also the most complex.

Solutions

Invest in Onboarding:

From The Effective Engineer (highly recommended book):

Invest in onboarding and mentoring. The more quickly you can ramp up new team members, the more effective your team will be. [...] Training a new engineer for an hour or two a day during her first month generates much more organizational impact than spending those same hours working on the product. Moreover, the initial time investment to create onboarding resources continues to pay dividends with each additional team member.

Examples:

• Facebook 6-week bootcamp for all new engineers
• Google engEDU

Agree on best practices then document them

Examples:

• login.gov best practices
• thoughtbot best practices
• 18F Architecture review for Micropurchase Rails app
• 18F - Limiting technical debt on Micropurchase

Recommended best practices

• Improvement Over Consistency. One book on the shelf and five in the pile is better than six books in the pile. (from Sarah Mei's Livable Code talk and Twitter thread mentioned above)

• Leave code better than you found it each time you touch it. A great way to make incremental improvements is to go through the 30-Day Code Quality Challenge.

• Spend time coming up with well-named methods that convey both the what and the why. Examples:

  • https://thoughtbot.com/blog/code-that-says-why-it-does

• Keep classes small

  • https://thoughtbot.com/blog/skinny-controllers-skinny-models
  • https://www.rubytapas.com/2017/04/11/skinny-controller-domain-model-events/
  • https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/
  • https://thoughtbot.com/upcase/videos/form_objects

• Keep controllers small and use separate classes to handle the business logic

  • Moncef wrote a Tech Spec to explain how this pattern works and how it's beneficial.
  • Examples of this pattern in the wild:
    • https://github.com/gitlabhq/gitlabhq/blob/master/app/controllers/projects_controller.rb
    • https://github.com/houndci/hound/blob/master/app/controllers/activations_controller.rb

• Follow SOLID principles. Dependency Injection is especially useful.

• Write detailed commits with a consistent format. Keep all details in Git vs GitHub only.

  • https://thoughtbot.com/blog/5-useful-tips-for-a-better-commit-message
  • https://thoughtbot.com/blog/better-commit-messages-with-a-gitmessage-template

• Measure code quality over time.

Action items

• Moncef will set up Code Climate
• Moncef will set up a meeting for engineers to discuss and document the practices we want to adopt consistently going forward.