diff --git a/.gitattributes b/.gitattributes index b1d6160..9cb09f4 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,2 +1,2 @@ *.md linguist-documentation=false -*.md linguist-language=PHP +*.md linguist-language=Python diff --git a/.github/workflows/python-app.yml b/.github/workflows/python-app.yml new file mode 100644 index 0000000..8c262bd --- /dev/null +++ b/.github/workflows/python-app.yml @@ -0,0 +1,36 @@ +# This workflow will install Python dependencies, run tests and lint with a single version of Python +# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions + +name: Python application + +on: + push: + branches: [ master ] + pull_request: + branches: [ master ] + +jobs: + build: + + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + - name: Set up Python 3.10 + uses: actions/setup-python@v2 + with: + python-version: "3.10" + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install flake8 pytest + if [ -f requirements.txt ]; then pip install -r requirements.txt; fi + - name: Lint with flake8 + run: | + # stop the build if there are Python syntax errors or undefined names + flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics + # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide + flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics + - name: Test with pytest + run: | + pytest diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..d7e5931 --- /dev/null +++ b/.gitignore @@ -0,0 +1,129 @@ +# Byte-compiled / optimized / DLL files +__pycache__/ +*.py[cod] +*$py.class + +# C extensions +*.so + +# Distribution / packaging +.Python +build/ +develop-eggs/ +dist/ +downloads/ +eggs/ +.eggs/ +lib/ +lib64/ +parts/ +sdist/ +var/ +wheels/ +pip-wheel-metadata/ +share/python-wheels/ +*.egg-info/ +.installed.cfg +*.egg +MANIFEST + +# PyInstaller +# Usually these files are written by a python script from a template +# before PyInstaller builds the exe, so as to inject date/other infos into it. +*.manifest +*.spec + +# Installer logs +pip-log.txt +pip-delete-this-directory.txt + +# Unit test / coverage reports +htmlcov/ +.tox/ +.nox/ +.coverage +.coverage.* +.cache +nosetests.xml +coverage.xml +*.cover +*.py,cover +.hypothesis/ +.pytest_cache/ + +# Translations +*.mo +*.pot + +# Django stuff: +*.log +local_settings.py +db.sqlite3 +db.sqlite3-journal + +# Flask stuff: +instance/ +.webassets-cache + +# Scrapy stuff: +.scrapy + +# Sphinx documentation +docs/_build/ + +# PyBuilder +target/ + +# Jupyter Notebook +.ipynb_checkpoints + +# IPython +profile_default/ +ipython_config.py + +# pyenv +.python-version + +# pipenv +# According to pypa/pipenv#598, it is recommended to include Pipfile.lock in version control. +# However, in case of collaboration, if having platform-specific dependencies or dependencies +# having no cross-platform support, pipenv may install dependencies that don't work, or not +# install all needed dependencies. +#Pipfile.lock + +# celery beat schedule file +celerybeat-schedule + +# SageMath parsed files +*.sage.py + +# Environments +.env +.venv +env/ +venv/ +ENV/ +env.bak/ +venv.bak/ + +# Spyder project settings +.spyderproject +.spyproject + +# Rope project settings +.ropeproject + +# mkdocs documentation +/site + +# mypy +.mypy_cache/ +.dmypy.json +dmypy.json + +# Pyre type checker +.pyre/ + +# VSCODE +.vscode +.idea diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..4e198a2 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,9 @@ +language: python +python: + - "3.8.3" +# command to install dependencies +install: + - make deps +# command to run tests +script: + - make tests diff --git a/CONTRIBUTORS b/CONTRIBUTORS new file mode 100644 index 0000000..96657b2 --- /dev/null +++ b/CONTRIBUTORS @@ -0,0 +1,17 @@ +Rigel Di Scala +Zachary Anglin +AirbusDriver +Micheal +Erik OShaughnessy +Mukhammad Karimov +sitnarf +Miguel Gonzalez +Anvar +Martin Pavlásek +Shahrukh Khan +Aaron Law +Fredson Chaves +MartinThoma +Ali Bagheri +yinruiqing +YongWoo Lee diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..eb8f7eb --- /dev/null +++ b/Makefile @@ -0,0 +1,26 @@ +.PHONY: deps clean tests + +ENV=.env +PYTHON=python3 +PYTHON_VERSION=$(shell ${PYTHON} -V | cut -d " " -f 2 | cut -c1-3) +SITE_PACKAGES=${ENV}/lib/python${PYTHON_VERSION}/site-packages +IN_ENV=source ${ENV}/bin/activate; + +default: tests + +${ENV}: + @echo "Creating Python environment..." >&2 + @${PYTHON} -m venv ${ENV} + @echo "Updating pip..." >&2 + @${IN_ENV} pip install -U pip + +${SITE_PACKAGES}/pytest.py: + @${IN_ENV} pip install -r requirements.txt + +deps: ${SITE_PACKAGES}/pytest.py + +tests: ${ENV} ${SITE_PACKAGES}/pytest.py + @${IN_ENV} pytest + +clean: + @rm -rf ${ENV} .env .pytest_cache diff --git a/README.md b/README.md index 0b7bdc6..cae487b 100644 --- a/README.md +++ b/README.md @@ -1,112 +1,161 @@ # clean-code-python +[![Build Status](https://travis-ci.com/zedr/clean-code-python.svg?branch=master)](https://travis-ci.com/zedr/clean-code-python) +[![](https://img.shields.io/badge/python-3.8+-blue.svg)](https://www.python.org/download/releases/3.8.3/) + ## Table of Contents - 1. [Introduction](#introduction) - 2. [Variables](#variables) - 3. [Functions](#functions) - 4. [Objects and Data Structures](#objects-and-data-structures) - 5. [Classes](#classes) - 1. [S: Single Responsibility Principle (SRP)](#single-responsibility-principle-srp) - 2. [O: Open/Closed Principle (OCP)](#openclosed-principle-ocp) - 3. [L: Liskov Substitution Principle (LSP)](#liskov-substitution-principle-lsp) - 4. [I: Interface Segregation Principle (ISP)](#interface-segregation-principle-isp) - 5. [D: Dependency Inversion Principle (DIP)](#dependency-inversion-principle-dip) - 6. [Don’t repeat yourself (DRY)](#dont-repeat-yourself-dry) + +1. [Introduction](#introduction) +2. [Variables](#variables) +3. [Functions](#functions) +5. [Classes](#classes) + 1. [S: Single Responsibility Principle (SRP)](#single-responsibility-principle-srp) + 2. [O: Open/Closed Principle (OCP)](#openclosed-principle-ocp) + 3. [L: Liskov Substitution Principle (LSP)](#liskov-substitution-principle-lsp) + 4. [I: Interface Segregation Principle (ISP)](#interface-segregation-principle-isp) + 5. [D: Dependency Inversion Principle (DIP)](#dependency-inversion-principle-dip) +6. [Don't repeat yourself (DRY)](#dont-repeat-yourself-dry) +7. [Translations](#translations) ## Introduction Software engineering principles, from Robert C. Martin's book -[*Clean Code*](https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882), -adapted for Python. This is not a style guide. It's a guide to producing +[*Clean +Code*](https://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882) +, adapted for Python. This is not a style guide. It's a guide to producing readable, reusable, and refactorable software in Python. -Not every principle herein has to be strictly followed, and even fewer will be universally -agreed upon. These are guidelines and nothing more, but they are ones codified over many -years of collective experience by the authors of *Clean Code*. +Not every principle herein has to be strictly followed, and even fewer will be +universally agreed upon. These are guidelines and nothing more, but they are +ones codified over many years of collective experience by the authors of *Clean +Code*. -Inspired from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript) +Adapted +from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript) Targets Python3.7+ ## **Variables** + ### Use meaningful and pronounceable variable names **Bad:** + ```python +import datetime + ymdstr = datetime.date.today().strftime("%y-%m-%d") ``` +Additionally, there's no need to add the type of the variable (str) to its +name. + **Good**: + ```python +import datetime + current_date: str = datetime.date.today().strftime("%y-%m-%d") ``` + **[⬆ back to top](#table-of-contents)** ### Use the same vocabulary for the same type of variable **Bad:** Here we use three different names for the same underlying entity: + ```python -get_user_info() -get_client_data() -get_customer_record() +def get_user_info(): pass + + +def get_client_data(): pass + + +def get_customer_record(): pass ``` **Good**: -If the entity is the same, you should be consistent in referring to it in your functions: +If the entity is the same, you should be consistent in referring to it in your +functions: + ```python -get_user_info() -get_user_data() -get_user_record() +def get_user_info(): pass + + +def get_user_data(): pass + + +def get_user_record(): pass ``` **Even better** -Python is (also) an object oriented programming language. If it makes sense, package the functions together with the concrete implementation -of the entity in your code, as instance attributes, property methods, or methods: +Python is (also) an object oriented programming language. If it makes sense, +package the functions together with the concrete implementation of the entity +in your code, as instance attributes, property methods, or methods: ```python +from typing import Union, Dict + + +class Record: + pass + + class User: - info : str + info: str @property - def data(self) -> dict: - # ... + def data(self) -> Dict[str, str]: + return {} def get_record(self) -> Union[Record, None]: - # ... + return Record() ``` **[⬆ back to top](#table-of-contents)** ### Use searchable names -We will read more code than we will ever write. It's important that the code we do write is -readable and searchable. By *not* naming variables that end up being meaningful for -understanding our program, we hurt our readers. -Make your names searchable. + +We will read more code than we will ever write. It's important that the code we +do write is readable and searchable. By *not* naming variables that end up +being meaningful for understanding our program, we hurt our readers. Make your +names searchable. **Bad:** + ```python -# What the heck is 86400 for? -time.sleep(86400); +import time + +# What is the number 86400 for again? +time.sleep(86400) ``` **Good**: + ```python +import time + # Declare them in the global namespace for the module. SECONDS_IN_A_DAY = 60 * 60 * 24 - time.sleep(SECONDS_IN_A_DAY) ``` + **[⬆ back to top](#table-of-contents)** ### Use explanatory variables + **Bad:** + ```python -address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = r'^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' -matches = re.match(city_zip_code_regex, address) +import re + +address = "One Infinite Loop, Cupertino 95014" +city_zip_code_regex = r"^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$" -save_city_zip_code(matches[1], matches[2]) +matches = re.match(city_zip_code_regex, address) +if matches: + print(f"{matches[1]}: {matches[2]}") ``` **Not bad**: @@ -114,54 +163,65 @@ save_city_zip_code(matches[1], matches[2]) It's better, but we are still heavily dependent on regex. ```python -address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = r'^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' +import re + +address = "One Infinite Loop, Cupertino 95014" +city_zip_code_regex = r"^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$" matches = re.match(city_zip_code_regex, address) -city, zip_code = matches.groups() -save_city_zip_code(city, zip_code) +if matches: + city, zip_code = matches.groups() + print(f"{city}: {zip_code}") ``` **Good**: Decrease dependence on regex by naming subpatterns. + ```python -address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = r'^[^,\\]+[,\\\s]+(?P.+?)\s*(?P\d{5})?$' -matches = re.match(city_zip_code_regex, address) +import re -save_city_zip_code(matches['city'], matches['zip_code']) +address = "One Infinite Loop, Cupertino 95014" +city_zip_code_regex = r"^[^,\\]+[,\\\s]+(?P.+?)\s*(?P\d{5})?$" + +matches = re.match(city_zip_code_regex, address) +if matches: + print(f"{matches['city']}, {matches['zip_code']}") ``` + **[⬆ back to top](#table-of-contents)** ### Avoid Mental Mapping + Don’t force the reader of your code to translate what the variable means. Explicit is better than implicit. **Bad:** + ```python -seq = ('Austin', 'New York', 'San Francisco') +seq = ("Austin", "New York", "San Francisco") for item in seq: - do_stuff() - do_some_other_stuff() - # ... - # Wait, what's `item` for again? - dispatch(item) + # do_stuff() + # do_some_other_stuff() + + # Wait, what's `item` again? + print(item) ``` **Good**: + ```python -locations = ('Austin', 'New York', 'San Francisco') +locations = ("Austin", "New York", "San Francisco") for location in locations: - do_stuff() - do_some_other_stuff() + # do_stuff() + # do_some_other_stuff() # ... - dispatch(location) + print(location) ``` -**[⬆ back to top](#table-of-contents)** +**[⬆ back to top](#table-of-contents)** ### Don't add unneeded context @@ -195,49 +255,154 @@ class Car: Why write: ```python +import hashlib + + def create_micro_brewery(name): name = "Hipster Brew Co." if name is None else name slug = hashlib.sha1(name.encode()).hexdigest() # etc. ``` -... when you can specify a default argument instead? This also makes ist clear that -you are expecting a string as the argument. +... when you can specify a default argument instead? This also makes it clear +that you are expecting a string as the argument. **Good**: ```python -def create_micro_brewery(name: str="Hipster Brew Co."): +import hashlib + + +def create_micro_brewery(name: str = "Hipster Brew Co."): slug = hashlib.sha1(name.encode()).hexdigest() # etc. ``` **[⬆ back to top](#table-of-contents)** + ## **Functions** + +### Functions should do one thing + +This is by far the most important rule in software engineering. When functions +do more than one thing, they are harder to compose, test, and reason about. +When you can isolate a function to just one action, they can be refactored +easily and your code will read much cleaner. If you take nothing else away from +this guide other than this, you'll be ahead of many developers. + +**Bad:** + +```python +from typing import List + + +class Client: + active: bool + + +def email(client: Client) -> None: + pass + + +def email_clients(clients: List[Client]) -> None: + """Filter active clients and send them an email. + """ + for client in clients: + if client.active: + email(client) +``` + +**Good**: + +```python +from typing import List + + +class Client: + active: bool + + +def email(client: Client) -> None: + pass + + +def get_active_clients(clients: List[Client]) -> List[Client]: + """Filter active clients. + """ + return [client for client in clients if client.active] + + +def email_clients(clients: List[Client]) -> None: + """Send an email to a given list of clients. + """ + for client in get_active_clients(clients): + email(client) +``` + +Do you see an opportunity for using generators now? + +**Even better** + +```python +from typing import Generator, Iterator + + +class Client: + active: bool + + +def email(client: Client): + pass + + +def active_clients(clients: Iterator[Client]) -> Generator[Client, None, None]: + """Only active clients""" + return (client for client in clients if client.active) + + +def email_client(clients: Iterator[Client]) -> None: + """Send an email to a given list of clients. + """ + for client in active_clients(clients): + email(client) +``` + +**[⬆ back to top](#table-of-contents)** + ### Function arguments (2 or fewer ideally) -Limiting the amount of function parameters is incredibly important because it makes -testing your function easier. Having more than three leads to a combinatorial explosion -where you have to test tons of different cases with each separate argument. -Zero arguments is the ideal case. One or two arguments is ok, and three should be avoided. -Anything more than that should be consolidated. Usually, if you have more than two -arguments then your function is trying to do too much. In cases where it's not, most -of the time a higher-level object will suffice as an argument. +A large amount of parameters is usually the sign that a function is +doing too much (has more than one responsibility). Try to decompose it +into smaller functions having a reduced set of parameters, ideally less than +three. + +If the function has a single responsibility, consider if you can bundle +some or all parameters into a specialized object that will be passed as an +argument to the function. These parameters might be attributes of a single +entity that you can represent with a dedicated data structure. You may also +be able to reuse this entity elsewhere in your program. The reason why this is +a better arrangement is than having multiple parameters is that we may be able +to move some computations, done with those parameters inside the +function, into methods belonging to the new object, therefore reducing the +complexity of the function. **Bad:** + ```python def create_menu(title, body, button_text, cancellable): - # ... + pass ``` -**Good**: +**Java-esque**: + ```python class Menu: def __init__(self, config: dict): - title = config["title"] - body = config["body"] + self.title = config["title"] + self.body = config["body"] # ... + menu = Menu( { "title": "My Menu", @@ -249,6 +414,7 @@ menu = Menu( ``` **Also good** + ```python class MenuConfig: """A configuration for the Menu. @@ -265,13 +431,13 @@ class MenuConfig: cancellable: bool = False -def create_menu(config: MenuConfig): +def create_menu(config: MenuConfig) -> None: title = config.title body = config.body # ... -config = MenuConfig +config = MenuConfig() config.title = "My delicious menu" config.body = "A description of the various items on the menu" config.button_text = "Order now!" @@ -282,6 +448,7 @@ create_menu(config) ``` **Fancy** + ```python from typing import NamedTuple @@ -303,6 +470,7 @@ class MenuConfig(NamedTuple): def create_menu(config: MenuConfig): title, body, button_text, cancellable = config + # ... create_menu( @@ -314,58 +482,78 @@ create_menu( ) ``` -**[⬆ back to top](#table-of-contents)** - -### Functions should do one thing -This is by far the most important rule in software engineering. When functions do more -than one thing, they are harder to compose, test, and reason about. When you can isolate -a function to just one action, they can be refactored easily and your code will read much -cleaner. If you take nothing else away from this guide other than this, you'll be ahead -of many developers. +**Even fancier** -**Bad:** ```python +from dataclasses import astuple, dataclass -def email_clients(clients: List[Client]): - """Filter active clients and send them an email. - """ - for client in clients: - if client.active: - email(client) -``` -**Good**: -```python -def get_active_clients(clients: List[Client]) -> List[Client]: - """Filter active clients. +@dataclass +class MenuConfig: + """A configuration for the Menu. + + Attributes: + title: The title of the Menu. + body: The body of the Menu. + button_text: The text for the button label. + cancellable: Can it be cancelled? """ - return [client for client in clients if client.active] + title: str + body: str + button_text: str + cancellable: bool = False -def email_clients(clients: List[Client, ...]) -> None: - """Send an email to a given list of clients. - """ - for client in clients: - email(client) +def create_menu(config: MenuConfig): + title, body, button_text, cancellable = astuple(config) + # ... + + +create_menu( + MenuConfig( + title="My delicious menu", + body="A description of the various items on the menu", + button_text="Order now!" + ) +) ``` -Do you see an opportunity for using generators now? +**Even fancier, Python3.8+ only** -**Even better** ```python -def active_clients(clients: List[Client]) -> Generator[Client]: - """Only active clients. - """ - return (client for client in clients if client.active) +from typing import TypedDict -def email_client(clients: Iterator[Client]) -> None: - """Send an email to a given list of clients. +class MenuConfig(TypedDict): + """A configuration for the Menu. + + Attributes: + title: The title of the Menu. + body: The body of the Menu. + button_text: The text for the button label. + cancellable: Can it be cancelled? """ - for client in clients: - email(client) -``` + title: str + body: str + button_text: str + cancellable: bool + + +def create_menu(config: MenuConfig): + title = config["title"] + # ... + +create_menu( + # You need to supply all the parameters + MenuConfig( + title="My delicious menu", + body="A description of the various items on the menu", + button_text="Order now!", + cancellable=True + ) +) +``` **[⬆ back to top](#table-of-contents)** @@ -376,7 +564,8 @@ def email_client(clients: Iterator[Client]) -> None: ```python class Email: def handle(self) -> None: - # Do something... + pass + message = Email() # What is this supposed to do again? @@ -388,8 +577,8 @@ message.handle() ```python class Email: def send(self) -> None: - """Send this message. - """ + """Send this message""" + message = Email() message.send() @@ -399,1407 +588,1024 @@ message.send() ### Functions should only be one level of abstraction -When you have more than one level of abstraction your function is usually -doing too much. Splitting up functions leads to reusability and easier -testing. +When you have more than one level of abstraction, your function is usually +doing too much. Splitting up functions leads to reusability and easier testing. **Bad:** -```php -function parseBetterJSAlternative($code) -{ - $regexes = [ - // ... - ]; - - $statements = split(' ', $code); - $tokens = []; - foreach ($regexes as $regex) { - foreach ($statements as $statement) { - // ... - } - } - - $ast = []; - foreach ($tokens as $token) { - // lex... - } - - foreach ($ast as $node) { - // parse... - } -} -``` +```python +# type: ignore -**Bad too:** - -We have carried out some of the functionality, but the `parseBetterJSAlternative()` function is still very complex and not testable. - -```php -function tokenize($code) -{ - $regexes = [ - // ... - ]; - - $statements = split(' ', $code); - $tokens = []; - foreach ($regexes as $regex) { - foreach ($statements as $statement) { - $tokens[] = /* ... */; - } - } - - return $tokens; -} - -function lexer($tokens) -{ - $ast = []; - foreach ($tokens as $token) { - $ast[] = /* ... */; - } - - return $ast; -} - -function parseBetterJSAlternative($code) -{ - $tokens = tokenize($code); - $ast = lexer($tokens); - foreach ($ast as $node) { - // parse... - } -} +def parse_better_js_alternative(code: str) -> None: + regexes = [ + # ... + ] + + statements = code.split('\n') + tokens = [] + for regex in regexes: + for statement in statements: + pass + + ast = [] + for token in tokens: + pass + + for node in ast: + pass ``` **Good:** -The best solution is move out the dependencies of `parseBetterJSAlternative()` function. +```python +from typing import Tuple, List, Dict -```php -class Tokenizer -{ - public function tokenize($code) - { - $regexes = [ - // ... - ]; - - $statements = split(' ', $code); - $tokens = []; - foreach ($regexes as $regex) { - foreach ($statements as $statement) { - $tokens[] = /* ... */; - } - } - - return $tokens; - } -} -``` +REGEXES: Tuple = ( + # ... +) -```php -class Lexer -{ - public function lexify($tokens) - { - $ast = []; - foreach ($tokens as $token) { - $ast[] = /* ... */; - } - return $ast; - } -} -``` +def parse_better_js_alternative(code: str) -> None: + tokens: List = tokenize(code) + syntax_tree: List = parse(tokens) -```php -class BetterJSAlternative -{ - private $tokenizer; - private $lexer; + for node in syntax_tree: + pass - public function __construct(Tokenizer $tokenizer, Lexer $lexer) - { - $this->tokenizer = $tokenizer; - $this->lexer = $lexer; - } - public function parse($code) - { - $tokens = $this->tokenizer->tokenize($code); - $ast = $this->lexer->lexify($tokens); - foreach ($ast as $node) { - // parse... - } - } -} -``` +def tokenize(code: str) -> List: + statements = code.split() + tokens: List[Dict] = [] + for regex in REGEXES: + for statement in statements: + pass -Now we can mock the dependencies and test only the work of method `BetterJSAlternative::parse()`. + return tokens -**[⬆ back to top](#table-of-contents)** -### Don't use flags as function parameters -Flags tell your user that this function does more than one thing. Functions should -do one thing. Split out your functions if they are following different code paths -based on a boolean. +def parse(tokens: List) -> List: + syntax_tree: List[Dict] = [] + for token in tokens: + pass -**Bad:** -```php -function createFile($name, $temp = false) { - if ($temp) { - touch('./temp/'.$name); - } else { - touch($name); - } -} + return syntax_tree ``` -**Good**: -```php -function createFile($name) { - touch($name); -} - -function createTempFile($name) { - touch('./temp/'.$name); -} -``` **[⬆ back to top](#table-of-contents)** -### Avoid Side Effects -A function produces a side effect if it does anything other than take a value in and -return another value or values. A side effect could be writing to a file, modifying -some global variable, or accidentally wiring all your money to a stranger. - -Now, you do need to have side effects in a program on occasion. Like the previous -example, you might need to write to a file. What you want to do is to centralize where -you are doing this. Don't have several functions and classes that write to a particular -file. Have one service that does it. One and only one. +### Don't use flags as function parameters -The main point is to avoid common pitfalls like sharing state between objects without -any structure, using mutable data types that can be written to by anything, and not -centralizing where your side effects occur. If you can do this, you will be happier -than the vast majority of other programmers. +Flags tell your user that this function does more than one thing. Functions +should do one thing. Split your functions if they are following different code +paths based on a boolean. **Bad:** -```php -// Global variable referenced by following function. -// If we had another function that used this name, now it'd be an array and it could break it. -$name = 'Ryan McDermott'; -function splitIntoFirstAndLastName() { - global $name; - - $name = preg_split('/ /', $name); -} +```python +from tempfile import gettempdir +from pathlib import Path -splitIntoFirstAndLastName(); -var_dump($name); // ['Ryan', 'McDermott']; +def create_file(name: str, temp: bool) -> None: + if temp: + (Path(gettempdir()) / name).touch() + else: + Path(name).touch() ``` -**Good**: -```php -function splitIntoFirstAndLastName($name) { - return preg_split('/ /', $name); -} +**Good:** -$name = 'Ryan McDermott'; -$newName = splitIntoFirstAndLastName($name); +```python +from tempfile import gettempdir +from pathlib import Path -var_dump($name); // 'Ryan McDermott'; -var_dump($newName); // ['Ryan', 'McDermott']; -``` -**[⬆ back to top](#table-of-contents)** -### Don't write to global functions -Polluting globals is a bad practice in many languages because you could clash with another -library and the user of your API would be none-the-wiser until they get an exception in -production. Let's think about an example: what if you wanted to have configuration array. -You could write global function like `config()`, but it could clash with another library -that tried to do the same thing. +def create_file(name: str) -> None: + Path(name).touch() -**Bad:** -```php -function config() -{ - return [ - 'foo' => 'bar', - ] -} +def create_temp_file(name: str) -> None: + (Path(gettempdir()) / name).touch() ``` -**Good:** +**[⬆ back to top](#table-of-contents)** -Create PHP configuration file or something else +### Avoid side effects -```php -// config.php -return [ - 'foo' => 'bar', -]; -``` +A function produces a side effect if it does anything other than take a value +in and return another value or values. For example, a side effect could be +writing to a file, modifying some global variable, or accidentally wiring all +your money to a stranger. -```php -class Configuration -{ - private $configuration = []; +Now, you do need to have side effects in a program on occasion - for example, +like in the previous example, you might need to write to a file. In these +cases, you should centralize and indicate where you are incorporating side +effects. Don't have several functions and classes that write to a particular +file - rather, have one +(and only one) service that does it. - public function __construct(array $configuration) - { - $this->configuration = $configuration; - } +The main point is to avoid common pitfalls like sharing state between objects +without any structure, using mutable data types that can be written to by +anything, or using an instance of a class, and not centralizing where your side +effects occur. If you can do this, you will be happier than the vast majority +of other programmers. - public function get($key) - { - return isset($this->configuration[$key]) ? $this->configuration[$key] : null; - } -} -``` +**Bad:** -Load configuration from file and create instance of `Configuration` class +```python +# type: ignore -```php -$configuration = new Configuration($configuration); -``` +# This is a module-level name. +# It's good practice to define these as immutable values, such as a string. +# However... +fullname = "Ryan McDermott" -And now you must use instance of `Configuration` in your application. -**[⬆ back to top](#table-of-contents)** +def split_into_first_and_last_name() -> None: + # The use of the global keyword here is changing the meaning of the + # the following line. This function is now mutating the module-level + # state and introducing a side-effect! + global fullname + fullname = fullname.split() -### Don't use a Singleton pattern -Singleton is a [anti-pattern](https://en.wikipedia.org/wiki/Singleton_pattern). -**Bad:** +split_into_first_and_last_name() -```php -class DBConnection -{ - private static $instance; +# MyPy will spot the problem, complaining about 'Incompatible types in +# assignment: (expression has type "List[str]", variable has type "str")' +print(fullname) # ["Ryan", "McDermott"] - private function __construct($dsn) - { - // ... - } +# OK. It worked the first time, but what will happen if we call the +# function again? +``` - public static function getInstance() - { - if (self::$instance === null) { - self::$instance = new self(); - } +**Good:** - return self::$instance; - } +```python +from typing import List, AnyStr - // ... -} -$singleton = DBConnection::getInstance(); -``` +def split_into_first_and_last_name(name: AnyStr) -> List[AnyStr]: + return name.split() -**Good:** -```php -class DBConnection -{ - public function __construct(array $dsn) - { - // ... - } +fullname = "Ryan McDermott" +name, surname = split_into_first_and_last_name(fullname) - // ... -} +print(name, surname) # => Ryan McDermott ``` -Create instance of `DBConnection` class and configure it with [DSN](http://php.net/manual/en/pdo.construct.php#refsect1-pdo.construct-parameters). +**Also good** -```php -$connection = new DBConnection($dsn); -``` +```python +from dataclasses import dataclass -And now you must use instance of `DBConnection` in your application. -**[⬆ back to top](#table-of-contents)** +@dataclass +class Person: + name: str -### Encapsulate conditionals + @property + def name_as_first_and_last(self) -> list: + return self.name.split() -**Bad:** -```php -if ($article->state === 'published') { - // ... -} +# The reason why we create instances of classes is to manage state! +person = Person("Ryan McDermott") +print(person.name) # => "Ryan McDermott" +print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] ``` -**Good**: +**[⬆ back to top](#table-of-contents)** -```php -if ($article->isPublished()) { - // ... -} -``` +## **Classes** -**[⬆ back to top](#table-of-contents)** +### **Single Responsibility Principle (SRP)** -### Avoid negative conditionals +Robert C. Martin writes: -**Bad:** -```php -function isDOMNodeNotPresent($node) { - // ... -} - -if (!isDOMNodeNotPresent($node)) { - // ... -} -``` +> A class should have only one reason to change. -**Good**: -```php -function isDOMNodePresent($node) { - // ... -} - -if (isDOMNodePresent($node)) { - // ... -} -``` -**[⬆ back to top](#table-of-contents)** +"Reasons to change" are, in essence, the responsibilities managed by a class or +function. -### Avoid conditionals -This seems like an impossible task. Upon first hearing this, most people say, -"how am I supposed to do anything without an `if` statement?" The answer is that -you can use polymorphism to achieve the same task in many cases. The second -question is usually, "well that's great but why would I want to do that?" The -answer is a previous clean code concept we learned: a function should only do -one thing. When you have classes and functions that have `if` statements, you -are telling your user that your function does more than one thing. Remember, -just do one thing. +In the following example, we create an HTML element that represents a comment +with the version of the document: -**Bad:** -```php -class Airplane { - // ... - public function getCruisingAltitude() { - switch ($this->type) { - case '777': - return $this->getMaxAltitude() - $this->getPassengerCount(); - case 'Air Force One': - return $this->getMaxAltitude(); - case 'Cessna': - return $this->getMaxAltitude() - $this->getFuelExpenditure(); - } - } -} -``` +**Bad** -**Good**: -```php -class Airplane { - // ... -} - -class Boeing777 extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude() - $this->getPassengerCount(); - } -} +```python +from importlib import metadata -class AirForceOne extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude(); - } -} -class Cessna extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude() - $this->getFuelExpenditure(); - } -} -``` -**[⬆ back to top](#table-of-contents)** +class VersionCommentElement: + """An element that renders an HTML comment with the program's version number + """ -### Avoid type-checking (part 1) + def get_version(self) -> str: + """Get the package version""" + return metadata.version("pip") -PHP is untyped, which means your functions can take any type of argument. -Sometimes you are bitten by this freedom and it becomes tempting to do -type-checking in your functions. There are many ways to avoid having to do this. -The first thing to consider is consistent APIs. + def render(self) -> None: + print(f'') -**Bad:** -```php -function travelToTexas($vehicle) -{ - if ($vehicle instanceof Bicycle) { - $vehicle->peddleTo(new Location('texas')); - } elseif ($vehicle instanceof Car) { - $vehicle->driveTo(new Location('texas')); - } -} +VersionCommentElement().render() ``` -**Good**: +This class has two responsibilities: -```php -function travelToTexas(Traveler $vehicle) -{ - $vehicle->travelTo(new Location('texas')); -} -``` +- Retrieve the version number of the Python package +- Render itself as an HTML element -**[⬆ back to top](#table-of-contents)** +Any change to one or the other carries the risk of impacting the other. -### Avoid type-checking (part 2) +We can rewrite the class and decouple these responsibilities: -If you are working with basic primitive values like strings, integers, and arrays, -and you use PHP 7+ and you can't use polymorphism but you still feel the need to -type-check, you should consider -[type declaration](http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration) -or strict mode. It provides you with static typing on top of standard PHP syntax. -The problem with manually type-checking is that doing it well requires so much -extra verbiage that the faux "type-safety" you get doesn't make up for the lost -readability. Keep your PHP clean, write good tests, and have good code reviews. -Otherwise, do all of that but with PHP strict type declaration or strict mode. +**Good** -**Bad:** +```python +from importlib import metadata -```php -function combine($val1, $val2) -{ - if (!is_numeric($val1) || !is_numeric($val2)) { - throw new \Exception('Must be of type Number'); - } - return $val1 + $val2; -} -``` +def get_version(pkg_name: str) -> str: + """Retrieve the version of a given package""" + return metadata.version(pkg_name) -**Good**: -```php -function combine(int $val1, int $val2) -{ - return $val1 + $val2; -} +class VersionCommentElement: + """An element that renders an HTML comment with the program's version number + """ + + def __init__(self, version: str): + self.version = version + + def render(self) -> None: + print(f'') + + +VersionCommentElement(get_version("pip")).render() ``` -**[⬆ back to top](#table-of-contents)** +The result is that the class only needs to take care of rendering itself. It +receives the version text during instantiation and this text is generated by +calling a separate function, `get_version()`. Changing the class has no impact +on the other, and vice-versa, as long as the contract between them does not +change, i.e. the function provides a string and the class `__init__` method +accepts a string. -### Remove dead code -Dead code is just as bad as duplicate code. There's no reason to keep it in -your codebase. If it's not being called, get rid of it! It will still be safe -in your version history if you still need it. +As an added bonus, the `get_version()` is now reusable elsewhere. -**Bad:** -```php -function oldRequestModule($url) { - // ... -} +### **Open/Closed Principle (OCP)** -function newRequestModule($url) { - // ... -} +> “Incorporate new features by extending the system, not by making +> modifications (to it)”, +> Uncle Bob. -$request = newRequestModule($requestUrl); -inventoryTracker('apples', $request, 'www.inventory-awesome.io'); +Objects should be open for extension, but closed to modification. It should be +possible to augment the functionality provided by an object (for example, a +class) +without changing its internal contracts. An object can enable this when it is +designed to be extended cleanly. -``` +In the following example, we try to implement a simple web framework that +handles HTTP requests and returns responses. The `View` class has a single +method `.get()` that will be called when the HTTP server will receive a GET +request from a client. -**Good**: -```php -function requestModule($url) { - // ... -} +`View` is intentionally simple and returns `text/plain` responses. We would +also like to return HTML responses based on a template file, so we subclass it +using the `TemplateView` class. -$request = requestModule($requestUrl); -inventoryTracker('apples', $request, 'www.inventory-awesome.io'); -``` -**[⬆ back to top](#table-of-contents)** +**Bad** +```python +from dataclasses import dataclass -## **Objects and Data Structures** -### Use getters and setters -In PHP you can set `public`, `protected` and `private` keywords for methods. -Using it, you can control properties modification on an object. -* When you want to do more beyond getting an object property, you don't have -to look up and change every accessor in your codebase. -* Makes adding validation simple when doing a `set`. -* Encapsulates the internal representation. -* Easy to add logging and error handling when getting and setting. -* Inheriting this class, you can override default functionality. -* You can lazy load your object's properties, let's say getting it from a -server. +@dataclass +class Response: + """An HTTP response""" -Additionally, this is part of Open/Closed principle, from object-oriented -design principles. + status: int + content_type: str + body: str + + +class View: + """A simple view that returns plain text responses""" + + def get(self, request) -> Response: + """Handle a GET request and return a message in the response""" + return Response( + status=200, + content_type='text/plain', + body="Welcome to my web site" + ) -**Bad:** -```php -class BankAccount { - public $balance = 1000; -} -$bankAccount = new BankAccount(); +class TemplateView(View): + """A view that returns HTML responses based on a template file.""" + + def get(self, request) -> Response: + """Handle a GET request and return an HTML document in the response""" + with open("index.html") as fd: + return Response( + status=200, + content_type='text/html', + body=fd.read() + ) -// Buy shoes... -$bankAccount->balance -= 100; ``` -**Good**: -```php -class BankAccount { - private $balance; - - public function __construct($balance = 1000) { - $this->balance = $balance; - } - - public function withdrawBalance($amount) { - if ($amount > $this->balance) { - throw new \Exception('Amount greater than available balance.'); - } - $this->balance -= $amount; - } - - public function depositBalance($amount) { - $this->balance += $amount; - } - - public function getBalance() { - return $this->balance; - } -} +The `TemplateView` class has modified the internal behaviour of its parent +class in order to enable the more advanced functionality. In doing so, it now +relies on the `View` to not change the implementation of the `.get()` +method, which now needs to be frozen in time. We cannot introduce, for example, +some additional checks in all our `View`-derived classes because the behaviour +is overridden in at least one subtype and we will need to update it. -$bankAccount = new BankAccount(); +Let's redesign our classes to fix this problem and let the `View` class be +extended (not modified) cleanly: -// Buy shoes... -$bankAccount->withdrawBalance($shoesPrice); +**Good** -// Get balance -$balance = $bankAccount->getBalance(); +```python +from dataclasses import dataclass -``` -**[⬆ back to top](#table-of-contents)** +@dataclass +class Response: + """An HTTP response""" -### Make objects have private/protected members + status: int + content_type: str + body: str -**Bad:** -```php -class Employee { - public $name; - - public function __construct($name) { - $this->name = $name; - } -} -$employee = new Employee('John Doe'); -echo 'Employee name: '.$employee->name; // Employee name: John Doe -``` +class View: + """A simple view that returns plain text responses""" -**Good**: -```php -class Employee { - protected $name; - - public function __construct($name) { - $this->name = $name; - } - - public function getName() { - return $this->name; - } -} + content_type = "text/plain" -$employee = new Employee('John Doe'); -echo 'Employee name: '.$employee->getName(); // Employee name: John Doe -``` -**[⬆ back to top](#table-of-contents)** + def render_body(self) -> str: + """Render the message body of the response""" + return "Welcome to my web site" + def get(self, request) -> Response: + """Handle a GET request and return a message in the response""" + return Response( + status=200, + content_type=self.content_type, + body=self.render_body() + ) -## **Classes** -### Single Responsibility Principle (SRP) -As stated in Clean Code, "There should never be more than one reason for a class -to change". It's tempting to jam-pack a class with a lot of functionality, like -when you can only take one suitcase on your flight. The issue with this is -that your class won't be conceptually cohesive and it will give it many reasons -to change. Minimizing the amount of times you need to change a class is important. -It's important because if too much functionality is in one class and you modify a piece of it, -it can be difficult to understand how that will affect other dependent modules in -your codebase. +class TemplateView(View): + """A view that returns HTML responses based on a template file.""" + + content_type = "text/html" + template_file = "index.html" + + def render_body(self) -> str: + """Render the message body as HTML""" + with open(self.template_file) as fd: + return fd.read() -**Bad:** -```php -class UserSettings { - private $user; - public function __construct($user) { - $this->user = $user; - } - - public function changeSettings($settings) { - if ($this->verifyCredentials()) { - // ... - } - } - - private function verifyCredentials() { - // ... - } -} ``` -**Good:** -```php -class UserAuth { - private $user; +Note that we did need to override the `render_body()` in order to change the +source of the body, but this method has a single, well defined responsibility +that **invites subtypes to override it**. It is designed to be extended by its +subtypes. - public function __construct($user) { - $this->user = $user; - } - - public function verifyCredentials() { - // ... - } -} +Another good way to use the strengths of both object inheritance and object +composition is to +use [Mixins](https://docs.djangoproject.com/en/4.1/topics/class-based-views/mixins/) +. +Mixins are bare-bones classes that are meant to be used exclusively with other +related classes. They are "mixed-in" with the target class using multiple +inheritance, in order to change the target's behaviour. -class UserSettings { - private $user; +A few rules: - public function __construct($user) { - $this->user = $user; - $this->auth = new UserAuth($user); - } - - public function changeSettings($settings) { - if ($this->auth->verifyCredentials()) { - // ... - } - } -} -``` -**[⬆ back to top](#table-of-contents)** +- Mixins should always inherit from `object` +- Mixins always come before the target class, + e.g. `class Foo(MixinA, MixinB, TargetClass): ...` -### Open/Closed Principle (OCP) +**Also good** -As stated by Bertrand Meyer, "software entities (classes, modules, functions, -etc.) should be open for extension, but closed for modification." What does that -mean though? This principle basically states that you should allow users to -add new functionalities without changing existing code. +```python +from dataclasses import dataclass, field +from typing import Protocol -**Bad:** -```php -abstract class Adapter -{ - protected $name; +@dataclass +class Response: + """An HTTP response""" - public function getName() - { - return $this->name; - } -} + status: int + content_type: str + body: str + headers: dict = field(default_factory=dict) -class AjaxAdapter extends Adapter -{ - public function __construct() - { - parent::__construct(); - $this->name = 'ajaxAdapter'; - } -} -class NodeAdapter extends Adapter -{ - public function __construct() - { - parent::__construct(); - $this->name = 'nodeAdapter'; - } -} +class View: + """A simple view that returns plain text responses""" -class HttpRequester -{ - private $adapter; + content_type = "text/plain" - public function __construct($adapter) - { - $this->adapter = $adapter; - } - - public function fetch($url) - { - $adapterName = $this->adapter->getName(); + def render_body(self) -> str: + """Render the message body of the response""" + return "Welcome to my web site" - if ($adapterName === 'ajaxAdapter') { - return $this->makeAjaxCall($url); - } elseif ($adapterName === 'httpNodeAdapter') { - return $this->makeHttpCall($url); - } - } - - protected function makeAjaxCall($url) - { - // request and return promise - } - - protected function makeHttpCall($url) - { - // request and return promise - } -} -``` + def get(self, request) -> Response: + """Handle a GET request and return a message in the response""" + return Response( + status=200, + content_type=self.content_type, + body=self.render_body() + ) -**Good:** -```php -interface Adapter -{ - public function request($url); -} +class TemplateRenderMixin: + """A mixin class for views that render HTML documents using a template file + + Not to be used by itself! + """ + template_file: str = "" -class AjaxAdapter implements Adapter -{ - public function request($url) - { - // request and return promise - } -} + def render_body(self) -> str: + """Render the message body as HTML""" + if not self.template_file: + raise ValueError("The path to a template file must be given.") -class NodeAdapter implements Adapter -{ - public function request($url) - { - // request and return promise - } -} + with open(self.template_file) as fd: + return fd.read() -class HttpRequester -{ - private $adapter; - public function __construct(Adapter $adapter) - { - $this->adapter = $adapter; - } - - public function fetch($url) - { - return $this->adapter->request($url); - } -} +class ContentLengthMixin: + """A mixin class for views that injects a Content-Length header in the + response + + Not to be used by itself! + """ + + def get(self, request) -> Response: + """Introspect and amend the response to inject the new header""" + response = super().get(request) # type: ignore + response.headers['Content-Length'] = len(response.body) + return response + + +class TemplateView(TemplateRenderMixin, ContentLengthMixin, View): + """A view that returns HTML responses based on a template file.""" + + content_type = "text/html" + template_file = "index.html" + ``` -**[⬆ back to top](#table-of-contents)** +As you can see, Mixins make object composition easier by packaging together +related functionality into a highly reusable class with a single +responsibility, allowing clean decoupling. Class extension is achieved by " +mixing-in" the additional classes. -### Liskov Substitution Principle (LSP) +The popular Django project makes heavy use of Mixins to compose its class-based +views. -This is a scary term for a very simple concept. It's formally defined as "If S -is a subtype of T, then objects of type T may be replaced with objects of type S -(i.e., objects of type S may substitute objects of type T) without altering any -of the desirable properties of that program (correctness, task performed, -etc.)." That's an even scarier definition. +FIXME: re-enable typechecking for the line above once it's clear how to use +`typing.Protocol` to make the type checker work with Mixins. -The best explanation for this is if you have a parent class and a child class, -then the base class and child class can be used interchangeably without getting -incorrect results. This might still be confusing, so let's take a look at the -classic Square-Rectangle example. Mathematically, a square is a rectangle, but -if you model it using the "is-a" relationship via inheritance, you quickly -get into trouble. +### **Liskov Substitution Principle (LSP)** -**Bad:** +> “Functions that use pointers or references to base classes +> must be able to use objects of derived classes without knowing it”, +> Uncle Bob. -```php -class Rectangle -{ - protected $width; - protected $height; +This principle is named after Barbara Liskov, who collaborated with fellow +computer scientist Jeannette Wing on the seminal paper +*"A behavioral notion of subtyping" (1994). A core tenet of the paper is that +"a subtype (must) preserve the behaviour of the supertype methods and also all +invariant and history properties of its supertype". - public function __construct() - { - $this->width = 0; - $this->height = 0; - } +In essence, a function accepting a supertype should also accept all its +subtypes with no modification. - public function render($area) - { - // ... - } +Can you spot the problem with the following code? - public function setWidth($width) - { - $this->width = $width; - } +**Bad** - public function setHeight($height) - { - $this->height = $height; - } +```python +from dataclasses import dataclass - public function getArea() - { - return $this->width * $this->height; - } -} -class Square extends Rectangle -{ - public function setWidth($width) - { - $this->width = $this->height = $width; - } +@dataclass +class Response: + """An HTTP response""" - public function setHeight(height) - { - $this->width = $this->height = $height; - } -} - -function renderLargeRectangles($rectangles) -{ - foreach ($rectangles as $rectangle) { - $rectangle->setWidth(4); - $rectangle->setHeight(5); - $area = $rectangle->getArea(); // BAD: Will return 25 for Square. Should be 20. - $rectangle->render($area); - } -} + status: int + content_type: str + body: str -$rectangles = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles($rectangles); -``` -**Good:** +class View: + """A simple view that returns plain text responses""" -```php -abstract class Shape -{ - protected $width; - protected $height; + content_type = "text/plain" - abstract public function getArea(); + def render_body(self) -> str: + """Render the message body of the response""" + return "Welcome to my web site" - public function render($area) - { - // ... - } -} + def get(self, request) -> Response: + """Handle a GET request and return a message in the response""" + return Response( + status=200, + content_type=self.content_type, + body=self.render_body() + ) -class Rectangle extends Shape -{ - public function __construct() - { - parent::__construct(); - $this->width = 0; - $this->height = 0; - } - public function setWidth($width) - { - $this->width = $width; - } +class TemplateView(View): + """A view that returns HTML responses based on a template file.""" - public function setHeight($height) - { - $this->height = $height; - } + content_type = "text/html" - public function getArea() - { - return $this->width * $this->height; - } -} + def get(self, request, template_file: str) -> Response: # type: ignore + """Render the message body as HTML""" + with open(template_file) as fd: + return Response( + status=200, + content_type=self.content_type, + body=fd.read() + ) -class Square extends Shape -{ - public function __construct() - { - parent::__construct(); - $this->length = 0; - } - public function setLength($length) - { - $this->length = $length; - } +def render(view: View, request) -> Response: + """Render a View""" + return view.get(request) - public function getArea() - { - return pow($this->length, 2); - } -} - -function renderLargeRectangles($rectangles) -{ - foreach ($rectangles as $rectangle) { - if ($rectangle instanceof Square) { - $rectangle->setLength(5); - } elseif ($rectangle instanceof Rectangle) { - $rectangle->setWidth(4); - $rectangle->setHeight(5); - } - - $area = $rectangle->getArea(); - $rectangle->render($area); - } -} +``` + +The expectation is that `render()` function will be able to work with `View` +and its subtype `TemplateView`, but the latter has broken compatibility by +modifying the signature of the `.get()` method. The function will raise +a `TypeError` +exception when used with `TemplateView`. -$shapes = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles($shapes); +If we want the `render()` function to work with any subtype of `View`, we must +pay attention not to break its public-facing protocol. But how do we know what +constitutes it for a given class? Type hinters like *mypy* will raise an error +when it detects mistakes like this: + +``` +error: Signature of "get" incompatible with supertype "View" +:36: note: Superclass: +:36: note: def get(self, request: Any) -> Response +:36: note: Subclass: +:36: note: def get(self, request: Any, template_file: str) -> Response ``` -**[⬆ back to top](#table-of-contents)** +### **Interface Segregation Principle (ISP)** -### Interface Segregation Principle (ISP) -ISP states that "Clients should not be forced to depend upon interfaces that -they do not use." +> “Keep interfaces small +> so that users don’t end up depending on things they don’t need.”, +> Uncle Bob. -A good example to look at that demonstrates this principle is for -classes that require large settings objects. Not requiring clients to setup -huge amounts of options is beneficial, because most of the time they won't need -all of the settings. Making them optional helps prevent having a "fat interface". +Several well known object oriented programming languages, like Java and Go, +have a concept called interfaces. An interface defines the public methods and +properties of an object without implementing them. They are useful when we +don't want to couple the signature of a function to a concrete object; we'd +rather say "I don't care what object you give me, as long as it has certain +methods and attributes I expect to make use of". -**Bad:** -```php -interface WorkerInterface { - public function work(); - public function eat(); -} - -class Worker implements WorkerInterface { - public function work() { - // ....working - } - public function eat() { - // ...... eating in launch break - } -} +Python does not have interfaces. We have Abstract Base Classes instead, which +are a little different, but can serve the same purpose. -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } +**Good** - public function eat() { - //.... eating in launch break - } -} - -class Manager { - /** @var WorkerInterface $worker **/ - private $worker; - - public function setWorker(WorkerInterface $worker) { - $this->worker = $worker; - } +```python - public function manage() { - $this->worker->work(); - } -} +from abc import ABCMeta, abstractmethod + + +# Define the Abstract Class for a generic Greeter object +class Greeter(metaclass=ABCMeta): + """An object that can perform a greeting action.""" + + @staticmethod + @abstractmethod + def greet(name: str) -> None: + """Display a greeting for the user with the given name""" + + +class FriendlyActor(Greeter): + """An actor that greets the user with a friendly salutation""" + + @staticmethod + def greet(name: str) -> None: + """Greet a person by name""" + print(f"Hello {name}!") + + +def welcome_user(user_name: str, actor: Greeter): + """Welcome a user with a given name using the provided actor""" + actor.greet(user_name) + + +welcome_user("Barbara", FriendlyActor()) ``` -**Good:** -```php -interface WorkerInterface extends FeedableInterface, WorkableInterface { -} +Now imagine the following scenario: we have a certain number of PDF documents +that we author and want to serve to our web site visitors. We are using a +Python web framework and we might be tempted to design a class to manage these +documents, so we go ahead and design a comprehensive abstract base class for +our document. -interface WorkableInterface { - public function work(); -} +**Error** -interface FeedableInterface { - public function eat(); -} +```python +import abc -class Worker implements WorkableInterface, FeedableInterface { - public function work() { - // ....working - } - public function eat() { - //.... eating in launch break - } -} +class Persistable(metaclass=abc.ABCMeta): + """Serialize a file to data and back""" -class Robot implements WorkableInterface { - public function work() { - // ....working - } -} + @property + @abc.abstractmethod + def data(self) -> bytes: + """The raw data of the file""" -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } + @classmethod + @abc.abstractmethod + def load(cls, name: str): + """Load the file from disk""" - public function eat() { - //.... eating in launch break - } -} + @abc.abstractmethod + def save(self) -> None: + """Save the file to disk""" -class Manager { - /** @var $worker WorkableInterface **/ - private $worker; - public function setWorker(WorkableInterface $w) { - $this->worker = $w; - } +# We just want to serve the documents, so our concrete PDF document +# implementation just needs to implement the `.load()` method and have +# a public attribute named `data`. - public function manage() { - $this->worker->work(); - } -} -``` -**[⬆ back to top](#table-of-contents)** +class PDFDocument(Persistable): + """A PDF document""" -### Dependency Inversion Principle (DIP) -This principle states two essential things: -1. High-level modules should not depend on low-level modules. Both should -depend on abstractions. -2. Abstractions should not depend upon details. Details should depend on -abstractions. + @property + def data(self) -> bytes: + """The raw bytes of the PDF document""" + ... # Code goes here - omitted for brevity -This can be hard to understand at first, but if you've worked with PHP frameworks (like Symfony), you've seen an implementation of this principle in the form of Dependency -Injection (DI). While they are not identical concepts, DIP keeps high-level -modules from knowing the details of its low-level modules and setting them up. -It can accomplish this through DI. A huge benefit of this is that it reduces -the coupling between modules. Coupling is a very bad development pattern because -it makes your code hard to refactor. + @classmethod + def load(cls, name: str): + """Load the file from the local filesystem""" + ... # Code goes here - omitted for brevity -**Bad:** -```php -class Worker { - public function work() { - // ....working - } -} - -class Manager { - /** @var Worker $worker **/ - private $worker; - - public function __construct(Worker $worker) { - $this->worker = $worker; - } - - public function manage() { - $this->worker->work(); - } -} -class SuperWorker extends Worker { - public function work() { - //.... working much more - } -} -``` +def view(request): + """A web view that handles a GET request for a document""" + requested_name = request.qs['name'] # We want to validate this! + return PDFDocument.load(requested_name).data -**Good:** -```php -interface WorkerInterface { - public function work(); -} - -class Worker implements WorkerInterface { - public function work() { - // ....working - } -} +``` -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } -} - -class Manager { - /** @var WorkerInterface $worker **/ - private $worker; - - public function __construct(WorkerInterface $worker) { - $this->worker = $worker; - } - - public function manage() { - $this->worker->work(); - } -} +But we can't! If we don't implement the `.save()` method, an exception will be +raised: ``` -**[⬆ back to top](#table-of-contents)** +Can't instantiate abstract class PDFDocument with abstract method save. +``` -### Use method chaining -This pattern is very useful and commonly used it many libraries such -as PHPUnit and Doctrine. It allows your code to be expressive, and less verbose. -For that reason, I say, use method chaining and take a look at how clean your code -will be. In your class functions, simply return `this` at the end of every function, -and you can chain further class methods onto it. +That's annoying. We don't really need to implement `.save()` here. We could +implement a dummy method that does nothing or raises `NotImplementedError`, but +that's useless code that we will need to maintain. -**Bad:** -```php -class Car { - private $make, $model, $color; - - public function __construct() { - $this->make = 'Honda'; - $this->model = 'Accord'; - $this->color = 'white'; - } - - public function setMake($make) { - $this->make = $make; - } - - public function setModel($model) { - $this->model = $model; - } - - public function setColor($color) { - $this->color = $color; - } - - public function dump() { - var_dump($this->make, $this->model, $this->color); - } -} +At the same time, if we remove `.save()` from the abstract class now we will +need to add it back when we will later implement a way for users to submit +their documents, bringing us back to the same situation as before. -$car = new Car(); -$car->setColor('pink'); -$car->setMake('Ford'); -$car->setModel('F-150'); -$car->dump(); -``` +The problem is that we have written an *interface* that has features we don't +need right now as we are not using them. + +The solution is to decompose the interface into smaller and composable +interfaces that segregate each feature. + +**Good** + +```python +import abc + + +class DataCarrier(metaclass=abc.ABCMeta): + """Carries a data payload""" + + @property + def data(self): + ... + + +class Loadable(DataCarrier): + """Can load data from storage by name""" + + @classmethod + @abc.abstractmethod + def load(cls, name: str): + ... -**Good:** -```php -class Car { - private $make, $model, $color; - - public function __construct() { - $this->make = 'Honda'; - $this->model = 'Accord'; - $this->color = 'white'; - } - - public function setMake($make) { - $this->make = $make; - - // NOTE: Returning this for chaining - return $this; - } - - public function setModel($model) { - $this->model = $model; - - // NOTE: Returning this for chaining - return $this; - } - - public function setColor($color) { - $this->color = $color; - - // NOTE: Returning this for chaining - return $this; - } - - public function dump() { - var_dump($this->make, $this->model, $this->color); - } -} -$car = (new Car()) - ->setColor('pink') - ->setMake('Ford') - ->setModel('F-150') - ->dump(); +class Saveable(DataCarrier): + """Can save data to storage""" + + @abc.abstractmethod + def save(self) -> None: + ... + + +class PDFDocument(Loadable): + """A PDF document""" + + @property + def data(self) -> bytes: + """The raw bytes of the PDF document""" + ... # Code goes here - omitted for brevity + + @classmethod + def load(cls, name: str) -> None: + """Load the file from the local filesystem""" + ... # Code goes here - omitted for brevity + + +def view(request): + """A web view that handles a GET request for a document""" + requested_name = request.qs['name'] # We want to validate this! + return PDFDocument.load(requested_name).data + ``` -**[⬆ back to top](#table-of-contents)** -### Prefer composition over inheritance -As stated famously in [*Design Patterns*](https://en.wikipedia.org/wiki/Design_Patterns) by the Gang of Four, -you should prefer composition over inheritance where you can. There are lots of -good reasons to use inheritance and lots of good reasons to use composition. -The main point for this maxim is that if your mind instinctively goes for -inheritance, try to think if composition could model your problem better. In some -cases it can. +### **Dependency Inversion Principle (DIP)** -You might be wondering then, "when should I use inheritance?" It -depends on your problem at hand, but this is a decent list of when inheritance -makes more sense than composition: +> “Depend upon abstractions, not concrete details”, +> Uncle Bob. -1. Your inheritance represents an "is-a" relationship and not a "has-a" -relationship (Human->Animal vs. User->UserDetails). -2. You can reuse code from the base classes (Humans can move like all animals). -3. You want to make global changes to derived classes by changing a base class. -(Change the caloric expenditure of all animals when they move). +Imagine we wanted to write a web view that returns an HTTP response that +streams rows of a CSV file we create on the fly. We want to use the CSV writer +that is provided by the standard library. + +**Bad** + +```python +import csv +from io import StringIO + + +class StreamingHttpResponse: + """A streaming HTTP response""" + ... # implementation code goes here + + +def some_view(request): + rows = ( + ['First row', 'Foo', 'Bar', 'Baz'], + ['Second row', 'A', 'B', 'C', '"Testing"', "Here's a quote"] + ) + + # Define a generator to stream data directly to the client + def stream(): + buffer_ = StringIO() + writer = csv.writer(buffer_, delimiter=';', quotechar='"') + for row in rows: + writer.writerow(row) + buffer_.seek(0) + data = buffer_.read() + buffer_.seek(0) + buffer_.truncate() + yield data + + # Create the streaming response object with the appropriate CSV header. + response = StreamingHttpResponse(stream(), content_type='text/csv') + response[ + 'Content-Disposition'] = 'attachment; filename="somefilename.csv"' + + return response -**Bad:** -```php -class Employee { - private $name, $email; - - public function __construct($name, $email) { - $this->name = $name; - $this->email = $email; - } - - // ... -} - -// Bad because Employees "have" tax data. -// EmployeeTaxData is not a type of Employee - -class EmployeeTaxData extends Employee { - private $ssn, $salary; - - public function __construct($name, $email, $ssn, $salary) { - parent::__construct($name, $email); - $this->ssn = $ssn; - $this->salary = $salary; - } - - // ... -} ``` -**Good:** -```php -class EmployeeTaxData { - private $ssn, $salary; - - public function __construct($ssn, $salary) { - $this->ssn = $ssn; - $this->salary = $salary; - } - - // ... -} - -class Employee { - private $name, $email, $taxData; - - public function __construct($name, $email) { - $this->name = $name; - $this->email = $email; - } - - public function setTaxData($ssn, $salary) { - $this->taxData = new EmployeeTaxData($ssn, $salary); - } - // ... -} +Our first implementation works around the CSV's writer interface by +manipulating a `StringIO` object (which is file-like) and performing several +low level operations in order to farm out the rows from the writer. It's a lot +of work and not very elegant. + +A better way is to leverage the fact that the writer just needs an object with +a `.write()` method to do our bidding. Why not pass it a dummy object that +immediately returns the newly assembled row, so that +the `StreamingHttpResponse` +class can immediate stream it back to the client? + +**Good** + +```python +import csv + + +class Echo: + """An object that implements just the write method of the file-like + interface. + """ + + def write(self, value): + """Write the value by returning it, instead of storing in a buffer.""" + return value + + +def some_streaming_csv_view(request): + """A view that streams a large CSV file.""" + rows = ( + ['First row', 'Foo', 'Bar', 'Baz'], + ['Second row', 'A', 'B', 'C', '"Testing"', "Here's a quote"] + ) + writer = csv.writer(Echo(), delimiter=';', quotechar='"') + return StreamingHttpResponse( + (writer.writerow(row) for row in rows), + content_type="text/csv", + headers={ + 'Content-Disposition': 'attachment; filename="somefilename.csv"'}, + ) + ``` + +Much better, and it works like a charm! The reason it's superior to the +previous implementation should be obvious: less code (and more performant) to +achieve the same result. We decided to leverage the fact that the writer class +depends on the `.write()` abstraction of the object it receives, without caring +about the low level, concrete details of what the method actually does. + +This example was taken from +[a submission made to the Django documentation](https://code.djangoproject.com/ticket/21179) +by this author. + **[⬆ back to top](#table-of-contents)** -## Don’t repeat yourself (DRY) +## **Don't repeat yourself (DRY)** -Try to observe the [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) principle. +Try to observe the [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) +principle. -Do your absolute best to avoid duplicate code. Duplicate code is bad because -it means that there's more than one place to alter something if you need to -change some logic. +Do your absolute best to avoid duplicate code. Duplicate code is bad because it +means that there's more than one place to alter something if you need to change +some logic. -Imagine if you run a restaurant and you keep track of your inventory: all your -tomatoes, onions, garlic, spices, etc. If you have multiple lists that -you keep this on, then all have to be updated when you serve a dish with -tomatoes in them. If you only have one list, there's only one place to update! +Imagine if you run a restaurant and you keep track of your inventory: all your +tomatoes, onions, garlic, spices, etc. If you have multiple lists that you keep +this on, then all have to be updated when you serve a dish with tomatoes in +them. If you only have one list, there's only one place to update! -Oftentimes you have duplicate code because you have two or more slightly -different things, that share a lot in common, but their differences force you -to have two or more separate functions that do much of the same things. Removing -duplicate code means creating an abstraction that can handle this set of different -things with just one function/module/class. +Often you have duplicate code because you have two or more slightly different +things, that share a lot in common, but their differences force you to have two +or more separate functions that do much of the same things. Removing duplicate +code means creating an abstraction that can handle this set of different things +with just one function/module/class. -Getting the abstraction right is critical, that's why you should follow the -SOLID principles laid out in the [Classes](#classes) section. Bad abstractions can be -worse than duplicate code, so be careful! Having said this, if you can make -a good abstraction, do it! Don't repeat yourself, otherwise you'll find yourself -updating multiple places anytime you want to change one thing. +Getting the abstraction right is critical. Bad abstractions can be worse than +duplicate code, so be careful! Having said this, if you can make a good +abstraction, do it! Don't repeat yourself, otherwise you'll find yourself +updating multiple places any time you want to change one thing. **Bad:** -```php -function showDeveloperList($developers) -{ -    foreach ($developers as $developer) { - $expectedSalary = $developer->calculateExpectedSalary(); - $experience = $developer->getExperience(); - $githubLink = $developer->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} - -function showManagerList($managers) -{ -    foreach ($managers as $manager) { - $expectedSalary = $manager->calculateExpectedSalary(); - $experience = $manager->getExperience(); - $githubLink = $manager->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} +```python +from typing import List, Dict +from dataclasses import dataclass + + +@dataclass +class Developer: + def __init__(self, experience: float, github_link: str) -> None: + self._experience = experience + self._github_link = github_link + + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> str: + return self._github_link + + +@dataclass +class Manager: + def __init__(self, experience: float, github_link: str) -> None: + self._experience = experience + self._github_link = github_link + + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> str: + return self._github_link + + +def get_developer_list(developers: List[Developer]) -> List[Dict]: + developers_list = [] + for developer in developers: + developers_list.append({ + 'experience': developer.experience, + 'github_link': developer.github_link + }) + return developers_list + + +def get_manager_list(managers: List[Manager]) -> List[Dict]: + managers_list = [] + for manager in managers: + managers_list.append({ + 'experience': manager.experience, + 'github_link': manager.github_link + }) + return managers_list + + +## create list objects of developers +company_developers = [ + Developer(experience=2.5, github_link='https://github.com/1'), + Developer(experience=1.5, github_link='https://github.com/2') +] +company_developers_list = get_developer_list(developers=company_developers) + +## create list objects of managers +company_managers = [ + Manager(experience=4.5, github_link='https://github.com/3'), + Manager(experience=5.7, github_link='https://github.com/4') +] +company_managers_list = get_manager_list(managers=company_managers) ``` **Good:** -```php -function showList($employees) -{ -    foreach ($employees as $employee) { -        $expectedSalary = $employee->calculateExpectedSalary(); -        $experience = $employee->getExperience(); -        $githubLink = $employee->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} -``` +```python +from typing import List, Dict +from dataclasses import dataclass -**Very good:** -It is better to use a compact version of the code. +@dataclass +class Employee: + def __init__(self, experience: float, github_link: str) -> None: + self._experience = experience + self._github_link = github_link -```php -function showList($employees) -{ -    foreach ($employees as $employee) { - render([ - $employee->calculateExpectedSalary(), - $employee->getExperience(), - $employee->getGithubLink() - ]); - } -} + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> str: + return self._github_link + + +def get_employee_list(employees: List[Employee]) -> List[Dict]: + employees_list = [] + for employee in employees: + employees_list.append({ + 'experience': employee.experience, + 'github_link': employee.github_link + }) + return employees_list + + +## create list objects of developers +company_developers = [ + Employee(experience=2.5, github_link='https://github.com/1'), + Employee(experience=1.5, github_link='https://github.com/2') +] +company_developers_list = get_employee_list(employees=company_developers) + +## create list objects of managers +company_managers = [ + Employee(experience=4.5, github_link='https://github.com/3'), + Employee(experience=5.7, github_link='https://github.com/4') +] +company_managers_list = get_employee_list(employees=company_managers) ``` **[⬆ back to top](#table-of-contents)** + +## **Translations** + +This document is also available in other languages: + +- 🇨🇳 ** + Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/clean-code-python) +- 🇰🇷 ** Korean ** [wooy0ng/clean-code-python](https://github.com/wooy0ng/clean-code-python) +- 🇵🇹 🇧🇷 ** + Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) +- 🇮🇷 ** + Persian:** [SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) + +**[⬆ back to top](#table-of-contents)** diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..cdf71bb --- /dev/null +++ b/conftest.py @@ -0,0 +1,89 @@ +import importlib +import re +import time +import typing +from pathlib import Path + +import pytest +from mypy import api + +code_rxp = re.compile('```python(.*?)```', re.DOTALL | re.MULTILINE) + + +class MyPyValidationError(BaseException): + """A validation error occurred when MyPy attempted to validate the code""" + + +def fake_print(*args, **kwargs): + """Dummy replacement for print() that does nothing""" + pass + + +def pytest_collect_file(parent, path): + """Collect all file suitable for use in tests""" + if path.basename == "README.md": + return ReadmeFile.from_parent(parent, path=Path(path)) + + +class ReadmeFile(pytest.File): + """A Markdown formatted readme file containing code snippets""" + + def collect(self): + """Collect all code snippets""" + raw_text = self.fspath.open().read() + for idx, code in enumerate(code_rxp.findall(raw_text), 1): + yield ReadmeItem.from_parent( + self, name=str(idx), spec=code.strip() + ) + + +def _with_patched_sleep(func, *args, **kwargs): + """Patch the sleep function so that it does nothing""" + _sleep = time.sleep + time.sleep = lambda *args: None + try: + return func(*args, **kwargs) + finally: + time.sleep = _sleep + + +class ReadmeItem(pytest.Item): + """A readme test item that validates a code snippet""" + builtins = ( + ('typing', typing), + ('datetime', importlib.import_module('datetime')), + ('hashlib', importlib.import_module('hashlib')), + ('print', fake_print) + ) + + def __init__(self, name, parent, spec): + super().__init__(name, parent) + self.spec = spec + + def runtest(self): + """Run the test""" + builtins = dict(self.builtins) + byte_code = compile(self.spec, '', 'exec') + _with_patched_sleep(exec, byte_code, builtins) + msg, _, error = api.run(['--no-color-output', '-c', self.spec]) + if error: + # Ignore missing return statements + if "Missing return statement" in msg: + return + # Ignore missing errors related to the injected names + for name in builtins: + if f"Name '{name}' is not defined" in msg: + break + else: + raise MyPyValidationError(msg) + + def repr_failure(self, excinfo, **kwargs): + """ called when self.runtest() raises an exception. """ + return ( + f"Code snippet {self.name} raised an error: {excinfo.value}. " + f"The executed code was: {self.spec}" + ) + + def reportinfo(self): + """Report some basic information on the test outcome""" + return self.fspath, 0, "usecase: {}".format(self.name) diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..4bfa28c --- /dev/null +++ b/requirements.txt @@ -0,0 +1,2 @@ +pytest +mypy