Code review and clean coding principles at ORNIO

How to make a good review of the code? Where to start? What you should remember? Why is clean coding important?
I will show you my practices which will help not only with the product itself, but also make wonders for the development team.

Code review

Code review helps give a fresh set of eyes to identify bugs and simple coding errors before your product gets to the next step, making the process for getting the software to the customer more efficient. Simply reviewing someone's code and identifying errors is great.

Here at Ornio we use this guideline to do an organised code review.

• Ask open-ended questions instead of making strong or opinionated statements. Offer alternatives and possible workarounds that might work better for the situation without insisting those solutions are the best or only way to proceed. These reviews assume the reviewer might be missing something and ask for clarification instead of correction.

• Once a reviewer completes their review, they can either mark it approved, block the review with change requests, or not set a specific status, leaving it in a “not yet approved” state.

• Flexible on the practice: sometimes, certain comments are addressed by the author with a separate, follow-up code change. For changes that are more urgent than others, reviewers should try to make themselves available for quicker reviews.

• Leave as many comments and questions as needed. If the revision does not address all of them, they will note those as well. When the conversation gets into a long back-and-forth, reviewers should try to switch to talking to the author in-person instead of burning more time using the code review tool.

• Proactively reach out to the person making the change after they do a first pass on the code and have lots of comments and questions. This will save a lot of time, misunderstandings, and hard feelings this way. The fact that there are many comments on the code indicates that there is likely some misunderstanding on either side. These kinds of misunderstandings are easier identified and resolved by talking things through.

• Make it clear when changes are unimportant nitpicks. They usually mark comments like these distinctively, adding the “nit:” prefix to them. Too many of these can become frustrating and take the attention away from the more important parts of the review, so reviewers aim to not go overboard with these.

• Realize that too many nitpicks are a sign of lack of tooling or a lack of standards. Reviewers who come across these frequently will look at solving this problem outside the code review process. For example, many of the common nitpick comments can be solved via automated linting. Those that cannot, can usually be resolved by the team agreeing to certain standards and following them—perhaps even automating them, eventually.

• Ensure that all engineers take part in the code review process—even those that might be working on solo projects.

• Have hard rules around no code making it to production without a code review—just as business logic changes don’t make it to production without automated tests.

• It is the author’s responsibility to submit PRs that are easy to review in order not to waste reviewers’ time and motivation:

  • Scope and size. Changes should have a narrow, well-defined, self-contained scope that they cover exhaustively.
    • For example, a change may implement a new feature or fix a bug. Shorter changes are preferred over longer ones. If a PR makes substantive changes to more than ~5 files, or took longer than 1–2 days to write, or would take more than 20 minutes to review, consider splitting it into multiple self-contained PRs.
    • For example, a developer can submit one change that defines the API for a new feature in terms of interfaces and documentation, and a second change that adds implementations for those interfaces.
  • Review fewer than 400 lines of code at a time.
  • Do not review for more than 60 minutes at a time
  • Only submit complete, self-reviewed (by diff), and self-tested PRs (Authors should annotate source code before the review). In order to save reviewers’ time, test the submitted changes (i.e., run the test suite) and make sure they pass all builds as well as all tests and code quality checks, both locally and on the CI servers, before assigning reviewers.
  • It is customary for the committer to propose one or two reviewers who are familiar with the code base. Often, one of the reviewers is the project lead or a senior engineer.
  • Project owners should consider subscribing to their projects in order to get notified of new PRs.
  • As a reviewer, it is your responsibility to enforce coding standards and keep the quality bar up.
  • Reviewing code is more of an art than a science. The only way to learn it, is to do it; an experienced reviewer should consider putting other less experienced reviewers on their changes and have them do a review first.

Maintainability

• Read the tests. If there are no tests and there should be, ask the author to write some. Truly untestable features are rare, while untested implementations of features are unfortunately common. Check the tests themselves: are they covering interesting cases? Are they readable? Does the PR lower overall test coverage? Think of ways this code could break. Style standards for tests are often different from core code, but still important.

