Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The impact of file position on code review

arxiv.org

Post reply on HN
Print view View original

31–38 of 38 posts

Re: The impact of file position on code review

#31

Post by astrange »

kevinsync wrote: post #15

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…

This is my problem with almost every UI framework invented in the overengineered era of the 90s-2000s.

Try to find a feature in basically any open-source C++ or Java GUI app just by looking for the code that does it. You can't, because there isn't any code that does anything! Instead there's several different widely scattered pieces of boilerplate, none of which individually has any meat in it.

Re: The impact of file position on code review

#32

Post by IceDane »

MarkusQ wrote: post #26

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

I find it easiest to just leave out the bugs in the first place. I put them aside for a rainy day.

I now have a collection spanning a decade's worth of bugs. Just waiting for a reason to unleash them on the world. It will be biblical.

Re: The impact of file position on code review

#33

Post by lmm »

devnullbrain wrote: post #27
jsbg wrote: post #14

> 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 s…

Read this as though it has a visibly bulging temple vein: Does that mean you do the thing where I respond to your review, request a re-review, then get comments about code that was already there a week ago?!

If your PR had lots of stuff that needs comments then I don't think you can reasonably expect me to check it fully in the first pass. E.g. if your first 3 functions are indented wrong, I'm going to write that they're indented wrong and not bother reading the rest of your PR until you've fixed that.

Re: The impact of file position on code review

#34

Post by ColonelPhantom »

FajitaNachos wrote: post #22
progbits wrote: post #17

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.

I would bet the vast majority of people do this. If they didn't, or preferred something else, it wouldn't be presented in the UI this way.

I disagree, because there is no 'easy' way to know what the most 'significant' changes are. I can think of a few heuristics (e.g. most changed, earliest changed, as well as prioritizing things like header files for C and friends), but nothing that would work universally or particularly well.

Re: The impact of file position on code review

#35

Post by Brajeshwar »

This is wholesome. Now, I’m happy that there is a good reason for the way I name and structure files/folders, especially when working in a team.

I was part of a team that built a large web application with Pocket PC interaction capabilities for clinics and physicians in 2002-2003. I scaffolded the front-end, worked on it, while tinkering with the back-end (worked with a brilliant programmer), and built the entire Pocket PC App.

I met the team maintaining the application system after 10+ years around 2015-2016. The lead, who was once the junior we hired, talked about the naming convention and organization and how they followed it for a decade plus. I felt happy and proud of my work.

Re: The impact of file position on code review

#36

Post by progbits »

ColonelPhantom wrote: post #34
FajitaNachos wrote: post #22

Earlier quoted context omitted.

I would bet the vast majority of people do this. If they didn't, or preferred something else, it wouldn't be presented in the UI this way.

I disagree, because there is no 'easy' way to know what the most 'significant' changes are. I can think of a few heuristics (e.g. most changed, earliest changed, as well as prioritizing things like header files for C and friends), but nothing that would work universally or particularly well.

Yeah I prefer the review UI to give me alphabetical order just so I can easily navigate it, but I won't read it in that order.

I think the google review tool (critique) would sort C/C++ header files before implementation, even though ".h" comes after ".cc". That's a nice convenience but it's still easy to navigate.

Re: The impact of file position on code review

#37

Post by Our_Benefactors »

lmm wrote: post #33
devnullbrain wrote: post #27

Earlier quoted context omitted.

Read this as though it has a visibly bulging temple vein: Does that mean you do the thing where I respond to your review, request a re-review, then get comments about code that was already there a week ago?!

If your PR had lots of stuff that needs comments then I don't think you can reasonably expect me to check it fully in the first pass. E.g. if your first 3 functions are indented wrong, I'm going to write that they're indented wrong and not bother reading the rest of your PR until you've fixed that.

> E.g. if your first 3 functions are indented wrong, I'm going to write that they're indented wrong and not bother reading the rest of your PR until you've fixed that.

Just add a linter and precommit hook, it’s not a good use of anyone’s time to go back and forth over trivialities.

Re: The impact of file position on code review

#38

Post by lmm »

Our_Benefactors wrote: post #37
lmm wrote: post #33

Earlier quoted context omitted.

If your PR had lots of stuff that needs comments then I don't think you can reasonably expect me to check it fully in the first pass. E.g. if your first 3 functions are indented wrong, I'm going to write that they're indented wrong and not bother reading the rest of your PR until you've fixed that.

> E.g. if your first 3 functions are indented wrong, I'm going to write that they're indented wrong and not bother reading the rest of your PR until you've fixed that. Just add a linter and precommit hook, it’s not a good use of anyone’s time to go back and forth over trivialities.

I know, just using it as a clear example. Point is if your PR has a lot of low-level issues then I'm not going to start on the higher level review until you've fixed them.
Post reply on HN