Timeline for How can I defend reducing the strength of code reviews?
Current License: CC BY-SA 4.0
52 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Feb 24, 2021 at 13:46 | comment | added | maple_shaft♦ | Comments are not for extended discussion. Please refer to our chat room for further discussion on this question. | |
| Feb 24, 2021 at 3:01 | history | tweeted | twitter.com/StackSoftEng/status/1364409970452815874 | ||
| Feb 23, 2021 at 3:44 | answer | added | Brandon | timeline score: 2 | |
| Feb 22, 2021 at 20:33 | comment | added | Thomas Matthews | Improve the quality of a code inspection: develop a checklist. The team should develop a list of common issues (such as typos) and use that as the code inspection. The code inspection (with checklist) will still work with TDD. The TDD is not going to catch every issue, but you can improve the quality of each "phase" of TDD by employing a checklist and review. For example before the next refactoring, apply the checklist to the code that is "kept". This can happen before the tests are written. | |
| Feb 22, 2021 at 18:14 | answer | added | Eric Hydrick | timeline score: 1 | |
| Feb 22, 2021 at 16:39 | answer | added | Ian Kemp | timeline score: 1 | |
| Feb 22, 2021 at 6:57 | comment | added | J Trana | Regarding big picture: I like to put out an incomplete draft of the code and ask for comments on the flow. Usually I like to put out an outline/draft of the code and a list of test stubs. I've found this works well psychologically: everybody knows it's incomplete, so it keeps focus on the big picture. For the team, that means they can feel they are part of the solution and provide high-level feedback - increased ownership. For me, I haven't turned the code into a work of art so I have less ego and attachment to it. It might be a way you can lead by example without starting a fight. | |
| Feb 21, 2021 at 19:46 | comment | added | Eelco Hoogendoorn | What ive settled on as a team-lead is to try and make a list of rules about what is valid as a suggestion and what is valid as a blocking request. For instance, concrete calls for clarification or documentation can never be ignored. Nor bugs obviously. On the other hand; humans are not linters and if you cant express it as an automatically enforced rule then we dont want to hear about it in code review. Furthermore there is some soft guidance, like 'defer matters of taste to the developer with greater ownership of the codebase'. Absolutely +1 for pushing back against the fashionable pedantry. | |
| Feb 21, 2021 at 17:09 | comment | added | Andrew Henle | The code review should be a recommendation That attitude is a huge red flag to me. In my experience leading teams, push back on code review is highly correlated with developers who produce, umm, lesser-quality code, shall we say. I see that attitude and the first thing I think of is, "Why don't you want anyone reviewing your code?" Given that any code review only has to find one small bug to pay for itself many times over (do the math on how much bugs cost to fix - and a "senior" developer should know that...), your attitude towards code reviews is more than a little concerning. | |
| Feb 21, 2021 at 10:54 | answer | added | Toby Speight | timeline score: 4 | |
| Feb 21, 2021 at 10:37 | comment | added | marshal craft | Some times policies are designed to prevent an issue. The way the policy works sometimes can be quite complicated how it works, and comes from experience. There is no general simple theory behind it. Not saying that is the case. But sometimes you should respect things even if it's not clear what they actually do to improve code. They can seem like waisted work etc. Just saying don't assume a convention is pointless immedietly. | |
| Feb 21, 2021 at 5:00 | comment | added | RonJohn | "when requests are done, the team does not focus on what is IMHO important ... but on small details." That's bikeshedding. en.wikipedia.org/wiki/Law_of_triviality | |
| Feb 20, 2021 at 23:46 | comment | added | Pesho | It is a little bit funny that just 2 years ago I have actualy asked the opposite question than the one I am asking now. I just realized that fact workplace.stackexchange.com/questions/121672/… | |
| S Feb 20, 2021 at 20:56 | history | suggested | Peter Mortensen | CC BY-SA 4.0 |
Copy edited (e.g. ref. <https://en.wiktionary.org/wiki/no_one#Pronoun>, <https://en.wiktionary.org/wiki/concious>, <https://en.wiktionary.org/wiki/grownup#Noun>, and <https://en.wiktionary.org/wiki/let%27s#Etymology>).
|
| Feb 20, 2021 at 20:49 | history | protected | gnat | ||
| Feb 20, 2021 at 20:41 | review | Suggested edits | |||
| S Feb 20, 2021 at 20:56 | |||||
| Feb 20, 2021 at 19:41 | history | edited | Pesho | CC BY-SA 4.0 |
added 285 characters in body
|
| Feb 20, 2021 at 19:29 | comment | added | Pesho | @DanielR.Collins I re-read my original answer. I have answered you even in the first comment. Sory if there were too many details hiding it. I am quoting : "Obviously that is some sort of convention accepted by the team." | |
| Feb 20, 2021 at 19:23 | comment | added | Pesho | @DanielR.Collins yes , the code has many statuses. Unfortunatly instead of doing some modeling and dividing the things in own classes they relay on methods. So one domain object may have 30 methods and then this methods are based on statuses and then these statuses have some chronological meaning in time. One may be executed after another, so the result is that they have accepted a convention to order them chronologicaly , not only logicaly. | |
| Feb 20, 2021 at 19:14 | answer | added | Greg Burghardt | timeline score: 14 | |
| Feb 20, 2021 at 18:00 | comment | added | Daniel R. Collins | Your comments don't answer my "yes or no" question. If you think what you wrote is an important addition to the question, then you should edit it to include them. | |
| Feb 20, 2021 at 17:55 | comment | added | Pesho | @DanielR.Collins I am a consultant for 20 years I have switched many teams, many projects. My job is to be adaptable. I have never experience a team with so many rules. | |
| Feb 20, 2021 at 17:54 | comment | added | Pesho | @DanielR.Collins continuation from previous comment Obviously if a requirement is to also reflect the chronology f.ex. Create comes before delete. But then lets say you have Ignore and Cancel which one would you logicaly put first ? You see the catch ? Maybe for some method you can clearly define it for others you cant The problem is about the number of different conventions you need to follow. | |
| Feb 20, 2021 at 17:53 | comment | added | Pesho | @DanielR.Collins Let me put things a bit in perspective. In general I dont mind so much puting the methods is chronological order. Onviously that is some sort of convention accepted by the team. The problem is not so much about me accepting the conventions. It is about the sole number of all sort of conventions many of which have cosmetic charactor. There are different ways of grouping methods. Example can be first public then private and so on... Another way would be grouping them by purpose. | |
| Feb 20, 2021 at 17:49 | comment | added | Daniel R. Collins | Re: "But then if you actually require that the methods must be ordered by their chronological execution, that goes a bit further than the general rule." To be clear, is that a custom on this team? | |
| Feb 20, 2021 at 17:29 | answer | added | Ilmari Karonen | timeline score: 11 | |
| Feb 20, 2021 at 17:25 | comment | added | Robert Harvey | @GarethRees: You haven't addressed the problem of whether those comments have any authoritative weight. Can you just ignore all of the comments and check the code in as-is, or does someone else have to approve your fixes? | |
| Feb 20, 2021 at 17:21 | comment | added | Gareth Rees | "Tens of comments" is nothing to be concerned about — I'm looking at a pull request here with 500 comments! (Only about 50 of them mine.) Fix the comments (or don't fix them if they don't add value), thank the reviewers for their work, and move on to the next bit of work. | |
| Feb 20, 2021 at 16:34 | answer | added | Diane M | timeline score: 11 | |
| Feb 20, 2021 at 14:28 | comment | added | Sam | You seem to disagree with the team's style and rather than working with them on an agreement, you want to do things on your own. That's hardly a good idea. | |
| Feb 20, 2021 at 13:38 | comment | added | anotherdave | Not an answer, but more of an aside — if people are focusing on nit-picky details, you should automate them away. Enforce that everything passes checkstyle/linting in a pre-push hook; also reformat code hierarchy in a pre-push hook to be e.g. depth-first. You may have to run reformatting across the entire codebase, once, but then you shouldn't ever have arguments about these things. If someone raises a cosmestic code formatting issue in a PR, it should entire a) get agreement across the team (and be added to checkstyle) or b) safely ignored. | |
| Feb 20, 2021 at 12:43 | comment | added | noughtnaut | "The code review should be a recommendation , but not mandatory" - Depends on the business sector; where I work 100% review coverage and separation of duties is most definitely mandatory. | |
| Feb 20, 2021 at 12:13 | comment | added | alephzero | If code review is only a "recommendation", you could save even more time by not doing it at all. | |
| Feb 20, 2021 at 9:29 | answer | added | BlueRaja - Danny Pflughoeft | timeline score: 6 | |
| Feb 20, 2021 at 8:13 | history | became hot network question | |||
| Feb 20, 2021 at 6:42 | comment | added | gnat | see How do I explain ${something} to ${someone}? | |
| Feb 20, 2021 at 6:26 | comment | added | Theraot | The reason to move methods to another class should not be the number. A rule about ordering logically related methods together should not have come up if non-logically related methods don't share file. So, at first glace, split the class in multiple files. Then there is the question of single responsibility principle. A class with methods that are not logically related needs a SRP review. This also suggests that the design process (which happens to be TDD) is not taking care of that (which is not what I'd expect of "religious fundamentalism" TDD). Is nobody responsible for the design? | |
| Feb 20, 2021 at 3:48 | answer | added | jmoreno | timeline score: 0 | |
| Feb 19, 2021 at 23:32 | history | edited | Doc Brown | CC BY-SA 4.0 |
tried to improve spelling, grammar and wording
|
| Feb 19, 2021 at 23:02 | answer | added | gnasher729 | timeline score: 7 | |
| Feb 19, 2021 at 22:22 | answer | added | Telastyn | timeline score: 3 | |
| Feb 19, 2021 at 21:56 | answer | added | Philip Kendall | timeline score: 182 | |
| Feb 19, 2021 at 21:51 | answer | added | LeanMan | timeline score: 4 | |
| Feb 19, 2021 at 21:32 | answer | added | Erik Eidt | timeline score: 3 | |
| Feb 19, 2021 at 21:31 | answer | added | candied_orange | timeline score: 16 | |
| Feb 19, 2021 at 21:20 | answer | added | FrustratedWithFormsDesigner | timeline score: 9 | |
| Feb 19, 2021 at 21:20 | history | edited | Pesho | CC BY-SA 4.0 |
added 8 characters in body
|
| Feb 19, 2021 at 21:07 | review | Close votes | |||
| Feb 24, 2021 at 3:02 | |||||
| Feb 19, 2021 at 21:06 | answer | added | Robert Harvey | timeline score: 39 | |
| Feb 19, 2021 at 20:54 | history | undeleted | Pesho | ||
| Feb 19, 2021 at 20:54 | history | deleted | Pesho | via Vote | |
| Feb 19, 2021 at 20:49 | history | asked | Pesho | CC BY-SA 4.0 |