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 339e2e2..cae487b 100644 --- a/README.md +++ b/README.md @@ -1,1519 +1,1611 @@ -# clean-code-php +# 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) + +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 PHP. This is not a style guide. It's a guide to producing -readable, reusable, and refactorable software in PHP. +[*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:** -```php -$ymdstr = $moment->format('y-m-d'); + +```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**: -```php -$currentDate = $moment->format('y-m-d'); + +```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:** -```php -getUserInfo(); -getClientData(); -getCustomerRecord(); +Here we use three different names for the same underlying entity: + +```python +def get_user_info(): pass + + +def get_client_data(): pass + + +def get_customer_record(): pass ``` **Good**: -```php -getUser(); +If the entity is the same, you should be consistent in referring to it in your +functions: + +```python +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 +from typing import Union, Dict + + +class Record: + pass + + +class User: + info: str + + @property + 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:** -```php -// What the heck is 86400 for? -addExpireAt(86400); +```python +import time + +# What is the number 86400 for again? +time.sleep(86400) ``` **Good**: -```php -// Declare them as capitalized `const` globals. -interface DateGlobal { - const SECONDS_IN_A_DAY = 86400; -} -addExpireAt(DateGlobal::SECONDS_IN_A_DAY); +```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)** +**[⬆ back to top](#table-of-contents)** ### Use explanatory variables + **Bad:** -```php -$address = 'One Infinite Loop, Cupertino 95014'; -$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; -preg_match($cityZipCodeRegex, $address, $matches); -saveCityZipCode($matches[1], $matches[2]); +```python +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) +if matches: + print(f"{matches[1]}: {matches[2]}") +``` + +**Not bad**: + +It's better, but we are still heavily dependent on regex. + +```python +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) + +if matches: + city, zip_code = matches.groups() + print(f"{city}: {zip_code}") ``` **Good**: -```php -$address = 'One Infinite Loop, Cupertino 95014'; -$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; -preg_match($cityZipCodeRegex, $address, $matches); -list(, $city, $zipCode) = $matchers; -saveCityZipCode($city, $zipCode); +Decrease dependence on regex by naming subpatterns. + +```python +import re + +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:** -```php -$l = ['Austin', 'New York', 'San Francisco']; - -foreach($i=0; $i 'Honda', - 'carModel' => 'Accord', - 'carColor' => 'Blue', -]; - -function paintCar(&$car) { - $car['carColor'] = 'Red'; -} + +```python +class Car: + car_make: str + car_model: str + car_color: str ``` **Good**: -```php -$car = [ - 'make' => 'Honda', - 'model' => 'Accord', - 'color' => 'Blue', -]; - -function paintCar(&$car) { - $car['color'] = 'Red'; -} + +```python +class Car: + make: str + model: str + color: str ``` + **[⬆ back to top](#table-of-contents)** ### Use default arguments instead of short circuiting or conditionals -**Bad:** -```php -function createMicrobrewery($name = null) { - $breweryName = $name ?: 'Hipster Brew Co.'; - // ... -} +**Tricky** + +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 it clear +that you are expecting a string as the argument. + **Good**: -```php -function createMicrobrewery($breweryName = 'Hipster Brew Co.') { - // ... -} +```python +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** -### 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. +### 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:** -```php -function createMenu($title, $body, $buttonText, $cancellable) { - // ... -} -``` -**Good**: -```php -class menuConfig() { - public $title; - public $body; - public $buttonText; - public $cancellable = false; -} - -$config = new MenuConfig(); -$config->title = 'Foo'; -$config->body = 'Bar'; -$config->buttonText = 'Baz'; -$config->cancellable = true; - -function createMenu(MenuConfig $config) { - // ... -} +```python +from typing import List -``` -**[⬆ back to top](#table-of-contents)** +class Client: + active: bool -### 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:** -```php -function emailClients($clients) { - foreach ($clients as $client) { - $clientRecord = $db->find($client); - if ($clientRecord->isActive()) { - email($client); - } - } -} +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**: -```php -function emailClients($clients) { - $activeClients = activeClients($clients); - array_walk($activeClients, 'email'); -} - -function activeClients($clients) { - return array_filter($clients, 'isClientActive'); -} - -function isClientActive($client) { - $clientRecord = $db->find($client); - return $clientRecord->isActive(); -} -``` -**[⬆ back to top](#table-of-contents)** -### Function names should say what they do +```python +from typing import List -**Bad:** -```php -function addToDate($date, $month) { - // ... -} -$date = new \DateTime(); +class Client: + active: bool -// It's hard to to tell from the function name what is added -addToDate($date, 1); -``` -**Good**: -```php -function addMonthToDate($month, $date) { - // ... -} +def email(client: Client) -> None: + pass -$date = new \DateTime(); -addMonthToDate(1, $date); -``` -**[⬆ back to top](#table-of-contents)** -### 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. +def get_active_clients(clients: List[Client]) -> List[Client]: + """Filter active clients. + """ + return [client for client in clients if client.active] -**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... - } -} -``` -**Good**: -```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 email_clients(clients: List[Client]) -> None: + """Send an email to a given list of clients. + """ + for client in get_active_clients(clients): + email(client) ``` -**[⬆ back to top](#table-of-contents)** -### Remove duplicate code -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 you see an opportunity for using generators now? -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! +**Even better** -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. +```python +from typing import Generator, Iterator -Getting the abstraction right is critical, that's why you should follow the -SOLID principles laid out in the *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. -**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); - } -} -``` +class Client: + active: bool -**Good**: -```php -function showList($employees) { - foreach($employees as $employe) { - $expectedSalary = $employe->calculateExpectedSalary(); - $experience = $employe->getExperience(); - $githubLink = $employe->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} + +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)** -### Set default objects with Object.assign +### Function arguments (2 or fewer ideally) + +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:** -```php -$menuConfig = [ - 'title' => null, - 'body' => 'Bar', - 'buttonText' => null, - 'cancellable' => true, -]; - -function createMenu(&$config) { - $config['title'] = $config['title'] ?: 'Foo'; - $config['body'] = $config['body'] ?: 'Bar'; - $config['buttonText'] = $config['buttonText'] ?: 'Baz'; - $config['cancellable'] = $config['cancellable'] ?: true; -} - -createMenu($menuConfig); -``` -**Good**: -```php -$menuConfig = [ - 'title' => 'Order', - // User did not include 'body' key - 'buttonText' => 'Send', - 'cancellable' => true, -]; - -function createMenu(&$config) { - $config = array_merge([ - 'title' => 'Foo', - 'body' => 'Bar', - 'buttonText' => 'Baz', - 'cancellable' => true, - ], $config); - - // config now equals: {title: "Order", body: "Bar", buttonText: "Send", cancellable: true} - // ... -} - -createMenu($menuConfig); +```python +def create_menu(title, body, button_text, cancellable): + pass ``` -**[⬆ back to top](#table-of-contents)** +**Java-esque**: -### 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. +```python +class Menu: + def __init__(self, config: dict): + self.title = config["title"] + self.body = config["body"] + # ... -**Bad:** -```php -function createFile($name, $temp = false) { - if ($temp) { - touch('./temp/'.$name); - } else { - touch($name); + +menu = Menu( + { + "title": "My Menu", + "body": "Something about my menu", + "button_text": "OK", + "cancellable": False } -} +) ``` -**Good**: -```php -function createFile($name) { - touch($name); -} - -function createTempFile($name) { - touch('./temp/'.$name); -} -``` -**[⬆ back to top](#table-of-contents)** +**Also good** -### 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. +```python +class MenuConfig: + """A configuration for the Menu. -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. + 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? + """ + title: str + body: str + button_text: str + cancellable: bool = False -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. -**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'; +def create_menu(config: MenuConfig) -> None: + title = config.title + body = config.body + # ... -function splitIntoFirstAndLastName() { - $name = preg_split('/ /', $name); -} -splitIntoFirstAndLastName(); +config = MenuConfig() +config.title = "My delicious menu" +config.body = "A description of the various items on the menu" +config.button_text = "Order now!" +# The instance attribute overrides the default class attribute. +config.cancellable = True -var_dump($name); // ['Ryan', 'McDermott']; +create_menu(config) ``` -**Good**: -```php -$name = 'Ryan McDermott'; +**Fancy** -function splitIntoFirstAndLastName($name) { - return preg_split('/ /', $name); -} +```python +from typing import NamedTuple -$name = 'Ryan McDermott'; -$newName = splitIntoFirstAndLastName($name); -var_dump($name); // 'Ryan McDermott'; -var_dump($newName); // ['Ryan', 'McDermott']; -``` -**[⬆ back to top](#table-of-contents)** +class MenuConfig(NamedTuple): + """A configuration for the Menu. -### Don't write to global functions -Polluting globals is a bad practice in very 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. This is why it -would be much better to use singleton design pattern and simple set configuration. + 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? + """ + title: str + body: str + button_text: str + cancellable: bool = False -**Bad:** -```php -function config() { - return [ - 'foo': 'bar', - ] -} -``` -**Good:** -```php -class Configuration { - private static $instance; - private function __construct($configuration) {/* */} - public static function getInstance() { - if (self::$instance === null) { - self::$instance = new Configuration(); - } - return self::$instance; - } - public function get($key) {/* */} - public function getAll() {/* */} -} +def create_menu(config: MenuConfig): + title, body, button_text, cancellable = config + # ... -$singleton = Configuration::getInstance(); + +create_menu( + MenuConfig( + title="My delicious menu", + body="A description of the various items on the menu", + button_text="Order now!" + ) +) ``` -**[⬆ back to top](#table-of-contents)** -### Encapsulate conditionals +**Even fancier** -**Bad:** -```php -if ($fsm->state === 'fetching' && is_empty($listNode)) { - // ... -} -``` +```python +from dataclasses import astuple, dataclass -**Good**: -```php -function shouldShowSpinner($fsm, $listNode) { - return $fsm->state === 'fetching' && is_empty($listNode); -} - -if (shouldShowSpinner($fsmInstance, $listNodeInstance)) { - // ... -} -``` -**[⬆ back to top](#table-of-contents)** -### Avoid negative conditionals +@dataclass +class MenuConfig: + """A configuration for the Menu. -**Bad:** -```php -function isDOMNodeNotPresent($node) { - // ... -} - -if (!isDOMNodeNotPresent($node)) { - // ... -} + 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? + """ + title: str + body: str + button_text: str + cancellable: bool = False + + +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!" + ) +) ``` -**Good**: -```php -function isDOMNodePresent($node) { - // ... -} - -if (isDOMNodePresent($node)) { - // ... -} +**Even fancier, Python3.8+ only** + +```python +from typing import TypedDict + + +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? + """ + 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)** -### 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. +### Function names should say what they do **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(); - } - } -} + +```python +class Email: + def handle(self) -> None: + pass + + +message = Email() +# What is this supposed to do again? +message.handle() ``` -**Good**: -```php -class Airplane { - // ... -} - -class Boeing777 extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude() - $this->getPassengerCount(); - } -} +**Good:** -class AirForceOne extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude(); - } -} +```python +class Email: + def send(self) -> None: + """Send this message""" -class Cessna extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude() - $this->getFuelExpenditure(); - } -} + +message = Email() +message.send() ``` + **[⬆ back to top](#table-of-contents)** -### Avoid type-checking (part 1) -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. +### 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. **Bad:** -```php -function travelToTexas($vehicle) { - if ($vehicle instanceof Bicycle) { - $vehicle->peddle($this->currentLocation, new Location('texas')); - } else if ($vehicle instanceof Car) { - $vehicle->drive($this->currentLocation, new Location('texas')); - } -} + +```python +# type: ignore + +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**: -```php -function travelToTexas($vehicle) { - $vehicle->move($this->currentLocation, new Location('texas')); -} +**Good:** + +```python +from typing import Tuple, List, Dict + +REGEXES: Tuple = ( + # ... +) + + +def parse_better_js_alternative(code: str) -> None: + tokens: List = tokenize(code) + syntax_tree: List = parse(tokens) + + for node in syntax_tree: + pass + + +def tokenize(code: str) -> List: + statements = code.split() + tokens: List[Dict] = [] + for regex in REGEXES: + for statement in statements: + pass + + return tokens + + +def parse(tokens: List) -> List: + syntax_tree: List[Dict] = [] + for token in tokens: + pass + + return syntax_tree ``` + **[⬆ back to top](#table-of-contents)** -### Avoid type-checking (part 2) -If you are working with basic primitive values like strings, integers, and arrays, -and you can't use polymorphism but you still feel the need to type-check, -you should consider 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. +### 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 your functions if they are following different code +paths based on a boolean. **Bad:** -```php -function combine($val1, $val2) { - if (is_numeric($val1) && is_numeric($val2)) { - return $val1 + $val2; - } - - throw new \Exception('Must be of type Number'); -} + +```python +from tempfile import gettempdir +from pathlib import Path + + +def create_file(name: str, temp: bool) -> None: + if temp: + (Path(gettempdir()) / name).touch() + else: + Path(name).touch() ``` -**Good**: -```php -function combine(int $val1, int $val2) { - return $val1 + $val2; -} +**Good:** + +```python +from tempfile import gettempdir +from pathlib import Path + + +def create_file(name: str) -> None: + Path(name).touch() + + +def create_temp_file(name: str) -> None: + (Path(gettempdir()) / name).touch() ``` + **[⬆ back to top](#table-of-contents)** -### 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. +### 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. For example, 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 - 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. + +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. **Bad:** -```php -function oldRequestModule($url) { - // ... -} -function newRequestModule($url) { - // ... -} +```python +# type: ignore -$req = new newRequestModule(); -inventoryTracker('apples', $req, 'www.inventory-awesome.io'); +# This is a module-level name. +# It's good practice to define these as immutable values, such as a string. +# However... +fullname = "Ryan McDermott" -``` -**Good**: -```php -function newRequestModule($url) { - // ... -} +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() + -$req = new newRequestModule(); -inventoryTracker('apples', $req, 'www.inventory-awesome.io'); +split_into_first_and_last_name() + +# MyPy will spot the problem, complaining about 'Incompatible types in +# assignment: (expression has type "List[str]", variable has type "str")' +print(fullname) # ["Ryan", "McDermott"] + +# OK. It worked the first time, but what will happen if we call the +# function again? ``` -**[⬆ back to top](#table-of-contents)** +**Good:** -## **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. +```python +from typing import List, AnyStr -* 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. -Additionally, this is part of Open/Closed principle, from object-oriented -design principles. +def split_into_first_and_last_name(name: AnyStr) -> List[AnyStr]: + return name.split() -**Bad:** -```php -class BankAccount { - public $balance = 1000; -} -$bankAccount = new BankAccount(); +fullname = "Ryan McDermott" +name, surname = split_into_first_and_last_name(fullname) -// Buy shoes... -$bankAccount->balance -= 100; +print(name, surname) # => Ryan McDermott ``` -**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; - } -} +**Also good** + +```python +from dataclasses import dataclass -$bankAccount = new BankAccount(); -// Buy shoes... -$bankAccount->withdrawBalance(-$shoesPrice); +@dataclass +class Person: + name: str -// Get balance -$balance = $bankAccount->getBalance(); + @property + def name_as_first_and_last(self) -> list: + return self.name.split() + +# 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"] ``` + **[⬆ back to top](#table-of-contents)** +## **Classes** -### Make objects have private/protected members +### **Single Responsibility Principle (SRP)** -**Bad:** -```php -class Employee { - public $name; - - public function __construct($name) { - $this->name = $name; - } -} +Robert C. Martin writes: -$employee = new Employee('John Doe'); -echo 'Employee name: '.$employee->name; // Employee name: John Doe -``` +> A class should have only one reason to change. -**Good**: -```php -class Employee { - protected $name; - - public function __construct($name) { - $this->name = $name; - } - - public function getName() { - return $this->name; - } -} +"Reasons to change" are, in essence, the responsibilities managed by a class or +function. + +In the following example, we create an HTML element that represents a comment +with the version of the document: + +**Bad** + +```python +from importlib import metadata + + +class VersionCommentElement: + """An element that renders an HTML comment with the program's version number + """ -$employee = new Employee('John Doe'); -echo 'Employee name: '.$employee->getName(); // Employee name: John Doe + def get_version(self) -> str: + """Get the package version""" + return metadata.version("pip") + + def render(self) -> None: + print(f'') + + +VersionCommentElement().render() ``` -**[⬆ back to top](#table-of-contents)** +This class has two responsibilities: -## **Classes** +- Retrieve the version number of the Python package +- Render itself as an HTML element -### 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. +Any change to one or the other carries the risk of impacting the other. -**Bad:** -```php -class UserSettings { - private $user; - public function __construct($user) { - $this->user = user; - } - - public function changeSettings($settings) { - if ($this->verifyCredentials()) { - // ... - } - } - - private function verifyCredentials() { - // ... - } -} +We can rewrite the class and decouple these responsibilities: + +**Good** + +```python +from importlib import metadata + + +def get_version(pkg_name: str) -> str: + """Retrieve the version of a given package""" + return metadata.version(pkg_name) + + +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() ``` -**Good:** -```php -class UserAuth { - private $user; - public function __construct($user) { - $this->user = user; - } - - protected function verifyCredentials() { - // ... - } -} +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. +As an added bonus, the `get_version()` is now reusable elsewhere. + +### **Open/Closed Principle (OCP)** + +> “Incorporate new features by extending the system, not by making +> modifications (to it)”, +> Uncle Bob. + +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. + +`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. + +**Bad** + +```python +from dataclasses import dataclass + + +@dataclass +class Response: + """An HTTP response""" + + 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" + ) + + +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() + ) -class UserSettings { - private $user; - 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)** -### Open/Closed Principle (OCP) -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. +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. -**Bad:** -```php -abstract class Adapter { - protected $name; - public function getName() { - return $this->name; - } -} +Let's redesign our classes to fix this problem and let the `View` class be +extended (not modified) cleanly: -class AjaxAdapter extends Adapter { - public function __construct() { - parent::__construct(); - $this->name = 'ajaxAdapter'; - } -} +**Good** + +```python +from dataclasses import dataclass + + +@dataclass +class Response: + """An HTTP response""" + + status: int + content_type: str + body: str + + +class View: + """A simple view that returns plain text responses""" + + content_type = "text/plain" + + 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() + ) + + +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() -class NodeAdapter extends Adapter { - public function __construct() { - parent::__construct(); - $this->name = 'nodeAdapter'; - } -} -class HttpRequester { - private $adapter; - public function __construct($adapter) { - $this->adapter = $adapter; - } - - public function fetch($url) { - $adapterName = $this->adapter->getName(); - if ($adapterName === 'ajaxAdapter') { - return $this->makeAjaxCall($url); - } else if ($adapterName === 'httpNodeAdapter') { - return $this->makeHttpCall($url); - } - } - - protected function makeAjaxCall($url) { - // request and return promise - } - - protected function makeHttpCall($url) { - // request and return promise - } -} ``` -**Good:** -```php -abstract class Adapter { - abstract protected function getName(); - abstract public function request($url); -} - -class AjaxAdapter extends Adapter { - protected function getName() { - return 'ajaxAdapter'; - } - - public function request($url) { - // request and return promise - } -} +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. -class NodeAdapter extends Adapter { - protected function getName() { - return 'nodeAdapter'; - } - - public function request($url) { - // request and return promise - } -} +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. + +A few rules: + +- Mixins should always inherit from `object` +- Mixins always come before the target class, + e.g. `class Foo(MixinA, MixinB, TargetClass): ...` + +**Also good** + +```python +from dataclasses import dataclass, field +from typing import Protocol + + +@dataclass +class Response: + """An HTTP response""" + + status: int + content_type: str + body: str + headers: dict = field(default_factory=dict) + + +class View: + """A simple view that returns plain text responses""" + + content_type = "text/plain" + + 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() + ) -class HttpRequester { - private $adapter; - public function __construct(Adapter $adapter) { - $this->adapter = $adapter; - } - - public function fetch($url) { - return $this->adapter->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 = "" + + 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.") + + with open(self.template_file) as fd: + return fd.read() + + +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) -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. +The popular Django project makes heavy use of Mixins to compose its class-based +views. -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. +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. -**Bad:** -```php -class Rectangle { - private $width, $height; - - public function __construct() { - $this->width = 0; - $this->height = 0; - } - - public function setColor($color) { - // ... - } - - public function render($area) { - // ... - } - - public function setWidth($width) { - $this->width = $width; - } - - public function setHeight($height) { - $this->height = $height; - } - - public function getArea() { - return $this->width * $this->height; - } -} +### **Liskov Substitution Principle (LSP)** + +> “Functions that use pointers or references to base classes +> must be able to use objects of derived classes without knowing it”, +> Uncle Bob. + +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". + +In essence, a function accepting a supertype should also accept all its +subtypes with no modification. + +Can you spot the problem with the following code? + +**Bad** + +```python +from dataclasses import dataclass + + +@dataclass +class Response: + """An HTTP response""" + + status: int + content_type: str + body: str + + +class View: + """A simple view that returns plain text responses""" + + content_type = "text/plain" + + 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() + ) + + +class TemplateView(View): + """A view that returns HTML responses based on a template file.""" + + content_type = "text/html" + + 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() + ) + + +def render(view: View, request) -> Response: + """Render a View""" + return view.get(request) -class Square extends Rectangle { - public function setWidth($width) { - $this->width = $this->height = $width; - } - - public function setHeight(height) { - $this->width = $this->height = $height; - } -} - -function renderLargeRectangles($rectangles) { - foreach($rectangle in $rectangles) { - $rectangle->setWidth(4); - $rectangle->setHeight(5); - $area = $rectangle->getArea(); // BAD: Will return 25 for Square. Should be 20. - $rectangle->render($area); - }); -} - -$rectangles = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles($rectangles); ``` -**Good:** -```php -abstract class Shape { - private $width, $height; - - abstract public function getArea(); - - public function setColor($color) { - // ... - } - - public function 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`. -class Rectangle extends Shape { - public function __construct { - parent::__construct(); - $this->width = 0; - $this->height = 0; - } - - public function setWidth($width) { - $this->width = $width; - } - - public function setHeight($height) { - $this->height = $height; - } - - public function getArea() { - return $this->width * $this->height; - } -} +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: -class Square extends Shape { - public function __construct { - parent::__construct(); - $this->length = 0; - } - - public function setLength($length) { - $this->length = $length; - } - - public function getArea() { - return $this->length * $this->length; - } -} - -function renderLargeRectangles($rectangles) { - foreach($rectangle in $rectangles) { - if ($rectangle instanceof Square) { - $rectangle->setLength(5); - } else if ($rectangle instanceof Rectangle) { - $rectangle->setWidth(4); - $rectangle->setHeight(5); - } - - $area = $rectangle->getArea(); - $rectangle->render($area); - }); -} - -$shapes = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles($shapes); ``` -**[⬆ back to top](#table-of-contents)** +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 +``` -### Interface Segregation Principle (ISP) -ISP states that "Clients should not be forced to depend upon interfaces that -they do not use." +### **Interface Segregation Principle (ISP)** -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". +> “Keep interfaces small +> so that users don’t end up depending on things they don’t need.”, +> Uncle Bob. -**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 - } -} +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". -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } +Python does not have interfaces. We have Abstract Base Classes instead, which +are a little different, but can serve the same purpose. - public function eat() { - //.... eating in launch break - } -} - -class Manager { - /** @var WorkerInterface $worker **/ - private $worker; - - public void setWorker(WorkerInterface $worker) { - $this->worker = $worker; - } +**Good** - public function manage() { - $this->worker->work(); - } -} -``` +```python -**Good:** -```php -interface WorkerInterface extends FeedableInterface, WorkableInterface { -} +from abc import ABCMeta, abstractmethod -interface WorkableInterface { - public function work(); -} -interface FeedableInterface { - public function eat(); -} +# Define the Abstract Class for a generic Greeter object +class Greeter(metaclass=ABCMeta): + """An object that can perform a greeting action.""" -class Worker implements WorkableInterface, FeedableInterface { - public function work() { - // ....working - } + @staticmethod + @abstractmethod + def greet(name: str) -> None: + """Display a greeting for the user with the given name""" - public function eat() { - //.... eating in launch break - } -} -class Robot implements WorkableInterface { - public void work() { - // ....working - } -} +class FriendlyActor(Greeter): + """An actor that greets the user with a friendly salutation""" -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } + @staticmethod + def greet(name: str) -> None: + """Greet a person by name""" + print(f"Hello {name}!") - public function eat() { - //.... eating in launch break - } -} -class Manager { - /** @var $worker WorkableInterface **/ - private $worker; +def welcome_user(user_name: str, actor: Greeter): + """Welcome a user with a given name using the provided actor""" + actor.greet(user_name) - public function setWorker(WorkableInterface $w) { - $this->worker = $w; - } - public function manage() { - $this->worker->work(); - } -} +welcome_user("Barbara", FriendlyActor()) ``` -**[⬆ back to top](#table-of-contents)** -### 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. +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. -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. +**Error** -**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(); - } -} +```python +import abc + + +class Persistable(metaclass=abc.ABCMeta): + """Serialize a file to data and back""" + + @property + @abc.abstractmethod + def data(self) -> bytes: + """The raw data of the file""" + + @classmethod + @abc.abstractmethod + def load(cls, name: str): + """Load the file from disk""" + + @abc.abstractmethod + def save(self) -> None: + """Save the file to disk""" + + +# 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`. + +class PDFDocument(Persistable): + """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): + """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 -class SuperWorker extends Worker { - public function work() { - //.... working much more - } -} ``` -**Good:** -```php -interface WorkerInterface { - public function work(); -} - -class Worker implements WorkerInterface { - public function work() { - // ....working - } -} +But we can't! If we don't implement the `.save()` method, an exception will be +raised: -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } -} - -class Manager { - /** @var Worker $worker **/ - private $worker; - - public void __construct(WorkerInterface $worker) { - $this->worker = $worker; - } - - public void manage() { - $this->worker->work(); - } -} +``` +Can't instantiate abstract class PDFDocument with abstract method save. +``` + +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. + +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. + +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): + ... + + +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)** -### 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. +### **Dependency Inversion Principle (DIP)** -**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); - } -} +> “Depend upon abstractions, not concrete details”, +> Uncle Bob. + +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 -$car = new Car(); -$car->setColor('pink'); -$car->setMake('Ford'); -$car->setModel('F-150'); -$car->dump(); ``` -**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); - } -} +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"'}, + ) -$car = (new Car()) - ->setColor('pink') - ->setMake('Ford') - ->setModel('F-150') - ->dump(); ``` + +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)** -### 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. +## **Don't repeat yourself (DRY)** + +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. -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: +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! -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). +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. 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 -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($ssn, $salary) { - parent::__construct(); - $this->ssn = $ssn; - $this->salary = $salary; - } - - // ... -} + +```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 -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); - } - // ... -} + +```python +from typing import List, Dict +from dataclasses import dataclass + + +@dataclass +class Employee: + 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_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