476: Green Flags for Code
Summary
Joel and Sally open the episode by sharing personal updates. Sally has been altering her own clothes for a better fit, finding it empowering. Joel has been reading a blog series by an ancient Roman historian focusing on the lives of peasants throughout history, debunking myths about their leisure time compared to modern workers.
The conversation shifts to the main topic: identifying ‘green flags’ or positive heuristics when reviewing pull requests (PRs). Both hosts agree that a concise, focused PR with a good description is a strong initial indicator of quality. They discuss how overly large PRs (e.g., 1,500+ lines) can signal a lack of thoughtful decomposition. They appreciate when PRs include context about alternative approaches considered and the reasoning behind decisions, a practice they highlight from teammate Justin Taniazzo.
They delve into code structure signals, such as well-named methods that allow for high-level understanding before diving into details, and the value of extracting methods for readability even for single-use cases. Joel connects this to his ‘triangle of separation’ principles (single level of abstraction, separating branching from doing, pushing conditionals up). They discuss strategies for breaking down large features into smaller, full-stack PRs that deliver incremental value, contrasting this with splitting work by architectural layer (frontend/backend).
The hosts explore differences in reviewing React versus Rails code, noting the temptation in React to cram logic into single files due to import/export overhead. They conclude that a key green flag is code that can be read at multiple levels of granularity—from the PR description and file list, to the public interface, down to implementation details—which itself is a heuristic for good design. Finally, they touch on AI-generated code, questioning whether AI can replicate true quality signals like thoughtful PR descriptions and whether AI-written tests might look polished but be coupled to implementation or lack value.
Recommendations
Blogs
- Ancient Roman Historian’s Blog Series on Peasants — A blog series by an ancient Roman historian that Joel is following. It provides a deep dive into the lives of peasants throughout history, examining their economic incentives, household structures, and debunking modern myths about their amount of leisure time.
Concepts
- Triangle of Separation — A set of three coding principles mentioned by Joel: 1) Write code at a single level of abstraction, 2) Separate branching code from doing code, and 3) Push conditionals up the decision tree. He notes these principles converge towards a style of code with clear, layered methods.
People
- Justin Taniazzo — A teammate mentioned by Sally as an exemplar of good PR practices. He is noted for writing excellent PR descriptions and adding inline comments to explain his reasoning, considered alternative approaches, and provide additional context for reviewers.
Topic Timeline
- 00:00:56 — Personal Updates: Sewing and Ancient History — Sally shares her recent hobby of altering clothes for a better fit, finding it empowering. Joel discusses his deep dive into a blog series about the history of peasants, debunking myths about their leisure time compared to modern workers. They briefly compare historical and modern work-life balance.
- 00:07:15 — Introducing Heuristics for Code Review — Joel asks Sally about heuristics used to quickly assess the quality of a pull request. Sally immediately points to PR length as a key signal, suggesting very large PRs may indicate a lack of decomposition. They both express love for concise PRs with detailed descriptions that explain the ‘why’ behind changes, especially for tricky bug fixes.
- 00:11:32 — Initial PR Assessment and Code Structure — Sally describes her review process: starting with the PR description and getting a high-level view of the change’s scope and file count before diving into details. They discuss code structure green flags, like concise, well-named methods that allow for understanding at different levels of abstraction. They agree that extracting a method for readability, even if used only once, is a positive sign.
- 00:17:23 — The Value of Context and Alternative Approaches — Sally highlights a specific green flag: when authors add inline comments or PR descriptions explaining alternative approaches considered and why they were rejected. Joel agrees, noting it prevents redundant reviewer suggestions. They discuss related practices, like noting when a minimal implementation is chosen for speed with plans for a future refactor.
- 00:20:48 — Strategies for Breaking Down Large Features — The hosts debate strategies for decomposing large features. Joel prefers breaking work down into small, full-stack tickets that each deliver a slice of user value. Sally considers breaking by architectural layer (e.g., models, then controller) but acknowledges this can lead to mismatches requiring refactoring. They agree on the value of small, reviewable chunks.
- 00:23:37 — Reviewing React vs. Rails Code — Joel asks if review heuristics differ between React and Rails PRs. Sally notes a temptation in React to cram logic into single files to avoid import/export overhead, making file size and structure a key indicator. She likes being able to understand the PR’s approach from the list of changed file names, treating it like a table of contents.
- 00:27:54 — Code Readable at Multiple Levels of Detail — They synthesize their discussion into a core heuristic: high-quality code can be read at multiple levels of granularity. You can understand the overall purpose from the PR and file structure, the public interface from method names, and then drill into details. This layered readability is itself a strong green flag for good design.
- 00:30:54 — AI-Generated Code and Superficial Signals — Connecting to a broader conversation about AI, they discuss signals that might look like quality but aren’t. Sally points to long, thorough-looking tests that might be generated by AI but don’t actually test the change or are overly coupled to implementation. They speculate whether AI doing Test-Driven Development (TDD) might produce better tests than AI writing tests for existing code.
Episode Info
- Podcast: The Bike Shed
- Author: thoughtbot
- Category: Technology News Tech News
- Published: 2025-09-30T07:00:00Z
- Duration: 00:36:39
References
- URL PocketCasts: https://pocketcasts.com/podcast/ce1490c0-4406-0132-c909-5f4c86fd3263/episode/95672dbc-f8c9-454a-87e1-19a1b96a410c/
- Episode UUID: 95672dbc-f8c9-454a-87e1-19a1b96a410c
Podcast Info
- Name: The Bike Shed
- Type: episodic
- Site: https://bikeshed.thoughtbot.com
- UUID: ce1490c0-4406-0132-c909-5f4c86fd3263
Transcript
[00:00:01] Autoscaling is a simple concept.
[00:00:04] Automatically add resources when needed,
[00:00:06] and automatically shut them down to avoid paying for excess capacity.
[00:00:10] How hard could that be?
[00:00:12] If you’ve set up autoscaling yourself, you know it’s not that easy,
[00:00:16] especially using the native autoscalers on platforms like AWS and Heroku.
[00:00:21] That’s why you need JudoScale.
[00:00:23] JudoScale is autoscaling as a service,
[00:00:26] and they make autoscaling simple and easy, as it should be.
[00:00:29] You can use JudoScale on AWS, Heroku, Render, Fly.io, and more.
[00:00:35] It’s free for low-traffic apps.
[00:00:37] And unlimited plans start at $25 per month.
[00:00:40] Autoscale on easy mode at judoscale.com.
[00:00:56] Hello and welcome to another episode of The Bike Shed.
[00:00:59] A weekly podcast from your friends at Thoughtbot about developing great software.
[00:01:03] I’m Joel Kenville.
[00:01:05] And I’m Sally Hall.
[00:01:07] And together we’re here to share a bit of what we’ve learned along the way.
[00:01:10] So Sally, what’s new in your world?
[00:01:13] Ooh, I’ve been spending a little more time sewing recently.
[00:01:18] I mean, I always, I do a lot of crafts.
[00:01:21] But I’ve sort of taken it in a new direction of trying to alter clothes I already own
[00:01:26] to fit better or work better for me,
[00:01:28] rather than getting rid of them and buying new ones.
[00:01:31] And it’s been kind of empowering,
[00:01:34] like, realizing that I have,
[00:01:36] it’s like a superpower to, like, make these pants fit
[00:01:39] or make that dress less ugly.
[00:01:43] So I’ve been doing a lot of that, which has been really fun.
[00:01:46] That’s really cool.
[00:01:48] Because I feel like custom-tailored clothes that work for you is,
[00:01:53] you know, it’s a luxury.
[00:01:55] It’s the kind of thing that, like, you do,
[00:01:57] you pay a professional to do for, like, a special piece.
[00:02:00] And having the ability to just do that to your normal clothes is really nice.
[00:02:05] Yeah, it’s been really good.
[00:02:07] And, like, you know, the clothes you buy in a store
[00:02:10] are made for one particular body shape that most humans don’t have.
[00:02:14] So, like, none of us can really get great-fitting clothing off the rack.
[00:02:18] And so it’s been really helpful for me to start to learn
[00:02:21] about how to make little adjustments.
[00:02:23] These are not, you know, I’m not, like, tailoring a suit.
[00:02:25] I’m, like, taking in the waistband of a pair of pants.
[00:02:28] But it still feels pretty tremendous.
[00:02:30] Yeah!
[00:02:31] What about you? What’s new in your world?
[00:02:33] I’ve been sort of going on a deep dive on…
[00:02:39] There’s a blog that I follow by an ancient Roman historian.
[00:02:44] He’s been doing a series on ancient, but sort of more broadly
[00:02:50] all periods of time, peasants and what their lifestyle is like.
[00:02:54] What are the various economic incentives that impact them?
[00:02:57] And I think it’s really interesting that he notes is that
[00:02:59] when you study history, you often pay a lot of attention
[00:03:02] to sort of the kings and the nobles and the priests
[00:03:05] and, you know, the type of people that are either writing history
[00:03:09] or having history written about them.
[00:03:11] But the majority of people across all time and places
[00:03:16] have been peasants.
[00:03:18] And so he’s doing sort of a deep dive on, like,
[00:03:21] what is it like to be…
[00:03:23] What is it like to live as a peasant?
[00:03:25] What are peasant households like?
[00:03:28] What are their sort of land tenure?
[00:03:31] What is the interactions with each other, with their neighbors,
[00:03:35] with the sort of more wealthy people around them?
[00:03:39] It’s been really fascinating.
[00:03:41] And it puts out an article every Friday.
[00:03:43] And, I don’t know, it’s maybe halfway through.
[00:03:47] And it’s been going on for a few months.
[00:03:49] So if you really want to nerd out on some things,
[00:03:52] if you want to look at some actual, like, statistics,
[00:03:56] dig into things like life expectancy
[00:03:59] and fertility rates and things like that,
[00:04:02] patterns of family formation,
[00:04:04] you can go as deep as you want.
[00:04:06] That actually sounds really interesting to me.
[00:04:08] I’ve never been a huge fan of history,
[00:04:10] but as I’ve gotten older and, like, experienced history
[00:04:13] and learning about history in different ways,
[00:04:15] I’ve learned that I really like historical fiction
[00:04:17] that’s focused on, yeah, not the ruling people,
[00:04:21] but the everyday people.
[00:04:23] It’s been a lot more interesting to me.
[00:04:25] Also, it surprises me not at all that
[00:04:27] what’s new in your world starts with ancient Roman history.
[00:04:33] On brand.
[00:04:34] Definitely.
[00:04:35] I will have to check that out.
[00:04:37] I think I would enjoy that.
[00:04:39] And we’ll include a link of the show notes
[00:04:41] for anybody else who’s curious about the series.
[00:04:43] I was going to ask, I think a fact that I’ve seen,
[00:04:46] you know, in, like, totally non-reliable Instagram factoid areas,
[00:04:50] is that, you know, like, ancient history,
[00:04:53] peasants actually worked far less
[00:04:55] than we do today as modern people.
[00:04:57] Has he talked about that at all?
[00:04:59] Yes, I think actually part of the reason,
[00:05:01] he’s wanted to talk about peasants for a long time,
[00:05:04] but I think part of it is he’s been seeing that come up a lot,
[00:05:06] and it’s a fact that he’s trying to debunk.
[00:05:09] Okay, so it’s not true?
[00:05:11] According to him.
[00:05:12] And he sort of tabulates a lot of, like,
[00:05:14] what the labor is that goes in.
[00:05:17] So if you want to do the math, it goes into, you know,
[00:05:19] it goes into a lot more depth.
[00:05:22] One of the things that’s kind of crazy for us
[00:05:24] in terms of the sort of amount of leisure time that we have
[00:05:27] is just having a weekend.
[00:05:29] Yeah, that’s a relatively new thing.
[00:05:31] Once you, like, count up the days and, like,
[00:05:33] you take out the weekends,
[00:05:35] it’s not that many days a year that we work.
[00:05:37] Yeah.
[00:05:38] So we have fewer sort of, like, religious festivals
[00:05:40] and harvest festivals and things like that
[00:05:42] than a peasant would,
[00:05:44] but we only work five days a week,
[00:05:46] and that’s really nice.
[00:05:48] People in today’s world get time off for other things.
[00:05:52] Like, I know not everybody does.
[00:05:54] It would be great if we did,
[00:05:55] but, like, paid time off for vacation or sick days
[00:05:57] or things like that,
[00:05:58] which I don’t think was an idea back then.
[00:06:01] A peasant couldn’t just tell the overlord,
[00:06:03] I’m going to go to Italy for a week.
[00:06:05] See you later.
[00:06:07] Right, right.
[00:06:08] The idea of going off for a trip is very uncommon for peasants.
[00:06:12] To a certain extent, they have control over their schedule
[00:06:14] if they’re working their own land,
[00:06:16] but that’s still…
[00:06:20] You’ve got to get your harvest in,
[00:06:22] and it’s an awkward time to get sick.
[00:06:25] Yeah, you’re taking a little risk there, too.
[00:06:28] You’re working on some pretty slim margins, too, right?
[00:06:32] You want to survive the winter,
[00:06:35] and you want to be able to feed your family.
[00:06:39] Yeah, the stakes were very different.
[00:06:41] Well, I mean, I’m sure that those are the stakes
[00:06:43] for a lot of modern people, but…
[00:06:45] Generally, software developers are not struggling
[00:06:48] to get all the code written before winter.
[00:06:50] That’s right.
[00:06:52] Yeah, trying to hope that we have enough database seeds
[00:06:55] to plant next spring or something like that.
[00:06:58] Yeah.
[00:07:01] Grow delicious, delicious SQL queries?
[00:07:04] I don’t know.
[00:07:06] Yeah, yeah, you know, get those fertile servers
[00:07:09] and grow some real high-quality code.
[00:07:12] I guess on the topic of high-quality code, though,
[00:07:15] I’m curious, when you review a pull request,
[00:07:20] you know, somebody sends you a link,
[00:07:22] maybe there’s a lot of code in there.
[00:07:24] Do you ever sort of use some, like, heuristics
[00:07:27] to just sort of, like, scan it and get a sense of, like,
[00:07:29] maybe what the quality is of this PR in general?
[00:07:32] Oh, yeah, I think we all do.
[00:07:34] It’s funny that you said there’s a lot of code in there,
[00:07:37] because that’s actually one of my first ones.
[00:07:39] If you’ve opened a PR with 1,500 lines of actual code changes,
[00:07:44] you know, not like, oh, I’ve pulled in this giant library,
[00:07:48] immediately I’m going to be a little suspicious
[00:07:50] about the quality of that code.
[00:07:52] Not because writing a lot of code inherently is bad,
[00:07:56] but because I think the process of thinking through it
[00:07:59] and breaking it down into smaller steps and smaller PRs,
[00:08:03] even if you do that after the fact,
[00:08:05] is an important part of, like, thinking through the code
[00:08:08] and making sure you’re writing the correct thing.
[00:08:11] And if you don’t take that moment to do that,
[00:08:13] then my first thought is going to be,
[00:08:16] how much of this is just, like, over-engineered,
[00:08:22] extra circling around the point code
[00:08:26] that could have been done smaller?
[00:08:30] What’s the famous quote? Like, I’ve only…
[00:08:32] Sorry, this letter is so long, I didn’t have time to make it any shorter.
[00:08:35] I feel like I’ve heard it attributed to a bunch of people.
[00:08:38] I think most commonly I’ve heard Mark Twain.
[00:08:40] Yeah, that’s what I was going to say, too.
[00:08:42] But yeah, yeah, so sort of that thought.
[00:08:44] If the code is really long, it makes me feel like
[00:08:46] they haven’t taken the time and effort to make it shorter
[00:08:49] or to make it in chunks.
[00:08:51] And so that’s going to be a red flag.
[00:08:54] But I guess in the opposite, if we’re talking about
[00:08:56] sort of, like, what signals good code,
[00:08:59] like, really concise, focused PRs with good descriptions.
[00:09:06] I love that.
[00:09:08] There’s nothing better to me than, like, a two-line PR
[00:09:11] with, like, a six-paragraph PR comment,
[00:09:15] which is maybe a little bit over the top,
[00:09:17] but, like, that’s just so beautiful to me.
[00:09:20] When you’ve found a way to make the code actually concise
[00:09:23] and you’re taking time to sort of walk your colleagues through
[00:09:28] what you’re doing, why you’re doing it,
[00:09:30] maybe why you didn’t do other things.
[00:09:32] I found that those types of PRs,
[00:09:35] the, like, two-line change blog post in the description,
[00:09:41] often are tricky, like, bug fixes
[00:09:44] or security patches, things like that.
[00:09:47] Things where it took me all day
[00:09:50] to trace this and figure it out.
[00:09:53] And eventually it was a one-line fix.
[00:09:56] But there’s a story here and there’s a lot of context
[00:09:59] that you need to know because, again,
[00:10:02] this is the sort of thing that has gone through review
[00:10:04] for a lot of people and nobody, everybody missed the bug.
[00:10:07] Yeah.
[00:10:08] Because of more complex factors.
[00:10:09] So there needs to be an explanation.
[00:10:11] Maybe talk about experiments that I ran,
[00:10:13] like, how I got there, how I proved that this is, in fact, the case.
[00:10:17] Maybe some benchmarks for before and after.
[00:10:20] A test to prevent regression.
[00:10:22] Mm-hmm.
[00:10:23] That’s another thing I like is when there’s a little change,
[00:10:27] but, like, a substantial, not over-tested,
[00:10:31] but, like, enough testing that we can feel confident
[00:10:34] that we aren’t going to have to fix this mistake again.
[00:10:37] Or even, like, guide rails for making it, like,
[00:10:40] making it difficult or impossible
[00:10:42] to make the same kind of mistake in the future.
[00:10:44] Right, right.
[00:10:46] I want to only make every mistake once.
[00:10:48] Yeah.
[00:10:49] The worst are sort of those recurring bugs
[00:10:52] where you think you fixed it and then it starts breaking again
[00:10:56] in a slightly different way next week
[00:10:59] and it’s constantly the same bug that’s popping up.
[00:11:02] Yeah.
[00:11:03] Those are so frustrating.
[00:11:05] And it’s so, like, I feel like that always points to, like,
[00:11:08] this is a process problem, not a people problem.
[00:11:09] Like, we’re all thinking the same way
[00:11:12] and making the same mistake.
[00:11:14] So it’s not any, I mean,
[00:11:16] I feel like I rarely want to throw blame around anyway,
[00:11:19] but it’s, like, certainly not anybody’s fault
[00:11:21] because we’re all making the same mistake.
[00:11:23] So there’s something going wrong with our process
[00:11:25] or the way we’ve set up our repo or something
[00:11:27] that is making this easy to do the wrong way.
[00:11:30] What do you look for in APR?
[00:11:32] Ooh, I think similar to you,
[00:11:34] before I go anywhere, get a sense of the description.
[00:11:37] And then also, like you said,
[00:11:39] the length.
[00:11:40] Partly just because I need to budget
[00:11:41] how long I’m going to sit here for.
[00:11:43] Yeah.
[00:11:44] And a thing that I’ve learned
[00:11:46] is that, like, if you just sort of start
[00:11:48] and you’re doing sort of in-depth review on code
[00:11:51] and you don’t really pay attention to the length,
[00:11:53] it’s easy to go really into details on things
[00:11:56] and then you realize, wait,
[00:11:57] there’s another thousand lines here.
[00:11:59] And the real, I’ve got some meta feedback here
[00:12:01] about, like, your approach in general
[00:12:03] maybe could be restructured.
[00:12:05] And so, like, the type of feedback
[00:12:07] needs to be at a different level.
[00:12:08] And so getting that 10,000-foot view
[00:12:10] of what the PR is about
[00:12:11] before I actually look at the code in detail
[00:12:14] is, I think, an important step.
[00:12:16] Yeah, absolutely.
[00:12:18] Yeah, I’ve definitely gotten into a situation
[00:12:20] where I start reviewing code
[00:12:22] and, you know, maybe I skip that overview step
[00:12:24] and start leaving comments on, like, little things
[00:12:27] like syntax or whatever
[00:12:29] or suggestions for small improvements.
[00:12:32] And then about halfway through, I realize,
[00:12:34] oh, this, I want to suggest
[00:12:36] an entirely different approach.
[00:12:37] And so now all these comments
[00:12:39] that I’ve spent time making
[00:12:41] are kind of irrelevant.
[00:12:42] Exactly.
[00:12:43] I think maybe also having changes
[00:12:47] focused in just a few files
[00:12:50] tends to also be a green flag for me.
[00:12:52] If something is touching 50 different files
[00:12:56] across, like, a thousand-line diff,
[00:12:58] I know this is a real rough, kind of messy PR.
[00:13:02] Yeah.
[00:13:03] And maybe there’s a valid reason for it.
[00:13:05] So again, these are sort of green flags.
[00:13:07] They’re not necessarily for sure
[00:13:10] that your code is bad,
[00:13:11] but they’re definitely a raise an eyebrow,
[00:13:15] take a look at it more.
[00:13:17] If something is on the shorter side
[00:13:19] and only touches a couple of files,
[00:13:21] that to me is a sign of, like,
[00:13:23] okay, this is probably decent quality code.
[00:13:25] I’m going to sit down, understand it,
[00:13:27] and it’s probably relatively easy to approve as well.
[00:13:30] I think related to that,
[00:13:32] I also really like concise, well-named methods.
[00:13:35] Mm.
[00:13:36] You know, if I am looking at a method
[00:13:39] that you added to a class and it’s, like,
[00:13:41] I have to really sit and parse through every line
[00:13:44] before I even understand what it’s doing as a whole,
[00:13:47] that to me can be frustrating.
[00:13:50] But if I see a method and just by its name
[00:13:53] or something about, you know,
[00:13:55] I can know what it’s generally trying to do,
[00:13:58] I think that’s really helpful.
[00:14:00] And I can, you know, with the idea of what this is doing,
[00:14:03] I can see that in the context of the rest of the code,
[00:14:06] without first having to parse the whole method itself.
[00:14:09] Yeah, yeah.
[00:14:11] You can do, like, a high-level review of, like,
[00:14:13] what are these things called
[00:14:14] and how are they interacting with each other?
[00:14:16] And then second level, okay,
[00:14:18] now let’s take them one by one
[00:14:19] and see how they’re doing those things.
[00:14:21] It’s almost like writing parts of your code
[00:14:26] at a high level of abstraction.
[00:14:28] Yeah, yeah.
[00:14:30] And that sometimes can make your code a little bit longer
[00:14:32] if you’re pulling out a lot of, like, private methods
[00:14:34] to give names to classes.
[00:14:36] That’s true.
[00:14:38] Yeah, so that’s going to be a situation
[00:14:40] where longer is better,
[00:14:43] which I think can also be, yeah,
[00:14:46] a really good indicator is that, you know,
[00:14:48] sometimes there’s something that you can do
[00:14:50] in a really cool way in one line
[00:14:52] that feels very snazzy
[00:14:54] but can be difficult to parse,
[00:14:56] difficult for a human to read,
[00:14:58] which then makes it hard to maintain and debug.
[00:15:00] And so taking the time to break that out
[00:15:02] into smaller steps, I think,
[00:15:04] can be really helpful.
[00:15:05] It can be a really good sign.
[00:15:07] You know, the idea of, like,
[00:15:09] some people will only, like, extract a method
[00:15:11] if it’s being used in multiple places.
[00:15:13] But I think extracting a method
[00:15:15] that is only used once
[00:15:17] can sometimes be helpful
[00:15:19] if it makes it more readable, more manageable,
[00:15:21] and more maintainable.
[00:15:26] Have you ever tried to debug an issue in production
[00:15:28] while switching between your error tracker,
[00:15:30] log viewer, and APM tool?
[00:15:32] Maybe you know how painful it is
[00:15:34] when you can see the error
[00:15:36] but can’t find the relevant logs,
[00:15:38] or when you see slow performance
[00:15:40] but it can’t connect to the root cause.
[00:15:42] Scout Monitoring is the all-in-one platform
[00:15:45] built to make your life easier.
[00:15:47] With log management, performance tracking,
[00:15:49] and error monitoring coming soon,
[00:15:51] all of your data will be together at last.
[00:15:54] It will go from bug to fix in one place.
[00:15:57] The result?
[00:15:58] Less context switching,
[00:16:00] faster debugging,
[00:16:01] and monitoring that actually helps you ship code
[00:16:03] instead of distracting from it.
[00:16:05] No DevOps team?
[00:16:07] No problem.
[00:16:08] Scout works with your existing stack
[00:16:10] and gives you insights in five minutes, not hours.
[00:16:13] From bug to fix in one place at scoutapm.com.
[00:16:20] I’m a huge fan.
[00:16:21] I’ve talked about my sort of trio of principles
[00:16:24] that I call the triangle of separation
[00:16:26] on the show before.
[00:16:28] They all sort of converge towards the same set of,
[00:16:31] I guess,
[00:16:32] the same style of code.
[00:16:34] Those being write code at a single level of abstraction.
[00:16:38] So don’t mix high and low level code.
[00:16:40] Separate branching code from doing code.
[00:16:43] So if you’re doing conditional,
[00:16:45] you don’t get to have an implementation
[00:16:47] in the branches of the conditional.
[00:16:48] You can only call other methods.
[00:16:50] And then finally push decisions up the,
[00:16:53] or push conditionals up the decision tree.
[00:16:55] Turns out that all of those sort of,
[00:16:57] I think, converge towards the style of code
[00:16:59] that we’re talking about right here,
[00:17:00] which is,
[00:17:01] at sort of a high level,
[00:17:03] you have a sort of almost like domain method
[00:17:05] on your object
[00:17:07] that is implemented in terms of other things internally.
[00:17:10] But they’re all just sort of high level calls.
[00:17:12] So you get a sense of,
[00:17:13] okay, this is what this method is doing.
[00:17:14] And then you can drill down into any of them
[00:17:16] if you want
[00:17:17] to see what the implementation is.
[00:17:20] I just thought of another like PR related thing
[00:17:23] that’s not necessarily how the code is written.
[00:17:25] It is just a huge green flag for me.
[00:17:28] And I’m going to call out one of our teammates,
[00:17:30] Justin Taniazzo,
[00:17:31] who I’m lucky enough to be working with
[00:17:33] on a project right now,
[00:17:34] does a great job of not only doing a great PR description,
[00:17:39] but then commenting on his own PR at points
[00:17:43] where code might need additional explanation
[00:17:46] or clarification or questions.
[00:17:50] So reading through a PR,
[00:17:52] there’ll be the description
[00:17:54] and then there’ll be parts of it where he might say,
[00:17:56] I thought about approach A,
[00:17:58] but went with approach B here
[00:17:59] because of X, Y, Z in line,
[00:18:03] which I think can be really helpful
[00:18:04] for going through a PR
[00:18:06] and sort of understanding the work that went into it.
[00:18:10] That is something that I really appreciate seeing,
[00:18:12] like you said,
[00:18:13] either in line or in the PR description,
[00:18:16] the alternate approaches that were considered,
[00:18:19] but not taken.
[00:18:21] Because if not,
[00:18:22] I’m the reviewer and I’d be like,
[00:18:23] oh, have you considered taking this approach?
[00:18:26] And it’s nice to be able to know,
[00:18:27] yes, they did.
[00:18:28] And they chose not to for these reasons.
[00:18:31] I think a variation on that can also be,
[00:18:33] I chose this approach for now,
[00:18:35] but if something changes
[00:18:36] and these other conditions apply in the future,
[00:18:39] we should consider doing this other approach.
[00:18:41] Yeah.
[00:18:42] Or I chose this approach now as an urgent bug fix,
[00:18:45] but I think we should fast-follow refactor it this way.
[00:18:49] Yeah, I think the version of it that I’ve seen a lot is the,
[00:18:53] we’re doing a more minimal implementation here
[00:18:55] and we sort of made these trade-offs to say,
[00:18:57] look, this is going to ship faster
[00:18:59] or this is going to be smaller
[00:19:00] for our sort of initial launch or whatever.
[00:19:03] All these other things that we looked at are potentially nice,
[00:19:07] but we haven’t quite been able to justify the need.
[00:19:10] But in six months,
[00:19:12] we may want to evolve this in this direction.
[00:19:15] Which can also be calling out
[00:19:18] why you’ve broken something down in certain ways too, right?
[00:19:21] So like if I’ve got a big piece of work that I’m working on
[00:19:24] and I decide that it actually needs to be split in half,
[00:19:27] I’m going to go into three or four PRs.
[00:19:29] If my teammates are aware of the problem I’m trying to solve,
[00:19:32] they might see the first PR and be like,
[00:19:34] this is an incomplete solution, right?
[00:19:36] So putting, if you know that you’re doing work in chunks,
[00:19:40] putting each chunk in context of the other chunks
[00:19:43] and linking to things can be really helpful.
[00:19:45] So something like, you know, this sets up the whole back end,
[00:19:49] but the front end part of this will be a different PR
[00:19:52] or like, this is all of the…
[00:19:55] I don’t like that style.
[00:19:56] I don’t like that style for a ticket,
[00:19:58] but I think sometimes that can be really useful for PR,
[00:20:01] especially if you’re doing complicated front end stuff.
[00:20:04] So I might start out with,
[00:20:05] here I’m going to add some models that we’re using
[00:20:08] and sort of set up the structure of this data
[00:20:11] and then actually use the data in a subsequent PR
[00:20:14] just so that they’re easier to review one by one.
[00:20:16] I tend to like to do things as full stack as possible.
[00:20:20] And sometimes you have to split it that way.
[00:20:22] But I like every PR to actually deliver value.
[00:20:26] Rather than just sort of…
[00:20:28] I guess, you know, you can have like a refactor PR
[00:20:30] that’s maybe delivering value in a different way.
[00:20:33] I usually try not to add dead code that’s like,
[00:20:36] oh, I added models.
[00:20:38] They don’t do anything.
[00:20:39] Eventually I’m going to wire up a controller
[00:20:42] and then eventually I’m going to wire up a view
[00:20:44] and instead sort of work across the stack
[00:20:46] rather than each layer at a time.
[00:20:48] That’s interesting.
[00:20:49] I think it’s important to do that when planning work.
[00:20:51] And like, I want to have the person who owns,
[00:20:56] the piece of work own the whole stack, ideally.
[00:20:59] But I think breaking it up front end, back end,
[00:21:02] or like database controller or something
[00:21:05] can sometimes be a helpful way to break it down.
[00:21:10] As I’m saying this, though,
[00:21:12] I’m realizing that that definitely leads to situations
[00:21:14] where it’s like, you know,
[00:21:16] the model on its own kind of makes sense,
[00:21:18] but without knowing how we’re using it,
[00:21:20] I can’t know.
[00:21:21] And then the next PR comes that’s the controller using it
[00:21:24] and now the model seems off.
[00:21:26] Right, and you end up having to refactor the model
[00:21:28] as part of the controller PR.
[00:21:30] So given that, how would you want to break down
[00:21:33] like a big, big piece of work
[00:21:35] that you know needs to be broken down into smaller PRs?
[00:21:38] I usually break it down by ticket.
[00:21:43] So like you were saying,
[00:21:44] you like the tickets to be full stack,
[00:21:45] I will try to make the tickets really small.
[00:21:47] Hmm.
[00:21:48] Where each ticket is delivering an actual little chunk of work,
[00:21:51] some amount of value,
[00:21:53] and like maybe that ticket is doing something
[00:21:55] really, really minimal.
[00:21:57] So there’s a dashboard that you want.
[00:22:00] It has to be full stack
[00:22:01] and there’s all these like complicated behaviors on it.
[00:22:03] And maybe version one is just,
[00:22:05] there is a dashboard that prints your name.
[00:22:07] And that is it.
[00:22:09] But everything works full stack.
[00:22:11] And then one by one,
[00:22:13] maybe I introduce sort of one widget at a time
[00:22:15] on the dashboard, but full stack.
[00:22:18] But find ways where each commit is delivering
[00:22:22] some small new feature into it.
[00:22:25] Yeah.
[00:22:26] The tricky thing there is that you have to learn
[00:22:28] to really split features rather than architectural layers.
[00:22:33] And that’s hard.
[00:22:34] Yeah.
[00:22:35] I think the work I’ve been doing recently,
[00:22:37] now that we’re talking about it,
[00:22:39] I could be breaking it down better,
[00:22:41] which is something that I usually feel like
[00:22:43] I am good at and proud of.
[00:22:48] So I’m going to take that back with me this week
[00:22:51] and see, start focusing more on,
[00:22:53] is this ticket too big?
[00:22:54] How can I make it smaller?
[00:22:56] I think practices also vary within teams.
[00:22:59] So I’ve been on teams that have been like sort of
[00:23:01] really heavy on not doing this in this way.
[00:23:04] So I might bring a little bit of that to the team,
[00:23:06] but I’m not going to force them to totally redo
[00:23:09] the way that they work.
[00:23:11] Yeah.
[00:23:12] Just say, hey, you know, there’s some value
[00:23:13] in like scoping your tickets smaller
[00:23:15] because it allows you to ship faster.
[00:23:18] It allows you to get better reviews,
[00:23:20] that kind of thing.
[00:23:21] It’s also really helpful for isolating breakages.
[00:23:23] Right?
[00:23:24] So if I’ve merged two giant PRs this week
[00:23:27] and then the code stops working,
[00:23:28] it’s a lot harder to focus down on exactly
[00:23:31] which change caused the problem.
[00:23:34] I wonder if there are maybe some language-specific
[00:23:37] things for you as well?
[00:23:39] I know you’ve been working on a lot of
[00:23:41] more React code recently.
[00:23:43] Do you find that the way you review
[00:23:46] or even give that sort of, you know,
[00:23:48] two-minute analysis at the beginning of a PR
[00:23:50] before you dig into review,
[00:23:52] is that different when you’re looking at a React PR
[00:23:54] versus a Rails PR?
[00:23:56] Ooh, that’s a great question.
[00:23:58] Although ideally, you know, shouldn’t it be both
[00:24:00] because we’re doing the full stack?
[00:24:01] Fair.
[00:24:04] I think it can be really tempting
[00:24:07] to cram a whole bunch of stuff into one file
[00:24:10] in a React PR in a way that is, I think,
[00:24:13] more natural to avoid in a Rails or Ruby PR,
[00:24:17] which could just be a reflection
[00:24:19] of my experience with it.
[00:24:21] I think that the structure of a Rails app
[00:24:24] is more intuitive to me at this point than a React app.
[00:24:27] But I also see, like, you know,
[00:24:30] we’re making this huge change and, you know,
[00:24:33] I don’t necessarily want to see something
[00:24:35] that touches every single file,
[00:24:37] but I also don’t necessarily want to see something
[00:24:40] that triples the size of this one file
[00:24:43] when it might make more sense to break it out.
[00:24:45] And I think part of the temptation there is that
[00:24:48] having to import an extra file
[00:24:51] and export things can make it tempting
[00:24:54] to be a little bit lazy about that.
[00:24:57] And like, you know, if I extract this out
[00:24:59] into a whole different file or a whole different class
[00:25:01] or whatever, then I have to import it here
[00:25:03] where I want to use it.
[00:25:04] So I’m just going to put it all here because it’s easier,
[00:25:07] which I think is less of a temptation in Rails world
[00:25:09] because you don’t have to be directly importing
[00:25:12] and exporting things like that.
[00:25:14] So that is definitely one thing I’ll look for is, like,
[00:25:17] have we crammed a whole bunch of stuff into one file
[00:25:19] that actually belongs separate?
[00:25:21] So as a sort of a green flag,
[00:25:24] if you’re scanning a PR to get a sense of very rough,
[00:25:29] almost internal grade for the quality of the code,
[00:25:32] seeing the React code, if it’s over a certain size,
[00:25:35] maybe in a few different files,
[00:25:37] is a thing where you’re immediately like,
[00:25:39] okay, that feels good.
[00:25:40] Yeah, or seeing something where I can look at the list of files,
[00:25:43] just look at the list of file names that have been changed or added
[00:25:46] and get a sense of what are we doing here?
[00:25:49] Like, what’s the purpose of this PR?
[00:25:51] Right?
[00:25:52] If it’s a PR, if every PR just changes, like, the index file,
[00:25:56] then I don’t really know what’s going on.
[00:25:58] But if you’ve added, you know, some, we’re doing TypeScript also,
[00:26:02] which is a fun new adventure for me that I actually love.
[00:26:05] But if you’re adding a couple of types and then adding a component
[00:26:08] that you then import into this page,
[00:26:10] sometimes you can get a better sense of what is the overall approach
[00:26:14] to this change just from reading through the file names.
[00:26:17] And so I think if I can sort of read the PR description
[00:26:20] and then take a glance at the file names and see what’s involved,
[00:26:24] that can often give me a good gut check of, like,
[00:26:27] where is this going?
[00:26:28] What story are we telling here?
[00:26:30] Like a table of contents.
[00:26:32] Yeah.
[00:26:33] So the GitHub will give you that gutter on the side
[00:26:37] that shows you all of the, like, the little file tree.
[00:26:41] Yeah.
[00:26:42] Is that something that you find yourself scanning
[00:26:44] at the beginning of a code review?
[00:26:46] Absolutely.
[00:26:47] Yeah.
[00:26:48] A lot of times I scan that
[00:26:49] and then like to start with the tests, you know?
[00:26:51] So it’s sort of like a bottom-up reading thing.
[00:26:54] Mm-hmm.
[00:26:55] Because a well-written test is often a guide for, like,
[00:26:59] this is what I think this code should do and this is how this works
[00:27:02] and this is the story I’m trying to tell with this,
[00:27:06] which might be a cheesy way to think about it.
[00:27:08] But, like, if you’re trying to add value with every PR,
[00:27:10] then there’s a little tiny user story in there that you can tell
[00:27:13] and you can tell it well.
[00:27:15] Yeah.
[00:27:16] So scan the file thing and then sort of start bottom-up.
[00:27:19] Well, at least the structure of the projects I’m working on,
[00:27:22] the tests tend to be at the bottom
[00:27:24] because test is alphabetically later or spec.
[00:27:27] Yeah.
[00:27:28] So scan that, start bottom-up, start to see, like,
[00:27:31] am I understanding where we’re going with this?
[00:27:33] Which I’m starting to see a pattern in the things that we’re saying
[00:27:36] is that, like, being able to approach it high level
[00:27:41] and then, like, sort of more detail and then more detail
[00:27:45] as you go into the code review feels like a good sign
[00:27:48] at the end.
[00:27:49] As opposed to, I have to absorb every single detail
[00:27:51] before I really understand what’s happening.
[00:27:53] Yeah.
[00:27:54] So almost like the fact that the code can be read
[00:27:57] at multiple levels of detail is itself heuristic for code quality.
[00:28:04] Right.
[00:28:05] Which sounds like it could be contradictory to your guideline
[00:28:11] of writing at, like, one level of abstraction.
[00:28:13] But I don’t think it is.
[00:28:15] I’m not saying that the code should be abstracted at different levels,
[00:28:18] but that I should be able to absorb it
[00:28:21] at different levels of granularity.
[00:28:24] Right.
[00:28:25] And get more meaning as I go deeper for each one.
[00:28:28] And I think you can get that a little bit
[00:28:30] just from the structure of a Ruby file,
[00:28:33] even if you’re using the approach that I’m using,
[00:28:35] because all the, like, implementation details end up in private method.
[00:28:39] So you just sort of scan everything above the private keyword
[00:28:42] and be like, okay, this object does two or three things.
[00:28:47] This is its public interface.
[00:28:49] And each of them look like they call, you know, a few private methods.
[00:28:53] They’re named with the behavior that they do.
[00:28:55] So I get a sense that, like, okay, this thing is making a network request
[00:28:59] and constructing an instance of a value object or something like that.
[00:29:03] Or this is validating an input and writing to a file
[00:29:07] without necessarily knowing the details of what that validation is.
[00:29:10] Yeah.
[00:29:11] I don’t think I had thought about it explicitly like that before.
[00:29:13] But the idea of, like, the public code is what it’s doing,
[00:29:16] and then the private code is the details of how it’s doing it.
[00:29:19] Yeah, that is an interesting way of putting it.
[00:29:21] Which I guess makes a little bit of sense,
[00:29:23] because if we’re taking that even more abstract,
[00:29:25] before we’ve even gotten into the code,
[00:29:27] when we’re talking about value to the user,
[00:29:30] like the user story or ticket description or whatever,
[00:29:33] is going to be what we need to do,
[00:29:35] but not how we need to do it necessarily.
[00:29:38] So as you get deeper into the code, it’s more how.
[00:29:41] And hopefully, you know, that’s sort of the level at which your specs are written.
[00:29:45] So this conversation partly came out of a broader conversation
[00:29:49] around AI tooling in the workplace
[00:29:52] and how it often creates artifacts that sort of look polished,
[00:29:57] but then when you dig into them, are not.
[00:30:01] And that got me sort of wondering about, you know,
[00:30:04] what is polish or signals that we have that code is high quality
[00:30:08] when we’re looking at code rather than a slide deck or something like that.
[00:30:13] Uh-huh.
[00:30:14] I feel like a lot of the signals that we talk about
[00:30:17] are a little bit harder for AI to replicate.
[00:30:19] And I think PR descriptions in general
[00:30:21] is an area where I’ve seen AI just fall flat
[00:30:24] because it summarizes oftentimes the diff.
[00:30:26] They’re beautifully formatted, though.
[00:30:28] Yeah, yeah.
[00:30:30] And it’s probably not a bad thing.
[00:30:32] I probably wouldn’t hate having just a button to summarize the diff in GitHub.
[00:30:37] Less as a PR description,
[00:30:39] because I feel like I want other things when I read a PR description.
[00:30:42] But sort of additionally,
[00:30:45] if there was just a pop up a modal and give me a summary
[00:30:48] or give me a summary in a sidebar or something,
[00:30:51] that might not be a bad thing either.
[00:30:54] But I think in a description,
[00:30:56] I’m looking for more sort of the purpose of the PR,
[00:30:59] paths that we went down and chose not to take,
[00:31:03] trade-offs that were considered,
[00:31:05] general sort of architecture strategy, things like that.
[00:31:10] So what are some things that we think, you know,
[00:31:11] things that we think might be stuff that AI or,
[00:31:16] you know, insert other creator of code might do
[00:31:22] that appear to be signs of good code but that are not?
[00:31:26] Right?
[00:31:27] So like what are some of the like shiny wrappers
[00:31:29] that don’t actually signify what they seem to at first?
[00:31:34] And I guess I can start.
[00:31:36] One of them I think is really long, thorough tests.
[00:31:41] Can look like a great thing.
[00:31:43] And I do love well-tested code.
[00:31:46] But I find that AI will sometimes generate a really long test
[00:31:49] that actually ultimately doesn’t do anything.
[00:31:52] And tests are often something that we,
[00:31:55] people don’t review quite as much detail.
[00:31:58] So it’s easy to sort of get those just merged
[00:32:00] and maybe there’s some subtle things there that are not as valuable.
[00:32:03] Yeah.
[00:32:04] And I think like one of my heuristics for it,
[00:32:06] did I write a good test or not,
[00:32:08] is if I remove the code that I’ve included,
[00:32:11] in this change, does this test fail?
[00:32:13] Right.
[00:32:14] You can write some really beautiful-looking tests
[00:32:16] that seem to be doing interesting and worthwhile things
[00:32:19] and then they don’t actually test the changes that you’ve made.
[00:32:23] And that’s a challenge, I think,
[00:32:25] even when you’re manually writing your tests,
[00:32:27] if you’re using a test-after approach.
[00:32:30] And oftentimes I feel like I can tell just by reading the spec
[00:32:35] whether it was written test-first or test-after.
[00:32:38] Oh, that’s interesting.
[00:32:40] Just because writing test-after,
[00:32:42] you’re now aware of your implementation,
[00:32:44] that causes you sometimes to write the test in a different way.
[00:32:47] What does it look like when a test has been written after?
[00:32:49] Can you identify some sort of like signs or patterns that you see in those?
[00:32:55] It tends to be more coupled to the implementation.
[00:32:58] It might do some more mocking.
[00:33:01] It might be tied to particular methods being called,
[00:33:04] that sort of thing.
[00:33:06] It might be also the setup data might be done in a way
[00:33:10] that’s like very particular to a way that a method has been structured
[00:33:16] in a way that’s not necessary if all you knew was the interface.
[00:33:21] So like there’s some secret data tricks you have to do
[00:33:25] for this test to do what we want it to do that aren’t…
[00:33:28] They’re not even required is the thing, right?
[00:33:31] Oh, okay.
[00:33:33] But because you happen to know that it’s implemented in a certain way,
[00:33:37] you wrote the setup data
[00:33:39] to kind of match that,
[00:33:41] even though it’s not necessary for the data to be in that way.
[00:33:45] Yeah.
[00:33:46] So maybe it just so happened that you wrote it that way,
[00:33:49] but odds are you wrote it that way because you know of the implementation already.
[00:33:53] It sounds like the resulting tests from those two approaches would be,
[00:33:57] you know, the test first approach would probably produce a
[00:34:02] simpler and maybe smaller test
[00:34:04] than the one where we’re digging into
[00:34:07] where you can tell that we are
[00:34:09] relying on the implementation details
[00:34:11] or the complicated setup.
[00:34:14] Does that sound true?
[00:34:16] Yes.
[00:34:17] I think when you write test first,
[00:34:18] you often are testing more of the interface.
[00:34:24] So you say, given these inputs,
[00:34:26] I expect these outputs or I expect these side effects.
[00:34:30] And the how we get there is sort of up to,
[00:34:34] you know, when I get to the green phase of my TDD.
[00:34:37] I have gotten AI agents to do TDD.
[00:34:42] You have.
[00:34:43] I don’t know that I’ve really put much thought into seeing
[00:34:48] if the style of tests it creates
[00:34:51] are different with one form versus another.
[00:34:54] So I know humans tend to overmock when they test after.
[00:34:59] And oftentimes if you’re testing first and the test is painful,
[00:35:03] then that leads to refactoring your implementation.
[00:35:06] And I have seen situations where AI overmocks as well.
[00:35:10] And I think those situations have tended to be,
[00:35:13] here’s an implementation, write a test for it.
[00:35:16] It’s quite possible that having a TDD helps reduce that behavior
[00:35:20] just like it does for humans.
[00:35:22] But that’s not a thing I’ve paid attention to.
[00:35:24] That’s a really good question.
[00:35:26] I would be interested to sort of explore that and see,
[00:35:28] you know, does asking an agent to approach it in a TDD way
[00:35:32] improve the results?
[00:35:34] It seems like it could.
[00:35:36] Because like you were saying, as a human doing tests first,
[00:35:40] I’m going to get a deeper understanding of what’s going on.
[00:35:43] And I hesitate to use the word understanding with AI,
[00:35:46] but it does seem to have at least better context,
[00:35:50] you know, if it starts with what you’re actually trying to accomplish
[00:35:54] rather than what you’ve already written.
[00:35:57] It’s almost like knowing the implementation
[00:36:00] almost like pollutes the context.
[00:36:03] I feel that happens to humans.
[00:36:05] But I wouldn’t be surprised if there’s something kind of like that
[00:36:08] that happens for the AI as well.
[00:36:11] Maybe do some experiments.
[00:36:14] We should follow up on this and see.
[00:36:18] Try to come back to this later.
[00:36:20] Have we done those experiments?
[00:36:21] What have we learned?
[00:36:23] Uh-huh, yeah.
[00:36:24] AI tests, how do you make them better?
[00:36:26] Can they be made better?
[00:36:28] Yeah, something to maybe talk about in our recap episode
[00:36:31] at the end of the season.
[00:36:33] On that note, shall we wrap up?
[00:36:34] Sounds good.