code reviews

Introduction to code reviews

what's for?

 It is a software quality assurance activity.

how's done?

One or several people check a program mainly by viewing and reading parts of its source code (reviewers).

what to look for?

  • Code quality: Based on "clean code" principles of readability, consistency and atomicity.

 

formatting

If it's mandatory then it's automated.

formatting

from my_lib import B
from my_lib import A

import sys
import os

def is_unique(    s   ):
    s = list(s
                )
    s.sort()
    for i in range(len(s)-1):
        if s[i] == s[i+1]:
            return 0
    else:
        return 1
 
if __name__ == "__main__":
    print (
        is_unique(input())
    )
import os
import sys

from my_lib import A, B

def is_unique(s):
    s = list(s)
    s.sort()
    for i in range(len(s) - 1):
        if s[i] == s[i + 1]:
            return 0
    else:
        return 1
 
 
if __name__ == "__main__":
    print(is_unique(input()))

Automatic checks in CI

formatting

If it's not automated then it's not mandatory.

formatting

def add(a: str, b: str) -> str:
    return a + b

print(add("this", "that"))
print(add(1, 2))
from typing import Union


def add(
    a: Union[str, int],
    b: Union[str, int],
) -> Union[str, int]:
    return a + b


print(add("this", "that"))
print(add(1, 2))

Reviewer to check only if added.

def add(a, b):
    return a + b

print(add("this", "that"))
print(add(1, 2))

defects

Unused code, performance problems, unreachable and unnecessary code, etc.

unused code

Linters don't find everything.

Focus

On newly created functions.

+
+ def is_new(entity: Dict) -> bool:
+     if entity:
+         return entity.is_new()
+     return False

Focus

On removed function calls.


  def is_new(entity: Dict) -> bool:
      if entity:
-         return entity.is_new()
+         return entity["created"] > days_ago(1)
      return False

performance

If there's no performance problem, don't fix it.

unreacheable code

Logically unreacheable.


def country(person: Person) -> str:
    if person.is_nomad and person.country:
        return person.country
    return ""

unreacheable code

Programmatically unreacheable.


class Person:

    def __init__(self, country: str):
      self._country = country or ""
    
    def has_country(self):
      if self._country is None:
        raise ValueError("Unable to load the country")
      return self._country

unnecessary code

YAGNI or You Ain’t Gonna Need It.


def path(filename: str, filepath: str = None) -> str:
    if filepath:
        return f"{filename}/{filepath}"
    return f"/tmp/{filename}"

If nobody is calling this method with a filepath, then it's not necessary.

tests

Ensure the code is properly tested.

tests

Code coverage is not a metric for test quality.

unit test vs integration test

Unit tests are narrow in scope, and allow us to cover all cases, ensuring that every single part works correctly.

unit test vs integration test

const TEST_DATE_TIME = "2021-08-23T09:00:00+00:00";

it.each([
    ["UTC", "en-US", "8/23/2021, 9:00 AM"],
    ["America/New_York", "en-US", "8/23/2021, 5:00 AM"],
    ["Europe/London", "en-GB", "23/08/2021, 10:00"],
    ["Europe/Andorra", "es-ES", "23/8/2021 11:00"],
    ["Australia/Sydney", "en-AU", "23/08/2021, 7:00 pm"],
])(
    "localDateFormat for timezone %s and locale %s returns %s",
    (timeZone, locale, expectedOutput) => {
        Settings.defaultZone = timeZone;
        Settings.defaultLocale = locale;
        expect(localDateFormat(TEST_DATE_TIME)).toEqual(expectedOutput);
    }
);

unit test vs integration test

Integration tests demonstrate that different parts of a system work together and validate complex scenarios. 

unit test vs integration test

it("submits form with a button", () => {
    const name = "my name";
    cy.get("input").type(name);
    cy.get("button").click();
    cy.wait("@postAPI").its("request.body").should("containSubset", { name });
});

@pytest.fixture
def post_entity(test_client):
    def _run(data: Dict):
        response = test_client.post("/api/v1/entity/", data=data)
        assert response.status_code == 201
        return response.get_json()

    return _run


def test_entity_is_created_with_name(post_entity):
    test_name = "test-name"
    response = post_entity({"name": test_name})
    assert response["name"] == test_name

UNIT TEST VS INTEGRATION TEST

Integration tests don't replace/substitute unit tests.

UNIT TEST VS INTEGRATION TEST

Testing private methods break encapsulation.

 

Be aware of Icebergs.

UNIT TEST VS INTEGRATION TEST

Testing must cover everything.

 

Not being able to cover everything is the exception, not the rule.

UNIT TEST VS INTEGRATION TEST

class Entity:
  
    TYPE_A = "A"
    TYPE_B = "B"
  
    def __init__(self, config: Dict, **kwargs):
        self._config = config
       	self._id = kwargs["id"]
        self._type = kwargs["type"]
        
    def _type_label(self):
        if self._type in [self.TYPE_A, self.TYPE_B]:
          return self._config.get(f"type_{self._type}")
        return self._config.get("default_type", "unknown")

    def name(self) -> str:
        return f"[{self._id}] {self._type_label()}"
      
    @staticmethod
    def load_entities(config: Dict) -> List[Entity]:
      	def _is_entity(row: Dict) -> bool:
          return "id" in row and "type" in row
      
        entities = [row for row in load_from_database() if _is_entity(row)]
        return [Entity(config, **entity) for entity in entities]

?

code standards

Clean Code: A Handbook of Agile Software Craftsmanship

code standards

DRY or Don't repeat yourself

Beware of premature abstraction.
 

Do not mistake for splitting responsibilities.

code standards

DRY or Don't repeat yourself

Beware of premature abstraction.
 

Do not mistake for splitting responsibilities.

Sources

https://reinvently.com/blog/fundamentals-web-application-architecture/

https://www.oreilly.com/library/view/clean-code-a/9780136083238/

https://caniuse.com/

Code Reviews

By Gorka Guridi

Made with Slides.com

Code Reviews

Code reviews approach

  • 102

More from Gorka Guridi