Code review: bad tips for contributor and reviewer

    Hello! My name is Nikolai Izhikov. In this post I want to talk about one important element of interaction that we encounter in the process of software development, especially in open source. This is a walkthrough and a code review.

    I will give harmful advice on how to make my feature and bring it to merge in the opensource project wizard in the context of cultural, temporal, conceptual and other differences between community members. This is usually a tricky question, especially for those who are just starting out in open source.

    Let's start with a small introduction. When communicating with people, especially in chat or by mail, especially in English, always keep in mind that your message can be perceived completely differently from what you expected. Who will read the letter is unknown. He can be a Hindu, an Englishman, or just a sleepy person. There will always be differences in understanding specific words, and without going into technical details, I’ll tell you how to minimize them.

    Warning: the story contains sarcasm.

    Bad tips for a contributor

    Do not discuss a new feature with anyone

    If you want to implement a new feature, in no case do not discuss your revision with anyone. Someone can eavesdrop and do it before you! Or, having heard the details, they may tell you that you did not understand something - to criticize your idea. Criticism hurts a lot. Perhaps a feature like yours has already been discussed, and you need another ... In general, in no case tell anyone what you want to do. Never.

    Never do technical documentation

    Committers and experienced guys from the community are very fond of understanding the code of the received patch. Therefore, in no case do not do technical documentation. Wiki, Jira, Mail list discussion - all this is complete nonsense. And the bullshit is not for you!

    When you send a patch to [+5000, -3500] with a description of the improvements in the form of “Factory methods improvements and some other enhancements”, everyone will figure it out themselves, and at the same time will understand how good 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 rejection of the patch! And definitely never write new tests. Run All will be longer, CPU consumption will increase. Anyway, good developers write code without bugs - any experienced colleague will look at your patches and understand that there are no errors there.

    Never Read How-To

    Coming into an opensource project, never read any How-To. They are written for fools, right?
    Design your 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 as it is convenient for you. For example, the project is tied to the GitHub infrastructure - send it as it is sent to linux kernel, right in the letter. You can even not in the attachment, but directly in the text. After all, Linus Torvalds will not advise bad! And if the project is adopted differently, then these are problems of the project.

    Feel free to complicate the Public API

    When developing the new Public API, try to make it as abstract and complex as possible. Any user questions about the non-obvious behavior of the API can always be answered: “Have you not read the manual? Page 42, everything is written clearly! Read it again! ” In serious projects, everything should be complicated. We have a serious project?

    Do not advise or talk about problems

    Do not consult with anyone and do not tell anyone about the problems encountered in the development. It’s much more fun to understand one thing. Anyway, good developers always succeed 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 indeed it’s not customary to talk about one’s problems.

    The limitations of the final decision are also not worth mentioning. After all, the solution may be asked to finalize. Who cares about restrictions? When a user starts introducing your product into the product and encounters restrictions, he will be more interested and more fun. He will come to you and ask you to finish it. And up to this point, in no case do not tell him about the restrictions - what if he decides not to implement anything?

    A really good reviewer will find everything and he will ask you for 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’s best not only to not 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 are writing some kind of test, be sure to ask, "Do I need a null check for this collection?" You don’t have to figure it out on your own, you can always ask on the dev sheet. After all, there are many people who are just waiting to be asked such a question. They will seek to respond faster to it.

    "How do I do the task?" - Another great question. In no case do not indicate any details: the task ID will be enough. All who need it will see for themselves.

    “Which framework to use?” - also a very good question. It is always interesting to answer it, and you can debate.

    Fix all project problems in one pull request

    This is my favorite tip. Fix all the problems of the project in one pull request, especially if you are working in enterprise. There were only fools in the project before you, they wrote the code poorly. And you write well. Accordingly, you simply must:

    • Refactor all existing code that you don’t understand;
    • rename all, in your opinion, incorrectly named variables;
    • fix all existing javadoc comments.

    In general, you can take and take out some classes, factories, etc., make other transformations so that the code is better. This will look especially impressive in areas that are not relevant to your task. So you can more fully reveal your potential. To the Glory of the OOP! Amen!

    Request a code review in advance

    When you do a task, and then send a request for code review, the process may take a while. All experts are usually busy. Take advantage of the trick: when your task, as it seems to you, will end soon, request a review in advance. After all, they won’t look right away - until your hands reach you, you can just finish it all.

    Well, if the reviewer suddenly has time, while the task has not yet been completed, then he was unlucky. Explain the situation, say that tomorrow everything will be ready. This way you speed up the process (at least by review) and get merge faster.

    Tired of the task - drop it

    Everyone who works in open source has a lot of time. No need to finish anything. Passed code review, received comments. And why edit and bring something to merge? Take the following puzzle and do it. This is the way to success! The reviewer has a lot of time, and the next patch will look without result!

    Reviewer is your enemy

    And what else to name 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 in everyday development. This helps to achieve the result and do it right.

    Bad Advice for the Reviewer

    Do not automate routine checks

    If you have reached the level in the project when patches for review are already being sent to you, in no case do not 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 can be marked by them;
    • ... and everything else.

    In no case should routine checks be automated. Yes, and tests to drive to nothing.

    Never reveal all the rules of the game until the very end.

    To be ahead, you always need to keep a pair of aces up your sleeve. Do not tell anyone about the need for backward compatibility. It’s better to say just before the commit: “Here, according to our rules, backward compatibility must be ensured. Please correct. ” This will be especially effective if you’ve already reviewed five times. On the sixth one can still say: "I'm not an expert, so you need to wait for the review of Mr. X, who will look again."

    Such comments, especially at the final stage of the review, always add motivation to contributors.

    Emotions, authorities and no thanks

    This tip overlaps with the latest contributor tip. Write comments on pull request as emotionally as possible. If somewhere is not checked for null or the variable is not named that way, add passion to your comments. Let them see that you care.

    If a dispute arises, in no case give technical arguments. With technical arguments, it may turn out that you are wrong. Refer to the authorities - this is the best argument in the dispute, you will always win.

    If someone did go through your review, you should never say thank you. Still they want to commit to open source! In no case do not have to thank, and so they will come again!

    Now seriously: what should you think about when preparing and conducting a code review?

    I hope everyone understood how to conduct and pass the review? In each of these tips, in addition to sarcasm, there is pain that often occurs 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. Considerations further apply both to the one that develops the feature, and to the one that will review it. Still, these are two sides of the same coin.
    Consensus in communication is, firstly, achievable, and secondly, it is necessary for the project to move forward. Currently, few products can be developed alone. Usually this is teamwork.

    Use common sense

    This is the most important advice. Use common sense, it steers. It seems to me that this advice applies to all life situations. If you are doing something, consider whether this meets the simple rule of common sense?

    Assume ...

    This is about culture. I have been to several large open source community. I do not know if this is part of the Russian mentality, but often a person who can be perceived as a boss is simultaneously perceived as a person who fell into his place by chance. 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 at least for a second that:

    • Ревьювер (контрибьютер, ваш начальник или коллега) вам не враг. Он не хочет вам плохого. Это простое предположение, но достаточно часто оно не делается. Я советую его все-таки делать.
    • Человек, который пишет вам замечания, тоже хороший инженер. Вы, понятно, хороший, такую фичу сделали. Но на свете есть много других хороших инженеров. И тот, кто прислал вам замечания, тоже к ним относится.
    • This person also wants your task to be accomplished.

      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. You can ask about this reason: Why do you need to do just that? Why is backward compatibility needed here? If you ask simple questions calmly, reasonably and listen to the answers, you can achieve more.

    What value will the product bring?

    The review is not only ready-made patches, but also project improvements, corrections. In fact, code review begins at the moment when you are just discussing your revision. At this point, ask yourself: what value will the product bring to the product?

    • Will it be a new feature?
    • Is this an existing improvement?
    • Extension of an existing feature?
    • Will it be code refactoring? There is nothing wrong. Some are critical of refactoring, but it is necessary. And you need to be aware that you are doing it, and not a new feature or something else.
    • Is it speeding up a process, improving performance?
    • Is this a bug fix?

    There are other options. In any case, starting to develop something, to solve a problem, you should understand what value you will add to the project.

    Why is the revision (feature) just like that?

    There are a number of useful questions to ask.

    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 redo a certain application. There is an application A, you need to make an application B, which does almost the same with minor changes. I start to do the work and it turns out that A basically does not work. In fact, it is used somewhere in production using the human-machine interface - that is, someone sits and constantly restarts the program, fixing the null pointer exception literally on the air. Where is the start of work? In this case, it will be in fixing program A so that it works stably, then in writing program B.

    Where is the full completion of work? How should the work done look ideal? It’s important to formulate from the very beginning where you are going.

    Where is the end of the current stage? It is clear that you can’t eat an elephant right away and it’s better to break down the work into stages. It is important to understand where the end of the current stage is. Often projects are inflated and do not end just because the scope of the project becomes infinitely large.

    Why is the feature broken down precisely at such stages? This is about MVP and all that. Please think about this too.

    Now about the Public API

    There are many articles about the properties of the Public API. Read it before you implement it. As a good example, I can cite the JQuery, Spring framework in Java. There are also negative examples. Those who have been programming in Java for years, probably remember just the terrible from the point of view of the Public API EJB 2.1. The functionality there may be good, 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 and the API of internal components, which you yourself reuse among themselves. Important properties of the Public API:

    • Simplicity.
    • The evidence.
    • Правильные умолчания. Стоит подумать о них, если, например, на тестовом окружении вы создаете 500 потоков, как на продакшене. Или наоборот, в продакшене по умолчанию 3 потока, как на тестовом окружении.
    • Понятные сообщения об ошибках. Это бич очень многих продуктов. Когда что-то валится изнутри системы, непонятно, что сделано неправильно. Вчера работало, сегодня null pointer exception. Что именно вы сделали не так и как это исправить, из сообщения об ошибке не понятно.
    • Сложно допустить ошибку. На этот счет есть очень много рекомендаций. Compile time проверки всегда лучше чем проверки runtime и т.п.
    • Понятные и достаточные логи. Это особенно важно, когда вы пишете код, который будет переиспользоваться и деплоиться куда-то на сервер.
    • The ability to monitor. You need to understand that logs and monitoring are also part of your Public API. When parsing errors, users will see how the metrics that you spit out in monitoring behave.

    Subsystem Changes

    When you come to the code review, it’s important to have in your head a complete list of systems and subsystems of a large product where you are changing. In enterprise projects, it may not be clear: whether it is a database schema, or a controller, or a presentation, some kind of reporting system, uploads, downloads, 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 a backward compatibility? Does it affect performance? If it affects, then the performance of what? How does this affect user experience?

    Standard system processes

    Each business system has standard processes: start, installation, some list of operations. How will they flow now? Before code review, it's important to understand this. You must go through the code that implements these processes. In the case of Ignite, this is:

    • Node Start / Stop (server / client, coordinator / regular) - start, stop a node, start a server or client node, start a coordinator or a normal node
    • Node Join - in terms of a 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, updating your application, and the like can happen. In the case of Ignite, we have the following corner cases:

    • change of coordinator;
    • node drop;
    • network problems - when network messages do not reach;
    • broken files.

    We must check and verify these things that everything is in order.

    Good code features

    So we got to the code. I advise you not to be lazy and seek in it:

    • you just,
    • extensibility
    • testability
    • reliability
    • high speed of work.


    Java has its own peculiarities when writing concurrency code. If you have a business system, and there is little concurrency there, you do not need to consider these features. However, usually some synchronization passes through the database. In things like Ignite, this is a little more complicated. And here, not only the functionality of the code is important, but also its properties:

    • How hard is it to understand your concurrency model?
    • Shared data structure - how is their integrity guaranteed?
    • Locks - what and why?
    • Threads - which pools are used? Why?
    • Guarantees of visibility of changes - what are they provided for?

    These questions must be asked before revising concurrency code. It is clear that this list can be continued for a very long time.

    Performance tests. Benchmarks

    If you are developing some kind of system, it has clients, installations, then it obviously works with some performance. In today's world, it is impossible to increase hardware power indefinitely. We need tests and performance monitoring. When conducting a code review, it is important to understand:

    • Is performance testing necessary at all? Maybe this is some kind of refinement that does not need performance tests?
    • If necessary, which one? There are many measurement techniques and methods, etc.
    • Where-how-what needs to be measured?
    • What benchmarks are indicative? The number of nodes? Gland? Ignite configuration? The nature of the load?


    Overall, code review is a very rewarding practice. I hope all developers (including enterprise products) have already implemented it at home. If not, please implement as soon as possible. I will be happy to discuss code review practices with you in the comments.

    Video of the lecture:

    Presentation is available here .

    Also popular now: