The impact of file position on code review

This is why I always review the files with bugs in them first.

This is kind of a weird aside, but I attended many of my kids' music lessons. The teacher would ask them to play a selection, and then stop them after the first few measures, to work on some point of technique or interpretation. It meant that they never got to the end of anything, until the recital.

"Measure position" had a strong impact on review.

I realized that perfecting the entire selection isn't the point. Instead, the point is for the student to learn how to critique themselves, and improve their practicing.

IMO file/folder structure in a project is a geographic mapping exercise. It's not that far off from city planning. I think it's so important to design the structure and naming conventions of a project with the ultimate goal of aesthetically-pleasing, human-understandable spatial organization of digital artifacts. You "go here" to "see that". This "lives here". You travel to the destination of the thing you're looking for, and gain a deeper understanding of how the world is constructed that you are inhabiting.

This is one of my pet peeves with any kind of cloud file storage -- oftentimes they try to reinvent or abstract away the idea of a deeply-nested filesystem tree, and it just feels anti-human to me lol

Edit: I hit submit and then acknowledged I was kind of ranting about something else entirely from the paper itself!

> We found files shown earlier in a PR to receive more comments than files shown later

I often stop reviewing a PR if I have too many comments on it already that need to be addressed so I wouldn't say this is necessarily what it sounds like (later files get less attention). Also, a comment on an early file might address a concern in a later file that will be fixed by the time I actually review the later files.

That said, keep your PRs short and you won't have to worry about stuff like that.

People review files top down?

I can't imagine doing that. I glance at the list, and try to spot the highest level change (schema changes, interfaces, etc) and start there. If the core idea is wrong no point wasting time on the rest.

Then I go up from there to implementation details and tests. Often jumping between modules or functions, coming back to some things later when I have more context.

My coworker Toby Crawley recently developed this Firefox plugin to randomize the order of files in GitHub PR reviews:

https://addons.mozilla.org/en-US/firefox/addon/github-pr-fil...

I'm curious if this actually tells us anything about the quality of the reviews, though? You could easily surmise that you make some basic comments on structure and some patterns in a file and send it back to the coder with a general "take that advice to the full change, let me know when I can look at it again."

That is, it isn't necessarily the case that the other files weren't reviewed. Just any overarching comments could have been left as an exercise for the coder. No?

I've recently started using Github code reviews for a lot of C++, and one thing that I wish it would do is show the header (.h) files before the implementation (.cc) files.

Small PRs help, but I often end up just opening a handful of windows to have everything open at once.

Note to self: bury malicious code in "later"-reviewed files.

One of the few times I've read a paper before it was posted.

I like this paper. While it's not anything groundbreaking or sensational, it did help me with the design of something I am working on.

One thing my colleagues and I do is expressly offer up a starting point when reviewing changes: "the meat of the change is in xxx.go; the rest is ancillary/supporting". This allows the reader to contextualize themselves more quickly in the change, and understand the rest of the changes with greater ease.

You could take an adversarial view of it: maybe it's better if people get dropped into the changes so they're left to workout if the changes in each file stand alone rather, decontextualized from their original change.

I think the whole concept of source files as unordered blobs on the filesystem is just _wrong_ -- and a serious drag on the ability of a reader to quickly digest a codebase and make inferences about changes in isolation ...

Codebases are _ordered_ constructs -- regardless of the ridiculous broken build system abstractions that do everything possible to obscure this truth ...

I have always preferred small, self-contained commits for this reason. The GUI tooling for line based commits has left me wanting though. Maybe an LLM can be trained to demux commits.

It's a angle I hadn't considered, but I'm aware that I likely do this so the impact of the paper is that I be more dilligent. Thank you for sharing.

Heh, I've had certain modules where I ended up prefixing the files with `a_initialization.lang` `b_startup.lang` `c_run_the_thing.lang` [...] to make it easier to read for humans. Just like, in, say, a book, where the chapters appear in a specific order, the order of things matters! Probably this is a literate programming sort of thing?

Makes sense. As a reviewer I take active steps to focus primarily on the main change by "minimizing" (Phabricator) test and other incidental file changes in my first pass.