Introduction to code reviews
It is a software quality assurance activity.
One or several people check a program mainly by viewing and reading parts of its source code (reviewers).
If it's mandatory then it's automated.
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
If it's not automated then it's not mandatory.
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))
Unused code, performance problems, unreachable and unnecessary code, etc.
Linters don't find everything.
On newly created functions.
+
+ def is_new(entity: Dict) -> bool:
+ if entity:
+ return entity.is_new()
+ return False
On removed function calls.
def is_new(entity: Dict) -> bool:
if entity:
- return entity.is_new()
+ return entity["created"] > days_ago(1)
return False
If there's no performance problem, don't fix it.
Logically unreacheable.
def country(person: Person) -> str:
if person.is_nomad and person.country:
return person.country
return ""
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
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.
Ensure the code is properly tested.
Code coverage is not a metric for test quality.
Unit tests are narrow in scope, and allow us to cover all cases, ensuring that every single part works correctly.
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);
}
);
Integration tests demonstrate that different parts of a system work together and validate complex scenarios.
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
Integration tests don't replace/substitute unit tests.
Testing must cover everything.
Not being able to cover everything is the exception, not the rule.
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]
?
Clean Code: A Handbook of Agile Software Craftsmanship
DRY or Don't repeat yourself
Beware of premature abstraction.
Do not mistake for splitting responsibilities.
DRY or Don't repeat yourself
Beware of premature abstraction.
Do not mistake for splitting responsibilities.
https://reinvently.com/blog/fundamentals-web-application-architecture/
https://www.oreilly.com/library/view/clean-code-a/9780136083238/
https://caniuse.com/