Code review: bad advice for contributor and reviewer
Hello! My name is Nikolai Izhikov. In this post I want to talk about one important element of the interaction that we encounter in the software development process, especially in open source. This passage and conduct code review.
I will givebad advice on how to make my feature and bring it to merge in the opensource project wizard in terms of cultural, temporal, conceptual and other differences between community members. This is usually a delicate question, especially for those who are just starting to work in open source.
Let's start with a little introductory. When communicating with people, especially in chat or by mail, especially in English, always keep in mind that your message may be perceived in a completely different way than you intended. Who will read the letter is unknown. He can be a Hindu, an Englishman, or just a sleepy man. There will always be differences in the understanding of concrete words, and without going into technical details, I will tell you how to minimize them.
Warning: the story contains sarcasm.
')
If you want to implement a new feature, in no case do not discuss your revision with anyone. Someone can overhear and do it before you! Or, having heard the details, they can tell you that you did not understand something — to criticize your idea. Criticism hurts a lot. Perhaps such a feature as yours has already been discussed, and another one is needed ... In general, in no case tell anyone what you want to do. Never.
Committers and experienced guys from the community love to understand the code of the received patch. Therefore, in any case, do not do the technical documentation. Wiki, Jira, Mail list discussion - all this is nonsense. And nonsense is not for you!
When you send a patch to [+5000, -3500] with a description of the improvements in the form of “Factory methods and all other enhancements”, everyone will figure it out for themselves, and at the same time they will understand how well done you are.
Any tests are dangerous because they can break something. If you still managed to run the tests, do not show their results. Errors in the results can lead to patch rejection! And just never write new tests. Run All will be longer, consumption of processor time will increase. Anyway, good developers write code without bugs - any experienced colleague will look into your patches and understand that there are no errors.
Coming to an open source project, never read any how-to. They write for fools, right?
Make out the code in your own way. Otherwise, how will everyone understand that you are a good developer and a versatile creative person with a developed sense of beauty?
Send patches at your convenience. For example, the project is tied to the GitHub infrastructure - send it as you send to the linux kernel, right in the letter. You can not even in the attachment, but right in the text. After all, Linus Torvalds bad advise! And if the project is taken differently, then this is the problem of the project.
When developing a new Public API, try to make it as abstract and complex as possible. You can always answer any questions from users about unobvious API behavior: “Didn't you read the manual? Page 42, everything is clearly written there! Read again! In serious projects, everything should be difficult. Do we have a serious project?
Do not advise anyone and do not tell anyone about the problems you faced in the development. Much more fun to understand all alone. Anyway, good developers always get everything right the first time, they have no problems. If others find out about your problems, they will become smarter and write code faster than you. This should not be allowed. And generally speaking about their problems is not accepted.
On the limitations of the final decision is also not worth mentioning. After all, the solution may be asked to modify. Who is interested in hearing about restrictions? When the user starts to introduce your product into the product and faces restrictions, it will be more interesting and more fun for him. He will come to you and ask you to finish. Until this point, in no case tell him about the restrictions - what if he decides not to implement anything?
A really good reviewer will find everything and ask you details. In no case do not tell him about anything.
This tip complements the previous one. It is best not only not to tell anyone anything, but also to ask any questions primarily on the dev-list.
Here are examples of questions that everyone likes to answer. If you write some kind of test, be sure to ask, "Do you need a check on the null of this collection?". You don’t have to figure it out yourself, you can always ask on the dev-list. After all, there are many people who are just waiting to be asked such a question. They will seek to answer it faster.
“How do I do the task?” Is another great question. In no case do not provide any details: the ID of the task will be sufficient. All who need to see for yourself.
“What is the framework to use?” Is also a very good question. It is always interesting to answer it, and it is possible to debate.
This is my favorite tip. Fix all project problems in one pull request, especially if you work in the enterprise. Before you in the project were some fools, they wrote the code badly. And you write well. Accordingly, you simply must:
And in general, you can take and render some classes, factories, etc., do other transformations to make the code better. Especially effectively it will look in areas that are not relevant to your task. So you can more fully reach your potential. In glory OOP! Amen!
When you do a task, and then send a request for code review, the process can be delayed. All experts are usually busy. Use the trick: when your task, as it seems to you, will end soon, request a review in advance. After all, they will not look right away - until they reach you, you can just finish everything.
Well, if the reviewer suddenly has time, while the task is not completed yet, then he is not lucky. Explain the situation, say that tomorrow everything will be ready. This way you speed up the process (at least in a review) and achieve a merge faster.
Everyone who works in open source has a lot of time. No need to finish. Passed code review, received comments. And why do something to rule and bring to the merge? Take the next puzzle and do. This is the way to success! The reviewer has a lot of time, and will look at the next patch without a result!
How else to call the person who stands between you and the commit in master? Criticism of the code is a criticism of you personally! Who does he think he is? In personal communication, "bomb" as much as possible! Otherwise, how does the reviewer know that you care? This is the basic rule of development!
I recommend this advice to apply in daily development. It helps to achieve results and do it right.
If you have reached the level of the project, when patches are already being sent to you for review, do not in any way automate routine checks! It is much more fun to spend a couple of review cycles on troubleshooting and discussion:
Routine checks in any case do not need to automate. Yes, and tests to drive to anything.
To be ahead, you always need to keep a pair of aces in the sleeve. Don't tell anyone about the need for backward compatibility. Better just before the commit, say: “Here, according to our rules, we need to ensure backward compatibility. Please correct. This will be especially impressive if you have already done five times. On the sixth one can still say: “I am not an expert, so I have to wait for the review of Mr. X, who will look again.”
Such comments, especially at the final review, always add motivation to contributors.
This tip overlaps with the last tip of the contributor. Write comments to pull request as emotionally as possible. If somewhere is not checked for null or the variable is not so named, add passion to your comments. Let them see that you care.
If there is any dispute, in any case do not provide technical arguments. With technical arguments it may turn out that you are wrong. Referring to the authorities - this is the best argument in the dispute, you will always win.
If someone still passed your review, in no case can not say "thank you". Yet they want to commit to open source! In no case do not need to thank, and so they will come again!
I hope everyone understood how to conduct and review? In each of these tips, besides sarcasm, there is pain, which is often found in practice.
I will be the captain of Evidence and tell you what you really need to think about when preparing and conducting a code review. The considerations further apply both to the one that develops the feature and to the one that will be reviewing it. Still, these are two sides of the same coin.
A consensus in communication, firstly, is achievable, and secondly, it is necessary for the project to move forward. Currently, few products can be developed alone. This is usually teamwork.
This is the main advice. Use common sense, he steers. I think this advice applies to all life situations. If you are doing something, consider whether it meets the simple rule of common sense?
This is about culture. I was in a few large open source community. I do not know whether this is part of the Russian mentality, but often a person who can be perceived as a boss is also perceived as a person who fell into his place by accident. It is believed that he wants you bad, by default, there are doubts about his professionalism. Therefore, it is very important in any work to assume, even for a second, that:
The review is not only ready-made patches, but also improvements to the project, fixes. In essence, code review starts at the moment when you are just discussing your revision. At this point, ask yourself: what value does refinement bring to the product?
There are other options. In any case, starting to develop something, solve a problem, you must understand what value you add to the project.
There are a number of helpful questions worth asking.
Why make a feature? Why is this feature needed? The answer to this question is important.
Where is the start of work? In my practice, it happened that I was asked to remake a certain application. There is an application A, you need to make an application B, which does almost the same with minor changes. I begin to do the work and it turns out that, in principle, it does not work. In fact, it is used somewhere in the production with the help of a human-machine interface - that is, someone is sitting and constantly restarting the program, correcting the null pointer exception literally on the air. Where is the start of work? In this case, it will be in the correction of program A, so that it works stably, then in the writing of program B.
Where is the complete end of work? How should the work done ideally look like? It is important to formulate from the very beginning where you are going.
Where is the end of the current stage? It is clear that the elephant does not immediately eat and the work is better divided into stages. It is important to understand where the end of the current stage. Often projects swell and do not end simply because the scope of the project becomes infinitely large.
Why is the feature broken into such stages? This is about MVP and all that. Please think about it too.
On the properties of the Public API there are many articles. Read before implementing it. As a good example I can cite the jQuery framework, Spring in Java. There are negative examples. Whoever programs Java for the first year probably remembers just terrible from the point of view of Public API EJB 2.1. Maybe the functionality is not bad there, but if the Public API is bad, no one can convince users to use the product.
Public API is not only a tool for third-party users. This is an internal component API that you reuse amongst yourself. Important properties of the Public API:
When you come to code review, it is important to keep in your head a complete list of systems and subsystems of a large product, where changes take place. In enterprise projects, it may not be clear: whether it is a database schema, or a controller, or a view, some kind of reporting system, uploading, loading, etc.
When working with a boxed product, it is important to ask yourself the question: how do changes affect existing processes in the system? Is there backward compatibility? Does it affect performance? If it affects, then the performance of what? How does this affect the user experience?
Each business system has standard processes: start, installation, a list of operations. How will they proceed now? Before code review it is important to understand this. You have to go through exactly the code that implements these processes. In the case of Ignite, this is:
It is clear that the set of these processes is quite large. It is important to understand that such processes exist and how they change.
In your business application, disaster recovery, initial system initialization, node shutdown, restart, update of your application, and the like can happen. In the case of Ignite, we have the following corner cases:
These things we need to check and check that everything is in order.
So we got to the code. I advise you not to be lazy and seek in it:
Java has its own peculiarities when writing concurrency code. If you have a business system and there is not enough concurrency there, you do not need to take these features into account. However, usually some synchronization passes through the database. In things like Ignite, it's a bit more complicated. And here not only the functionality of the code is important, but also its properties:
These questions need to be asked before reviewing the concurrency code. It is clear that this list can be continued for a very long time.
If you are working on some system, it has clients, installations, then it obviously works with some kind of performance. In the modern world it is impossible to increase the hardware capacity infinitely. Need tests and performance monitoring. When conducting a code review, it is important to understand:
In general, code review is a very useful practice. I hope all developers (including enterprise-products) have already implemented it. If not, please implement as soon as possible. I would be happy to discuss with you the practice of code review in the comments.
Video lecture:
Presentation is available here .
I will give
Let's start with a little introductory. When communicating with people, especially in chat or by mail, especially in English, always keep in mind that your message may be perceived in a completely different way than you intended. Who will read the letter is unknown. He can be a Hindu, an Englishman, or just a sleepy man. There will always be differences in the understanding of concrete words, and without going into technical details, I will tell you how to minimize them.
Warning: the story contains sarcasm.
')
Bad advice for a contributor
Don't discuss the new feature with anyone.
If you want to implement a new feature, in no case do not discuss your revision with anyone. Someone can overhear and do it before you! Or, having heard the details, they can tell you that you did not understand something — to criticize your idea. Criticism hurts a lot. Perhaps such a feature as yours has already been discussed, and another one is needed ... In general, in no case tell anyone what you want to do. Never.
Never do technical documentation.
Committers and experienced guys from the community love to understand the code of the received patch. Therefore, in any case, do not do the technical documentation. Wiki, Jira, Mail list discussion - all this is nonsense. And nonsense is not for you!
When you send a patch to [+5000, -3500] with a description of the improvements in the form of “Factory methods and all other enhancements”, everyone will figure it out for themselves, and at the same time they will understand how well done you are.
Never run tests
Any tests are dangerous because they can break something. If you still managed to run the tests, do not show their results. Errors in the results can lead to patch rejection! And just never write new tests. Run All will be longer, consumption of processor time will increase. Anyway, good developers write code without bugs - any experienced colleague will look into your patches and understand that there are no errors.
Never read how-to
Coming to an open source project, never read any how-to. They write for fools, right?
Make out the code in your own way. Otherwise, how will everyone understand that you are a good developer and a versatile creative person with a developed sense of beauty?
Send patches at your convenience. For example, the project is tied to the GitHub infrastructure - send it as you send to the linux kernel, right in the letter. You can not even in the attachment, but right in the text. After all, Linus Torvalds bad advise! And if the project is taken differently, then this is the problem of the project.
Feel free to complicate the Public API
When developing a new Public API, try to make it as abstract and complex as possible. You can always answer any questions from users about unobvious API behavior: “Didn't you read the manual? Page 42, everything is clearly written there! Read again! In serious projects, everything should be difficult. Do we have a serious project?
Do not consult and talk about problems.
Do not advise anyone and do not tell anyone about the problems you faced in the development. Much more fun to understand all alone. Anyway, good developers always get everything right the first time, they have no problems. If others find out about your problems, they will become smarter and write code faster than you. This should not be allowed. And generally speaking about their problems is not accepted.
On the limitations of the final decision is also not worth mentioning. After all, the solution may be asked to modify. Who is interested in hearing about restrictions? When the user starts to introduce your product into the product and faces restrictions, it will be more interesting and more fun for him. He will come to you and ask you to finish. Until this point, in no case tell him about the restrictions - what if he decides not to implement anything?
A really good reviewer will find everything and ask you details. In no case do not tell him about anything.
If you have questions, write to the dev-list
This tip complements the previous one. It is best not only not to tell anyone anything, but also to ask any questions primarily on the dev-list.
Here are examples of questions that everyone likes to answer. If you write some kind of test, be sure to ask, "Do you need a check on the null of this collection?". You don’t have to figure it out yourself, you can always ask on the dev-list. After all, there are many people who are just waiting to be asked such a question. They will seek to answer it faster.
“How do I do the task?” Is another great question. In no case do not provide any details: the ID of the task will be sufficient. All who need to see for yourself.
“What is the framework to use?” Is also a very good question. It is always interesting to answer it, and it is possible to debate.
Fix all project problems in one pull request
This is my favorite tip. Fix all project problems in one pull request, especially if you work in the enterprise. Before you in the project were some fools, they wrote the code badly. And you write well. Accordingly, you simply must:
- refactor all existing code that you do not understand;
- rename all, in your opinion, incorrectly named variables;
- fix all existing javadoc comments.
And in general, you can take and render some classes, factories, etc., do other transformations to make the code better. Especially effectively it will look in areas that are not relevant to your task. So you can more fully reach your potential. In glory OOP! Amen!
Request code review in advance
When you do a task, and then send a request for code review, the process can be delayed. All experts are usually busy. Use the trick: when your task, as it seems to you, will end soon, request a review in advance. After all, they will not look right away - until they reach you, you can just finish everything.
Well, if the reviewer suddenly has time, while the task is not completed yet, then he is not lucky. Explain the situation, say that tomorrow everything will be ready. This way you speed up the process (at least in a review) and achieve a merge faster.
Tired of the challenge - throw
Everyone who works in open source has a lot of time. No need to finish. Passed code review, received comments. And why do something to rule and bring to the merge? Take the next puzzle and do. This is the way to success! The reviewer has a lot of time, and will look at the next patch without a result!
Reviewer - your enemy
How else to call the person who stands between you and the commit in master? Criticism of the code is a criticism of you personally! Who does he think he is? In personal communication, "bomb" as much as possible! Otherwise, how does the reviewer know that you care? This is the basic rule of development!
I recommend this advice to apply in daily development. It helps to achieve results and do it right.
Bad advice for the reviewer
Do not automate routine checks.
If you have reached the level of the project, when patches are already being sent to you for review, do not in any way automate routine checks! It is much more fun to spend a couple of review cycles on troubleshooting and discussion:
- code formatting;
- variable naming;
- checks whether those variables are marked final, which may be marked by it;
- ... and everything else.
Routine checks in any case do not need to automate. Yes, and tests to drive to anything.
Never disclose all the rules of the game until the very end.
To be ahead, you always need to keep a pair of aces in the sleeve. Don't tell anyone about the need for backward compatibility. Better just before the commit, say: “Here, according to our rules, we need to ensure backward compatibility. Please correct. This will be especially impressive if you have already done five times. On the sixth one can still say: “I am not an expert, so I have to wait for the review of Mr. X, who will look again.”
Such comments, especially at the final review, always add motivation to contributors.
Emotions, authorities and no thanks
This tip overlaps with the last tip of the contributor. Write comments to pull request as emotionally as possible. If somewhere is not checked for null or the variable is not so named, add passion to your comments. Let them see that you care.
If there is any dispute, in any case do not provide technical arguments. With technical arguments it may turn out that you are wrong. Referring to the authorities - this is the best argument in the dispute, you will always win.
If someone still passed your review, in no case can not say "thank you". Yet they want to commit to open source! In no case do not need to thank, and so they will come again!
And now seriously: what do you need to think about when preparing and conducting a code review?
I hope everyone understood how to conduct and review? In each of these tips, besides sarcasm, there is pain, which is often found in practice.
I will be the captain of Evidence and tell you what you really need to think about when preparing and conducting a code review. The considerations further apply both to the one that develops the feature and to the one that will be reviewing it. Still, these are two sides of the same coin.
A consensus in communication, firstly, is achievable, and secondly, it is necessary for the project to move forward. Currently, few products can be developed alone. This is usually teamwork.
Use common sense
This is the main advice. Use common sense, he steers. I think this advice applies to all life situations. If you are doing something, consider whether it meets the simple rule of common sense?
Guess ...
This is about culture. I was in a few large open source community. I do not know whether this is part of the Russian mentality, but often a person who can be perceived as a boss is also perceived as a person who fell into his place by accident. It is believed that he wants you bad, by default, there are doubts about his professionalism. Therefore, it is very important in any work to assume, even for a second, that:
- The reviewer (contributor, your boss or a colleague) is not your enemy . He does not want you bad. This is a simple assumption, but quite often it is not done. I advise him to do it all the same.
- The person who writes you comments is also a good engineer. You, of course, a good, such a feature made. But there are many other good engineers in the world. And the one who sent you comments also applies to them.
- This person also wants your task to be solved.
When he asks for something, he has some kind of motivation. He does it for a reason. Especially if this person is not the first day in the project. Surely there is some reason. About this reason, you can ask: Why do you need to do this? Why is backward compatibility needed here? If you ask simple questions calmly, reasonably and listen to the answers, you can achieve more.
What value does product refinement bring?
The review is not only ready-made patches, but also improvements to the project, fixes. In essence, code review starts at the moment when you are just discussing your revision. At this point, ask yourself: what value does refinement bring to the product?
- Will it be a new feature?
- Is this improvement existing?
- Extending an existing feature?
- Will it be refactoring code? There is nothing wrong. Some refer to refactoring critically, but it is necessary. And you need to realize that you are doing exactly it, and not a new feature or something else.
- Is it speeding up some process, improving performance?
- Is this a bug fix?
There are other options. In any case, starting to develop something, solve a problem, you must understand what value you add to the project.
Why is the revision (feature) just like that?
There are a number of helpful questions worth asking.
Why make a feature? Why is this feature needed? The answer to this question is important.
Where is the start of work? In my practice, it happened that I was asked to remake a certain application. There is an application A, you need to make an application B, which does almost the same with minor changes. I begin to do the work and it turns out that, in principle, it does not work. In fact, it is used somewhere in the production with the help of a human-machine interface - that is, someone is sitting and constantly restarting the program, correcting the null pointer exception literally on the air. Where is the start of work? In this case, it will be in the correction of program A, so that it works stably, then in the writing of program B.
Where is the complete end of work? How should the work done ideally look like? It is important to formulate from the very beginning where you are going.
Where is the end of the current stage? It is clear that the elephant does not immediately eat and the work is better divided into stages. It is important to understand where the end of the current stage. Often projects swell and do not end simply because the scope of the project becomes infinitely large.
Why is the feature broken into such stages? This is about MVP and all that. Please think about it too.
Now about the Public API
On the properties of the Public API there are many articles. Read before implementing it. As a good example I can cite the jQuery framework, Spring in Java. There are negative examples. Whoever programs Java for the first year probably remembers just terrible from the point of view of Public API EJB 2.1. Maybe the functionality is not bad there, but if the Public API is bad, no one can convince users to use the product.
Public API is not only a tool for third-party users. This is an internal component API that you reuse amongst yourself. Important properties of the Public API:
- Simplicity.
- Evidence.
- The correct defaults. It is worth thinking about them, if, for example, on a test environment you create 500 threads, as in production. Or vice versa, in the default production there are 3 streams, like on a test environment.
- Clear error messages. This is the scourge of so many products. When something falls from inside the system, it is not clear what was done wrong. Yesterday worked, today null pointer exception. What exactly you did wrong and how to fix it is not clear from the error message.
- It is difficult to make a mistake. There are a lot of recommendations on this. Compile time checks are always better than runtime checks, etc.
- Clear and sufficient logs. This is especially important when you write code that will be reused and deployed somewhere to the server.
- The possibility of monitoring. You need to understand that logs and monitoring are also part of your Public API. When parsing errors, users will watch how metrics behave that you spit out in monitoring.
Subsystem changes
When you come to code review, it is important to keep in your head a complete list of systems and subsystems of a large product, where changes take place. In enterprise projects, it may not be clear: whether it is a database schema, or a controller, or a view, some kind of reporting system, uploading, loading, etc.
When working with a boxed product, it is important to ask yourself the question: how do changes affect existing processes in the system? Is there backward compatibility? Does it affect performance? If it affects, then the performance of what? How does this affect the user experience?
Standard system processes
Each business system has standard processes: start, installation, a list of operations. How will they proceed now? Before code review it is important to understand this. You have to go through exactly the code that implements these processes. In the case of Ignite, this is:
- Node Start / Stop (server / client, coordinator / regular) - start, stop node, start server or client node, start coordinator or normal node
- Node Join - in terms of the new node / coordinator / server / client
- Cluster activation (& deactivation)
- Change of coordinator
- Create / Delete cache
- Persistence / BLT / MVCC
It is clear that the set of these processes is quite large. It is important to understand that such processes exist and how they change.
Corner cases
In your business application, disaster recovery, initial system initialization, node shutdown, restart, update of your application, and the like can happen. In the case of Ignite, we have the following corner cases:
- change of coordinator;
- drop node;
- network problems - when network messages do not reach;
- broken files.
These things we need to check and check that everything is in order.
Good code properties
So we got to the code. I advise you not to be lazy and seek in it:
- you just,
- extensibility
- testability
- reliability
- high speed work.
Concurrency
Java has its own peculiarities when writing concurrency code. If you have a business system and there is not enough concurrency there, you do not need to take these features into account. However, usually some synchronization passes through the database. In things like Ignite, it's a bit more complicated. And here not only the functionality of the code is important, but also its properties:
- How difficult is it to understand your concurrency model?
- Shared data structure - how is their integrity guaranteed?
- Locks - what and for what?
- Threads - what pools are used? Why?
- Guarantees of visibility of changes - how are they provided?
These questions need to be asked before reviewing the concurrency code. It is clear that this list can be continued for a very long time.
Performance tests Benchmarks
If you are working on some system, it has clients, installations, then it obviously works with some kind of performance. In the modern world it is impossible to increase the hardware capacity infinitely. Need tests and performance monitoring. When conducting a code review, it is important to understand:
- Do I need to perform performance testing at all? Maybe this is some kind of refinement, which performance tests are not needed?
- If so, which one? There are many methods and methods of measurement, etc.
- Where-how-to measure?
- What benchmarks are indicative? Number of nodes? Gland? Ignite configuration? The nature of the load?
Total
In general, code review is a very useful practice. I hope all developers (including enterprise-products) have already implemented it. If not, please implement as soon as possible. I would be happy to discuss with you the practice of code review in the comments.
Video lecture:
Presentation is available here .
Source: https://habr.com/ru/post/445440/
All Articles