4 ms·

I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent. Finding out that solution is subpar at thi

I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent. Finding out that solution is subpar at this stage is costly, and delaying integration makes ineffective teams. If you want to enforce standards, automate it (there are multiple highly configurable tools for that). If you want good solutions, pair program. If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor. Use TDD / BDD to ensure the app still works.

V-2 5y ago
"I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent." This implies the need for smaller pull requests maybe? If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on. Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late. "If you want to enforce standards, automate it" Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there! However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like. "If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor" Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years? "The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.
- KronisLV 5y ago
  > This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner. Otherwise you end up with something like the following being separate requests: - FEATURE-123 add data models for the feature - FEATURE-123 add service logic for the feature - FEATURE-123 add logging and validations for the feature - FEATURE-123 add unit tests for the feature - FEATURE-123 add REST endpoints for the feature - FEATURE-123 add front end code to forms - FEATURE-123 add front end logic to use the REST endpoints - FEATURE-123 add integration tests for headless browser You don't actually want these changes to end up in the main branch before they are done. Alone, they provide no actual value and could only confuse people. Furthermore, if there are changes that are needed to the data model after it has already been merged, then you'd also need to do those within a later merge request, or one in the middle, which would just pollute the history and serve as spam, instead of being able to add more commits to the other sub request. To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives. EDIT: So, you'd need something like the following: - (WIP, 3/8 reviewed) FEATURE-123 add new feature // feature-123 branch, cannot be merged into main yet, review the below first - (reviewed) add data models for the feature // feature-123-data-models branch, will be merged into previous - (reviewed) add service logic for the feature // feature-123-service-logic branch, will be merged into previous - (reviewed) add logging and validations for the feature // feature-123-logging-and-validations branch, will be merged into previous - (in review, 9/13 comments resolved) add unit tests // feature-123-unit-tests branch, will be merged into previous - (registered) add REST endpoints for the feature // feature-123-rest-endpoints branch, will be merged into previous - (registered) add front end code to forms // feature-123-front-end-code branch, will be merged into previous - (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous - (registered) add integration tests for headless browser // feature-123-integration-tests branch, will be merged into previous (personally i think merges make the most sense, but some people prefer rebasing; it's just an example) You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.
  - V-2 5y ago
    "You don't actually want these changes to end up in the main branch before they are done." True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me. The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).
    - KronisLV 5y ago
      Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it. I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!
      - WorldMaker 5y ago
        I don't have as much experience with GitLab, but I actually think Github has a lot of tools in their PR system today for doing "early" or "work in progress" PRs incrementally. You can start a PR on an empty branch. You can apply a label "Work in Progress" and start discussion immediately. If you review a chunk of code the PR is pretty good about showing you what you've already reviewed versus what is new in the most recent update. If for some reason you feel a need to start over and force push an entirely new branch to the PR, Github's PR system handles it better than you would expect (and auto-closes old comments, etc). I've found the tools are better than people expect them to be and the hard part for me is convincing junior developers that it is "okay" to start PRs "early", that they don't have to feel like they've finished a task to get comments/reviews on work in progress. There's a lot of embarrassment/group politics in the way sometimes from using "PR early" workflows.
        boleary-gl 5y ago
        GitLab employee here - that is true of GitLab too. But I agree the bigger challenge than any tooling challenge is getting the team convinced that starting a PR/MR early is not only okay but also the way to operate. Creating physiological safety around it as well as making iteration a part of the day to day workflow is a challenge but an important one.
frank_bin 5y ago
I tend to agree. I prefer design drafts (documents) instead of pair programming, though.

V-2 5y ago

"I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent." This implies the need for smaller pull requests maybe? If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on. Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late. "If you want to enforce standards, automate it" Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there! However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like. "If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor" Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years? "The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.

KronisLV 5y ago
> This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner. Otherwise you end up with something like the following being separate requests: - FEATURE-123 add data models for the feature - FEATURE-123 add service logic for the feature - FEATURE-123 add logging and validations for the feature - FEATURE-123 add unit tests for the feature - FEATURE-123 add REST endpoints for the feature - FEATURE-123 add front end code to forms - FEATURE-123 add front end logic to use the REST endpoints - FEATURE-123 add integration tests for headless browser You don't actually want these changes to end up in the main branch before they are done. Alone, they provide no actual value and could only confuse people. Furthermore, if there are changes that are needed to the data model after it has already been merged, then you'd also need to do those within a later merge request, or one in the middle, which would just pollute the history and serve as spam, instead of being able to add more commits to the other sub request. To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives. EDIT: So, you'd need something like the following: - (WIP, 3/8 reviewed) FEATURE-123 add new feature // feature-123 branch, cannot be merged into main yet, review the below first - (reviewed) add data models for the feature // feature-123-data-models branch, will be merged into previous - (reviewed) add service logic for the feature // feature-123-service-logic branch, will be merged into previous - (reviewed) add logging and validations for the feature // feature-123-logging-and-validations branch, will be merged into previous - (in review, 9/13 comments resolved) add unit tests // feature-123-unit-tests branch, will be merged into previous - (registered) add REST endpoints for the feature // feature-123-rest-endpoints branch, will be merged into previous - (registered) add front end code to forms // feature-123-front-end-code branch, will be merged into previous - (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous - (registered) add integration tests for headless browser // feature-123-integration-tests branch, will be merged into previous (personally i think merges make the most sense, but some people prefer rebasing; it's just an example) You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.
- V-2 5y ago
  "You don't actually want these changes to end up in the main branch before they are done." True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me. The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).
  - KronisLV 5y ago
    Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it. I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!
    - WorldMaker 5y ago
      I don't have as much experience with GitLab, but I actually think Github has a lot of tools in their PR system today for doing "early" or "work in progress" PRs incrementally. You can start a PR on an empty branch. You can apply a label "Work in Progress" and start discussion immediately. If you review a chunk of code the PR is pretty good about showing you what you've already reviewed versus what is new in the most recent update. If for some reason you feel a need to start over and force push an entirely new branch to the PR, Github's PR system handles it better than you would expect (and auto-closes old comments, etc). I've found the tools are better than people expect them to be and the hard part for me is convincing junior developers that it is "okay" to start PRs "early", that they don't have to feel like they've finished a task to get comments/reviews on work in progress. There's a lot of embarrassment/group politics in the way sometimes from using "PR early" workflows.
      - boleary-gl 5y ago
        GitLab employee here - that is true of GitLab too. But I agree the bigger challenge than any tooling challenge is getting the team convinced that starting a PR/MR early is not only okay but also the way to operate. Creating physiological safety around it as well as making iteration a part of the day to day workflow is a challenge but an important one.

frank_bin 5y ago

I tend to agree. I prefer design drafts (documents) instead of pair programming, though.