21–30 of 33 posts
One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.
One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.
This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.
My team has benefited significantly from diligently reviewing pull requests. It may seem like a waste of time at first, but I can't count how many bugs we've caught this way. Team productivity is also increased due to knowledge-sharing, as we have to understand more parts of the code in order to properly review it. Reviews typically take between 10 mins and 4 hours, depending on the size of the PR. We've also improve…
If there are multiple releases/branches then I also recommend to very carefully review merges between branches, even files/regions where git (or whatever SCM is used) merged automatically with no conflicts. I've personally seen multiple incidents now were through review bugs and security issues where found in merges that ranged from subtle to catastrophic. I even saw one catastrophic security issue introduced into a…
One of the problems we have faced when multiple engineers ding exactly similar work (e.g. Create and Update operations of a CRUD module) but in a very different class and method structures. Now whoever makes it to master first becomes the standard and others needs to modify to confirm to that style. However everyone's style is correct in some sense but reviewer needs to make sure all code is written by one team not t…
Not that there isn't value in having everyone follow the same patterns, but I find it gets emphasized over just getting work done a lot of the time. I've seen projects waste so much effort making the code uniform instead of delivering features. If the code works and is readable, who cares if it varies somewhat from similar code.
This one is usually an automated task. Everyone agrees on a coding style, configures his/her development environment and first time you work on a non-std file you convert and commit it (most things are done by your editor/IDE, if configured correctly).
> If the code works and is readable, who cares if it varies somewhat from similar code.
Me. I regularly have to clean up after lone warriors who talk like you have "delivered features".
A coding style does not represent some universal truth about code esthetics: On one hand it allows programmers to read code with a well defined set of expectations, on the other hand it helps prevent some of the big annoyances for reviewers.
The point is not to like some specific style but to adhere to it. It takes you 5 minutes to adapt your own code to the project's coding style, it will take others 10 minutes to do so and, unfortunately the most common case, it will take hours over a single year when nobody fixes your coding style and everybody who has to touch your code trips over the same things.
Earlier quoted context omitted.
This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.
What I dream of implementing at work (once our infrastructure is more fully containerized) is an addition to the CI/CD pipeline where each Pull Request doesn't just kick off tests but also spins up a full staging environment of its own, just waiting for reviewers/designers/PMs a click away. If there were no resource constraints you'd get a separate one for each commit/push so you could compare UX from individual chan…
Check it out: https://runnable.com/ or contact me if you have any questions.
Earlier quoted context omitted.
This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.
What I dream of implementing at work (once our infrastructure is more fully containerized) is an addition to the CI/CD pipeline where each Pull Request doesn't just kick off tests but also spins up a full staging environment of its own, just waiting for reviewers/designers/PMs a click away. If there were no resource constraints you'd get a separate one for each commit/push so you could compare UX from individual chan…
One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.
Ideally you'd have automated tests that must pass before the PR is mergeable.
One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.
This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.
About 4. Style over substance : Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets: void myMethod(FooBar arg) { if (arg != null) { arg.foo(something); for (int i = 0; i vs void myMethod(FooBar arg) { if (arg == null) return; arg.foo(something); arg.bars().forEach(baz); } Even though th…
Earlier quoted context omitted.
This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.
Rather than a demo, we often ask for screenshots or gifs, which has had similar effect of catching some things earlier.