• Does this code need integration tests? Sometimes, code can’t be adequately tested with unit tests alone, especially if the code interacts with outside systems or configuration.

Code Reviews for New Joiners

• The codebase is new, the style of programming is different than before, and people review your code very differently. So should code reviews be gentler for new starters, to get them used to the new environment, or should they keep the bar just as high, as it is for everyone else?
  • Use the same quality bar and approach for everyone, regardless of their job title, level or when they joined the company. Following the above, code reviews have a kind tone, request changes where needed, and will reach out to talk to reviewers when they have many comments.

Clean coding

• Decide on the indentation and keep it that way, follow standard conventions.

  • The code should be divided into logical units and it’s important to keep the style consistent throughout the whole document. In most of the programming languages, spaces and indentations do not affect the function. If you are using an IDE, it is recommended that you create a formatted config so everyone uses the same one.
  • Make comments
    • Comments will help you and others understand why you did what you did.
      • Always try to explain yourself in code.
      • Don't be redundant.
      • Don't add obvious noise.
      • Don't use closing brace comments.
      • Don't comment out code. Remove it instead.
      • Use as explanation of intent.
      • Use as clarification of code.
      • Use as warning of consequences.
  • Consistent name scheme
    • Be consistent. If you do something a certain way, do all similar things in the same way. Naming needs to stay consistent, otherwise, it will be very difficult to find things inside the document.
  • Don’t repeat code
    • Repeating code will make your document very long and it will break the reading flow. If you have pieces of code that will be used more than once, it is preferable to make a separate file.
  • Avoid writing long code lines
    • Writing long lines of code makes it very difficult to read, moving back and forth horizontally, losing any sense of what it’s actually written. Style and indentation will help with this.
  • Break down a big task into smaller chunks
    • A whole new feature will never be just a few lines long. Even with comments, a 500-lines function will still be a pain to browse, understand and edit. Your best choice is to break down the big task into a smaller chunk of code.
  • Organize your program into smaller files
    • Having a file with thousands of lines of code doesn’t help anyone, but broken down into shorter files organized based on function or feature will help you get right to the point when something needs fixing. Having the whole code organized in files and folders is very good for maintainability, especially if different teams and people are working on it.
      • Small.
      • Do one thing.
      • Use descriptive names.
      • Prefer fewer arguments.
      • Have no side effects.
      • Don't use flag arguments. Split method into several independent methods that can be called from the client without the flag.
  • Keep it simple, write clever code that is also readable
    • Write clever code, but the main focus should be on readability and maintainability. If the code is kept short it is easier to go through, but if it’s too smart, it will take too much time to understand and edit it. You know that your code was too clever if you can’t read it after 3 months. So be clever but not too much!
  • Code refactoring
    • Every developer knows this – you write some code because you want to finish the feature and in the end, it works. When you then look at it after some time you think – did I really write that? I could have shortened it so much! So you start rewriting that piece of code better, probably shorter, without changing any functionality. Ps. It is the developer’s responsibility to create tasks for refactoring the code.
  • Delete unnecessary code
    • You wrote new code, great. Is the new code working? If yes, then just delete the old one! There is no point in keeping the old code, and comment it out. It will just look messy and unnecessarily long.
  • Must avoid doing:
    • Rigidity. The software is difficult to change. A small change causes a cascade of subsequent changes.
    • Fragility. The software breaks in many places due to a single change.
    • Immobility. You cannot reuse parts of the code in other projects because of involved risks and high effort.
    • Needless Complexity.
    • Needless Repetition.
    • Opacity. The code is hard to understand.
    • Try to avoid temporary variables.
      • Using too many temporary variables can cause memory leaks, because of that try to use temporary variables only when:
        • statements are getting too long and too complex
      • Don’t use temporary variables when:
        • Return a value from a function:

                                Don’t do:

function name(){
  const message = "Hello";
  return message;
}

                                Do:

function name(){
  return "Hello";
}