Clean Code – Contribution, Discussion, Review and Decision Process
Clean Code – From Contribution to Decision
Clean code is a term used to describe software that is easy to read, understand, maintain and test. With the adoption of the clean code style guide by thousands of teams, changes to the style guide can have a large potential impact on developers (e.g. through new static code checks for new rules). While there is usually huge agreement for most of the style guide, there can also be very controversial conversations regarding some rules.
To improve the quality of the decision, the involvement of the interested engineers and be able to make decisions and keep pace with recent language developments, we need to improve the process for collaboration, moderation and decision making. In this blog I want to explain the current draft process for contribution, discussion, review and decision making. This process became necessary to deal with the increasing amount of feedback and change proposals. Probably we will make the process also part of the style guide repository later this year.
If you do not miss an update on clean code, test automation, communities of practice, decision making, testability and other engineering / craftsmanship / architecture topics, subscribe to the brand new newsletter.
Clean Code – Contribution, Discussion, Review and Decision Process
With the success of Clean ABAP and Clean SAPUI5 there comes the need for a more formal process when it comes to reviewing issues and improvement proposals. Clarity on how those should be opened, are processed and the typical timeframe. Besides, especially for changes with a larger impact (e.g. incompatible changes) we need a formal process for decision making.
Principles for Decision Making
The style guides are based on some universal principles. So when we make the decision between different alternatives we keep those principles in mind:
- Testability: Every piece of code needs to be testable in an efficient, fast and robust way
- Readability and Maintainability is important and determines developer productivity
- Studies found that developers spend about 60% of their development time on program comprehension activities and at least 10 times the amount of time is used for reading compared to writing the same piece of code.
- Readability and Modularity over premature performance optimization, but performance is important (e.g. mass read over single read)
- Tradeoffs: If we cannot come up with a consensus, we list the tradeoffs and explain the consequences of the alternatives.
Review and Discussion
Compared to an open source software project, the rate of change for a style guide should be much slower. Due to the adoption by many projects and usage by S4HANA as official programming guideline changes can have an impact on many projects and can result in an adjustment of code check profiles. Therefore, we want to give the readers and contributors enough time to have a conversation on the issues and pull requests.
When moderating the discussion and more clarity is needed for the discussed topic you should consider the following. Discussions are unlikely to give a clear picture of the community as a whole for several reasons. They favor people who like to argue, those who are more articulate or have more experience in English (or whatever language the discussion is in), and those who feel like they’re in the “in” crowd and will have a popular opinion. Finally, they hide how many people are in agreement with someone who speaks up. With this in mind, use the discussion to explore different sides of the issue. Raise awareness of the discussion by announcing in the respective channels (if appropriate) to make sure that the entire community is able to weigh in.
For deciding how to proceed with a pull request, we want to classify the different issues and pull requests into categories. Therefore, we have the following labels:
- Type of Change
- Clarity of Text, e.g. misunderstanding in text
Formatting, e.g. missing
.in code, or wrong “,” in text
- Content Error, e.g. bad link,
me->where it doesn’t belong (= consistency issue)
- New Rule, e.g. add a new rule
- Adjustment of Rule, e.g. change content of rule, such as generalizing a rule
- Idea – Usually an issue e.g. request to add a guide for clean EML
- Ready for Decision
We use these labels to organize our collaboration. Externals cannot add/remove them.
Review and Decision Process
Review by Maintainers and Owners
Every team member with triage rights can approve pull requests or request changes. Only Code Owners can merge the pull request. To join the team of clean code moderators, you can join the respective group:
Meetings: For discussing changes and other issues the Clean ABAP review and discussion moderators meet every second week. To ensure efficient processing an issue or pull request is presented by an advocate with a proposal how to proceed with the issue. Most of the issues and pull requests can be closed or merged within this meeting, but for reviewing changes with impact (e.g. new rules or adjustment of rules) there is an additional meeting with a larger amount of participants:
- Once every quarter, the agile software engineering circle with members from application development, tools and abap meet to discuss about important changes
- An advocate presents the change to the circle
- In order to ensure efficient decision making the following checklist needs to be fulfilled to bring the issue or pull request to the circle:
- The issue or pull request is older than 3 month so that the external and internal engineers had time for a conversation
- The external and internal circle expressed their opinion
For more details on process for important changes see the chapter on decision making for important changes.
Action by Author and Request for Changes
When asked to provide feedback or the pull request is set to request for changes, the author has 2 month time. After the 2 month, the issue or pull request can be closed or the code owner take over the necessary steps to include the proposal into the guide.
An advocate takes the ownership to drive the process for a specific issue or pull request towards closing an issue / pull request or merging a pull request and represent the topic in the different meetings. When picking up the topic, an advocate assigns himself to the issue or pull request.
Decision Board for ABAP
The decision board should represent the diversity of business applications and technology:
- ASE Application Development – 6 colleagues representing large business application areas
- ABAP group – 3 colleagues representing areas of the ABAP foundation
Everyone can participate and express his opinion, but only the decision board members have the power to block a change.
Decision Making for important changes
In the Clean Code Projects a discussion toward a decision follows this process. For important decisions about new rules or change of existing rules (labeled as “New Rule” or “Change of rule”), we strive for consensus among the decision board members. It only becomes a consensus when at least 2/3 of the members of the decision board – i.e. 6 or more of the 9 people in the decision board – have given a +1 and no additional member voted with -1. Therefore, we ask whether all those entitled to vote agree and aim for a consensus. Consensus means that no one has an objection, that a person responsibly shares the decision, if he decides not to veto that for him the decision is ok. So consensus looks at the veto, at the opposition.
- A proposal is put forth and a check for consensus is made.
- Consensus is signified through a +1 vote.
- A check is made for any dissent on the proposal.
- Reservations about the proposal are worked through, seeking consensus to resolve the reservations.
- A reservation is not a vote against the proposal, but may turn into a vote against if unresolved. It is often expressed with an initial -1 vote to indicate reservations and concerns. This indicates there is still discussion to be had.
- Stand aside? No comment, or state concerns without a -1 reservation
- This option allows a member to have issues with the proposal without choosing to block the proposal, by instead standing aside with a +/-0 vote.
- The stated concerns may influence other people to have or release reservations.
- Block? Vote -1 with reasons for the block.
- This is a complete block on a proposal, refusing to let it pass. A block is a -1 vote and must be accompanied with substantive arguments that are rooted in the criteria of the project.
Block (-1) votes used as a veto are typically used only when consensus cannot otherwise be met, and are effectively a veto that any sitting Board member can utilize with sufficient substantiation. A block should only happen if there are very strong concerns about the proposal (e.g. creates potential security problem or severe performance degradation).
A way to deal with reservations, concerns and vetos is to document the tradeoffs for the different alternatives.
In Github, this process is represented as follows: Any important change is represented by a pull request. The votes in the above description correspond to reviews of this pull request, with:
- +1 votes corresponding to reviews that approve the changes
- -1 votes (both blocks and reservations) corresponding to reviews that request changes
- Standing aside or concerns without reservation corresponding to comments or commenting reviews
Notify Code Owners about the need for decision making
If there is a pull request, which needs the decision from the code owners, write a short comment in the discussion section of the code owner team.
Code Owners put the decision of the group into practice and can merge smaller changes without going through the decision making process. Every code owner commits to follow the clean code process description and to agile software engineering principles.
Code Owner Teams:
Merge of Pull Request
Pull Request are merged with squash and merge:
- Example: Commit Message
- title: recommend avoiding manual versioning through comments
- description: #278, no vetos, 7 approvals
Process after Important Changes
Change Log: The new rules or adjusted rules can be found in the change log
The *Code Pal+ team is watching the merged pull request and elaborates if static code check adjustments are needed.
Update of the translations is not directly triggered after the change is merged. The translators are asked for watching the changes and update the translation from time.
How are new language features considered for an update of Clean ABAP?
Several members of the clean abap team are part of the ABAP team and also involved when rolling out those new ABAP features. These colleagues will create a pull request to update Clean ABAP if necessary.
Subscribe to Newsletter to stay informed
If you do not want to miss an update on clean code, test automation, code reviews, unit testing, decision making, testability and other engineering / craftsmanship / architecture topics, subscribe to the brand new newsletter. The newsletter will not only be used for sharing knowledge, but also offer opportunities for collaboration, building communities and co-creation.
To be honest, it felt like the project was slowly being abandoned by SAP due to SAP's low involvement in the discussions. Thanks for the transparency on the new process. I'm looking forward to future updates on the styleguide! The styleguide is a very helpful resource for me and my colleagues.
I hope this process will also move some of the long-discussed issues to a decision soon, like for example the discussion on preferring field-symbols versus references.
Thanks for the feedback!
We focused so far more on the open Pull Requests, since those came down. We can now think more about handling the open issues.
Very long post... Sorry, didn't have time (or interest, tbh) to read every word. I'm interested in what / how long does it take from the time an issue is opened for Clean ABAP in GitHub to the time when the repository owners react to it and either say "nah" or update the guide.
Here is what I'm observing. I was looking for this type of information in the repo itself. There is a comment saying the guides are updated "continuously" but it's very vague (once a year is also "continuously") and based on the information in this post seems inaccurate. I would suggest to add a TLDR version of this post on GitHub itself, otherwise this process is not clear at all to the contributors. Actually it's still not clear even after browsing through this post but it's the next subject.
It says some group meets every other week and then another group meets every quarter. OK, so to me this looks like at least every 3 months there should be some sort of movement. But this one is open from August 2022 and this one from May 2022. The one in Benjamin's comment has been "new issue" since 2019! There is no reaction from SAP / guide owners. Why?
Maybe it's buried in the text but I don't really understand what do these two groups do when they meet. What are they looking at? By comparison (and I can't believe I'm saying this), the process on Influence website seems much more transparent. There is X number of votes needed for someone to look at the improvement request. There are no votes on GitHub afaik so what is the criteria? How do you make decisions? Why do you not comment on long-standing issues?
As Benjamin Giampapa correctly noted, exactly for these reasons (no reaction from SAP, no activity in the comments) the project does look abandoned. And after reading this post, I wonder if now SAP plans to unleash the bureaucracy machinery so that it dies of "natural causes"...
Probably it is better to have a meeting. What is the best way to exchange contact information?
One of the reason for the process, was to increase was to increase the confidence in making decisions by representing different groups and set it up more sustainable (e.g. independent from the original creators).
The process will be moved to the GitHub when it is finalized. In the internal version of this document, there is also some SAP internal information.
The first priority for the meeting is to go open through open Pull Requests. The discussion about every single PR takes some time. So we did not have so much time yet, to discuss the issues.
The main criterias for the decisions are in the principle section. If further general criteria cristialize those will be documented.
The idea of leveraging something like voting to prioritize issues makes sense. GitHub offers that feature already: https://github.com/SAP/styleguides/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc
I have open DMs on Twitter and my email is in the profile description. Just want to make clear though that I can only speak for myself. You might want to reach out to other developers too who are still part of SAP Mentors.
I have sent you a meeting request.
Another thing, I am unable to access the links to Clean ABAP review and discussion moderators and Clean ABAP Code Owner. Are they only available at SAP internally?
Those are also stored on github.com. Are you logged in with your GitHub user?
Could be that GitHub teams in the SAP organization are only visible to GitHub users, which are part of the SAP organization.
Yes, I am logged into my GitHub user and I would agree with your assumption.
I got a notification about a comment, but it seems to be gone.
Did not find a way to change the visibility of the GitHub team.
I am thinking about setting up a joined meeting committers and motivated colleagues from the external community in Q2. If you are interested, you can write me an email with a few sentence why you would like to participate. Currently that is only an idea - need to check for the details.