Live data from Hacker News

Changing how I review code

itnext.io

31–40 of 80 posts

Re: Changing how I review code

#31

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

Just because there are tests doesn't mean the tests are any good. I've only started my quest in TDD and I've discovered it is rather trivial to write tests that don't really mean anything.

I find myself asking "what is this supposed to do?" and "does this actually do what it is supposed to do?". I tend to drift into a design mindset during test writing (more-so than in code review) that highlights data flow and the basic bits of logic. Even then, I find that I can miss test cases rather easily.

Re: Changing how I review code

#32

> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?

You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical the changes are. It amazes me the lackadaisical attitude towards code review. "It will probably be okay" is the worst attitude to have with computer code. It is the most brittle, brutal environment imaginable.

Can you expand on this?

Re: Changing how I review code

#33

Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have falle…

> Not sure why we have fallen so much in love with the idea.

Because organisations obsess about risk minimisation.

Re: Changing how I review code

#34
post #12

Pair programming is sooo much better than PR’s. I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh. Just have two people do the work together, and…

PRs are perfect in open source . In companies they’re just an illusion of minimising risk.

Re: Changing how I review code

#35
post #32

Earlier quoted context omitted.

You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical the changes are. It amazes me the lackadaisical attitude towards code review. "It will probably be okay" is the worst attitude to have with computer code. It is the most brittle, brutal environment imaginable.

Can you expand on this?

You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it's harder to remove properly than it is to add new things. Say they changed the validation on a model: is this going to work well with the production database? Do we need to write a migration to catch the database up to the new reality of the code? Automated tests are just automated warning flags that give you confidence that the codebase is basically hanging together. You still need to be very careful & thoughtful as you're making changes.

I like to actually reset the branch so I can see all the raw changes in my IDE, this helps me to think it through, in git you can do this via:

git pull feature-branch; git checkout develop; git checkout -b my_initials/feature-branch; git merge feature-branch --squash --no-commit

Re: Changing how I review code

#36

I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at…

Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?

Re: Changing how I review code

#37
post #8

Earlier quoted context omitted.

That is what testers are for. Testing is great thing.

/s/testers/users Just kidding and in reality UI tests exists and aren't super difficult to set up. The front end has plenty of tools like Cypress and its pretty simple to automate running a bunch of tests that screenshot and diff compare your site. I never really pulled down UI code to test them out. There has to be a certain level of trust between engineers. Sometimes I'd ask or post screenshots of changes but that…

> Just kidding and in reality UI tests exists and aren't super difficult to set up. The front end has plenty of tools like Cypress and its pretty simple to automate running a bunch of tests that screenshot and diff compare your site.

In a well oiled machine, that's what the QA department does. They have close working relationships with the engineering and customer support departments so they develop testing processes and systems tailored to the "quirks" of engineers and users. They spend extra time on the places they know the engineers have blind spots and work on automated regression tests based on customer support workloads.

Whether they go as low level as unit tests depends on the organization but one place I worked at, the domain was complicated enough that "I see no red squiggles in my IDE, pinky swear" was the threshold to go from engineering to QA. It was an ongoing joke that whenever there was a major dependency update, 10-25% of the work sent to QA that day contained obvious syntax errors and wouldn't compile because the IDE everyone used often took an hour or more to reindex the project without any clear indication that it was in a useless intermediate state. Running the compiler would slow it and the indexer to a crawl, so the engineers just YOLOd it so they could move on to other work, and the QA department set up a static analyzer with an empty rule set on all PRs to catch dumb errors caused by immutable process/purchasing decisions.

IME these kinds of QA departments are the #1 springboard into software engineering roles because they work so closely with engineering and are exposed to code in the form of automated tests.

Re: Changing how I review code

#38

Earlier quoted context omitted.

It sounds cool that you create on-demand review app. In a small scale it works. In a larger scale development is absolute impossible due to resource constraints to create on-demand environment for review app. For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.

I'm not going to dismiss your experience, or your knowledge about the environment you describe, but we are most definitively not doing small scale development. Our company has a large software stack comprised of several backends and frontends, maintained by 8+ teams. I maintain one frontend in this stack, and the review app produced will only contain my version of the front-end, and the API calls will go to the backe…

You are looking at a problem from developer's perspective. From infrastructure perspective, it is a terrible pattern if you own the feature-flow or review app flow. It is really wasteful with no guarantee that when all the features that are written concurrently are merged, it your feature will still work as expected.

Let me put it in another way, would it have been if you would have kept, development, test, staging and production environments up to date all the time and had meaningful integration tests?

Re: Changing how I review code

#39

Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have falle…

That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences?

To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together.

Re: Changing how I review code

#40

Earlier quoted context omitted.

Yeah I’ve always felt that PRs are ineffective. It’s to late a stage in the development process to give feedback.

Code reviews and PRs are different things. Still, PRs can be effective with the right tooling and methodology. We've doing continuous code reviews with PRs and it's been working great to have early feedback. More details at: https://reviewpad.com/blog/continuous-code-reviews-are-the-w...

Sure it is
Post reply on HN