From 53e07b44f0a81c7298e0f55fc28c22e0f1f722e1 Mon Sep 17 00:00:00 2001 From: Cassio Santos Date: Tue, 30 May 2017 17:08:55 -0300 Subject: [PATCH 001/128] Remove Object.assign section --- README.md | 47 ----------------------------------------------- 1 file changed, 47 deletions(-) diff --git a/README.md b/README.md index ec2affc..1a9691d 100644 --- a/README.md +++ b/README.md @@ -433,53 +433,6 @@ function showList($employees) { ``` **[⬆ back to top](#table-of-contents)** -### Set default objects with Object.assign - -**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); -``` -**[⬆ 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 From f7a26211c12b430b8f75579df69d28dad1465fa9 Mon Sep 17 00:00:00 2001 From: Piotr Plenik Date: Wed, 31 May 2017 10:21:48 +0200 Subject: [PATCH 002/128] add missing variable --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 30a19a7..d4b4e56 100644 --- a/README.md +++ b/README.md @@ -714,7 +714,7 @@ function newRequestModule($url) { // ... } -$req = new newRequestModule(); +$req = new newRequestModule($requestUrl); inventoryTracker('apples', $req, 'www.inventory-awesome.io'); ``` @@ -725,7 +725,7 @@ function newRequestModule($url) { // ... } -$req = new newRequestModule(); +$req = new newRequestModule($requestUrl); inventoryTracker('apples', $req, 'www.inventory-awesome.io'); ``` **[⬆ back to top](#table-of-contents)** From 58ba5d1b00a252329a319d118f043e2c81c43d83 Mon Sep 17 00:00:00 2001 From: Piotr Plenik Date: Wed, 31 May 2017 10:25:01 +0200 Subject: [PATCH 003/128] remove new in method name --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d4b4e56..eb072e6 100644 --- a/README.md +++ b/README.md @@ -721,11 +721,11 @@ inventoryTracker('apples', $req, 'www.inventory-awesome.io'); **Good**: ```php -function newRequestModule($url) { +function requestModule($url) { // ... } -$req = new newRequestModule($requestUrl); +$req = new requestModule($requestUrl); inventoryTracker('apples', $req, 'www.inventory-awesome.io'); ``` **[⬆ back to top](#table-of-contents)** From d89d27ad8caf905651392df48db97644a3bc2fcc Mon Sep 17 00:00:00 2001 From: vlassiuk Date: Mon, 28 Aug 2017 16:11:02 +0300 Subject: [PATCH 004/128] no need for minus before variable --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index eb072e6..985d5ca 100644 --- a/README.md +++ b/README.md @@ -788,7 +788,7 @@ class BankAccount { $bankAccount = new BankAccount(); // Buy shoes... -$bankAccount->withdrawBalance(-$shoesPrice); +$bankAccount->withdrawBalance($shoesPrice); // Get balance $balance = $bankAccount->getBalance(); From 6987a2eb7358f59bbea047c321e75ce38cf598cd Mon Sep 17 00:00:00 2001 From: Galymzhan Abdugalimov Date: Mon, 28 Aug 2017 22:16:17 +0600 Subject: [PATCH 005/128] Fixed array key=>value assign --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index eb072e6..1493cfb 100644 --- a/README.md +++ b/README.md @@ -519,7 +519,7 @@ would be much better to use singleton design pattern and simple set configuratio ```php function config() { return [ - 'foo': 'bar', + 'foo' => 'bar', ] } ``` From bfe98f0d2672de96001c6087f570163a45ac7ecd Mon Sep 17 00:00:00 2001 From: Valerii Kuznetsov Date: Tue, 29 Aug 2017 08:10:09 +1200 Subject: [PATCH 006/128] Update README.md Looks like a bug in good practice. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index eb072e6..985d5ca 100644 --- a/README.md +++ b/README.md @@ -788,7 +788,7 @@ class BankAccount { $bankAccount = new BankAccount(); // Buy shoes... -$bankAccount->withdrawBalance(-$shoesPrice); +$bankAccount->withdrawBalance($shoesPrice); // Get balance $balance = $bankAccount->getBalance(); From 8739c9f0a8d41a9d6ad87761523136aba6d01ddd Mon Sep 17 00:00:00 2001 From: vlassiuk Date: Tue, 29 Aug 2017 11:21:12 +0300 Subject: [PATCH 007/128] function, of course --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 985d5ca..ae5e428 100644 --- a/README.md +++ b/README.md @@ -1168,7 +1168,7 @@ class Manager { /** @var WorkerInterface $worker **/ private $worker; - public void setWorker(WorkerInterface $worker) { + public function setWorker(WorkerInterface $worker) { $this->worker = $worker; } From b4c9b75b21adade1fe31b0e4e061b8dab738b3e9 Mon Sep 17 00:00:00 2001 From: vlassiuk Date: Tue, 29 Aug 2017 11:22:13 +0300 Subject: [PATCH 008/128] function, of course --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index ae5e428..8af5920 100644 --- a/README.md +++ b/README.md @@ -1202,7 +1202,7 @@ class Worker implements WorkableInterface, FeedableInterface { } class Robot implements WorkableInterface { - public void work() { + public function work() { // ....working } } @@ -1296,11 +1296,11 @@ class Manager { /** @var Worker $worker **/ private $worker; - public void __construct(WorkerInterface $worker) { + public function __construct(WorkerInterface $worker) { $this->worker = $worker; } - public void manage() { + public function manage() { $this->worker->work(); } } From 78803ded9bce2f1d25f0b6ba213ee7fd2a4f48a3 Mon Sep 17 00:00:00 2001 From: vlassiuk Date: Tue, 29 Aug 2017 11:26:26 +0300 Subject: [PATCH 009/128] arrays syntax --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8af5920..2693a73 100644 --- a/README.md +++ b/README.md @@ -519,7 +519,7 @@ would be much better to use singleton design pattern and simple set configuratio ```php function config() { return [ - 'foo': 'bar', + 'foo' => 'bar', ] } ``` From c806ccd3736e5088f6900aa837d7bff9103b0a32 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Tue, 29 Aug 2017 18:04:53 +0300 Subject: [PATCH 010/128] decrease dependence on regex --- README.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index eb072e6..f51262e 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,10 @@ preg_match($cityZipCodeRegex, $address, $matches); saveCityZipCode($matches[1], $matches[2]); ``` -**Good**: +**Not bad**: + +It's better, but we are still heavily dependent on regex. + ```php $address = 'One Infinite Loop, Cupertino 95014'; $cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; @@ -98,6 +101,17 @@ preg_match($cityZipCodeRegex, $address, $matches); list(, $city, $zipCode) = $matchers; saveCityZipCode($city, $zipCode); ``` + +**Good**: + +Decrease dependence on regex by naming subpatterns. +```php +$address = 'One Infinite Loop, Cupertino 95014'; +$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(?.+?)\s*(?\d{5})?$/'; +preg_match($cityZipCodeRegex, $address, $matches); + +saveCityZipCode($matchers['city'], $matchers['zipCode']); +``` **[⬆ back to top](#table-of-contents)** ### Avoid Mental Mapping From 08d73d655efcbab0ae789326fc016ffacc2fdaa2 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Tue, 29 Aug 2017 18:18:18 +0300 Subject: [PATCH 011/128] correct use foreach --- README.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index eb072e6..a852edf 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,7 @@ Explicit is better than implicit. ```php $l = ['Austin', 'New York', 'San Francisco']; -foreach($i=0; $i Date: Tue, 29 Aug 2017 18:25:37 +0300 Subject: [PATCH 012/128] Correct declaration MenuConfig class --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index eb072e6..92b02ee 100644 --- a/README.md +++ b/README.md @@ -208,7 +208,8 @@ function createMenu($title, $body, $buttonText, $cancellable) { **Good**: ```php -class menuConfig() { +class MenuConfig +{ public $title; public $body; public $buttonText; From ee26d57559b30ba8136fa99303607bf1514d1d48 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Tue, 29 Aug 2017 19:23:58 +0300 Subject: [PATCH 013/128] Optimize showList function --- README.md | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index eb072e6..99c2010 100644 --- a/README.md +++ b/README.md @@ -418,16 +418,11 @@ function showManagerList($managers) { ```php function showList($employees) { foreach($employees as $employe) { - $expectedSalary = $employe->calculateExpectedSalary(); - $experience = $employe->getExperience(); - $githubLink = $employe->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); + render([ + $employe->calculateExpectedSalary(), + $employe->getExperience(), + $employe->getGithubLink() + ]); } } ``` From f6e0fe38bcb61bfd3ff5d84133969479df89c79d Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Tue, 29 Aug 2017 19:41:38 +0300 Subject: [PATCH 014/128] Singleton is a anti-pattern --- README.md | 59 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index eb072e6..24da54d 100644 --- a/README.md +++ b/README.md @@ -512,35 +512,64 @@ Polluting globals is a bad practice in very languages because you could clash wi 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. +that tried to do the same thing. **Bad:** ```php -function config() { +function config() +{ return [ - 'foo': 'bar', + 'foo' => 'bar', ] } ``` **Good:** + +Create configuration file + ```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; +// config.php +return [ + 'foo' => 'bar', +]; +``` + +Or a [YAML](https://en.wikipedia.org/wiki/YAML) configuration file + +```php +// config.yml +configuration: + foo: 'bar' +``` + +Or something else + +```php +class Configuration +{ + private $configuration = []; + + public function __construct(array $configuration) + { + $this->configuration = $configuration; + } + + public function get($key) + { + return isset($this->configuration[$key]) ? $this->configuration[$key] : null; } - public function get($key) {/* */} - public function getAll() {/* */} } +``` + +Load configuration from file and create instance of `Configuration` class -$singleton = Configuration::getInstance(); +```php +$configuration = new Configuration($configuration); ``` + +And now you must use instance of `Configuration` in your application. + **[⬆ back to top](#table-of-contents)** ### Encapsulate conditionals From 0fab26d206e44c1f446638608a815e8df73d7c13 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 00:21:31 +0300 Subject: [PATCH 015/128] use default arguments in PHP 7+ --- README.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index eb072e6..6307b22 100644 --- a/README.md +++ b/README.md @@ -171,19 +171,23 @@ function paintCar(&$car) { ### Use default arguments instead of short circuiting or conditionals -**Bad:** +**Not bad:** + ```php -function createMicrobrewery($name = null) { - $breweryName = $name ?: 'Hipster Brew Co.'; +function createMicrobrewery($name = null) +{ +    $breweryName = $name ?: 'Hipster Brew Co.'; // ... } ``` -**Good**: +**Good for PHP 7+**: + ```php -function createMicrobrewery($breweryName = 'Hipster Brew Co.') { - // ... +function createMicrobrewery(string $breweryName = 'Hipster Brew Co.') +{ +    // ... } ``` From 47d6f95f43fc0150b192a6179baf1858a953e6b0 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 08:41:34 +0300 Subject: [PATCH 016/128] Add origin bad example --- README.md | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 6307b22..7958a1b 100644 --- a/README.md +++ b/README.md @@ -171,26 +171,40 @@ function paintCar(&$car) { ### Use default arguments instead of short circuiting or conditionals +**Not good:** + +This is not good because `$breweryName` can be `NULL`. + +```php +function createMicrobrewery($breweryName = 'Hipster Brew Co.') +{ +    // ... +} +``` + **Not bad:** +This opinion is understandable than the previous version, but it better controls the value of the variable. + ```php function createMicrobrewery($name = null) {    $breweryName = $name ?: 'Hipster Brew Co.'; // ... } - ``` -**Good for PHP 7+**: +**Good**: + +If you support only PHP 7+, then you can use [type hinting](http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration) and be sure that the `$breweryName` will not be `NULL`. ```php function createMicrobrewery(string $breweryName = 'Hipster Brew Co.') {    // ... } - ``` + **[⬆ back to top](#table-of-contents)** ## **Functions** ### Function arguments (2 or fewer ideally) From 3c0c05e032c3194ec87a5dce46e637d078a52d54 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 10:45:54 +0300 Subject: [PATCH 017/128] add bad singleton example and remove YAML config --- README.md | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 24da54d..e029953 100644 --- a/README.md +++ b/README.md @@ -515,6 +515,7 @@ You could write global function like `config()`, but it could clash with another that tried to do the same thing. **Bad:** + ```php function config() { @@ -524,9 +525,41 @@ function config() } ``` +**Bad too:** + +Singleton is a [anti-pattern](https://en.wikipedia.org/wiki/Singleton_pattern). + +```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) + { + // ... + } +} + +$singleton = Configuration::getInstance(); +``` + **Good:** -Create configuration file +Create PHP configuration file or something else ```php // config.php @@ -535,16 +568,6 @@ return [ ]; ``` -Or a [YAML](https://en.wikipedia.org/wiki/YAML) configuration file - -```php -// config.yml -configuration: - foo: 'bar' -``` - -Or something else - ```php class Configuration { From 413895b25cd8e0d3caf91591c9c857ec0e64d548 Mon Sep 17 00:00:00 2001 From: Sergiy Litvinchuk Date: Wed, 30 Aug 2017 11:08:51 +0300 Subject: [PATCH 018/128] make verifyCredentials public after SRP splitting --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5dc5faf..d1c30f8 100644 --- a/README.md +++ b/README.md @@ -886,7 +886,7 @@ class UserAuth { $this->user = user; } - protected function verifyCredentials() { + public function verifyCredentials() { // ... } } From 1ddff50a9b3bbf01ae02658499d9c2615265c22d Mon Sep 17 00:00:00 2001 From: Pavel Date: Wed, 30 Aug 2017 14:44:26 +0500 Subject: [PATCH 019/128] Correction of typos --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5dc5faf..e2f0e9e 100644 --- a/README.md +++ b/README.md @@ -110,7 +110,7 @@ $address = 'One Infinite Loop, Cupertino 95014'; $cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(?.+?)\s*(?\d{5})?$/'; preg_match($cityZipCodeRegex, $address, $matches); -saveCityZipCode($matchers['city'], $matchers['zipCode']); +saveCityZipCode($matches['city'], $matches['zipCode']); ``` **[⬆ back to top](#table-of-contents)** @@ -124,7 +124,7 @@ $l = ['Austin', 'New York', 'San Francisco']; for ($i = 0; $i < count($l); $i++) { $li = $l[$i]; - oStuff(); + doStuff(); doSomeOtherStuff(); // ... // ... @@ -925,7 +925,7 @@ abstract class Adapter { class AjaxAdapter extends Adapter { public function __construct() { - parent::__construct(); + parent::__construct(); $this->name = 'ajaxAdapter'; } } @@ -1119,7 +1119,7 @@ class Square extends Shape { } public function getArea() { - return $this->length * $this->length; + return pow($this->length, 2); } } From 19b95ac77d9a726b52fc04d4e48671d3f7195d62 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 18:47:04 +0300 Subject: [PATCH 020/128] add section: Don't use a Singleton pattern --- README.md | 82 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index e029953..95e2244 100644 --- a/README.md +++ b/README.md @@ -525,16 +525,55 @@ function config() } ``` -**Bad too:** +**Good:** -Singleton is a [anti-pattern](https://en.wikipedia.org/wiki/Singleton_pattern). +Create PHP configuration file or something else + +```php +// config.php +return [ + 'foo' => 'bar', +]; +``` ```php class Configuration +{ + private $configuration = []; + + public function __construct(array $configuration) + { + $this->configuration = $configuration; + } + + public function get($key) + { + return isset($this->configuration[$key]) ? $this->configuration[$key] : null; + } +} +``` + +Load configuration from file and create instance of `Configuration` class + +```php +$configuration = new Configuration($configuration); +``` + +And now you must use instance of `Configuration` in your application. + +**[⬆ back to top](#table-of-contents)** + +### Don't use a Singleton pattern +Singleton is a [anti-pattern](https://en.wikipedia.org/wiki/Singleton_pattern). + +**Bad:** + +```php +class DBConnection { private static $instance; - private function __construct($configuration) + private function __construct($dsn) { // ... } @@ -542,56 +581,39 @@ class Configuration public static function getInstance() { if (self::$instance === null) { - self::$instance = new Configuration(); + self::$instance = new self(); } return self::$instance; } - public function get($key) - { - // ... - } + // ... } -$singleton = Configuration::getInstance(); +$singleton = DBConnection::getInstance(); ``` **Good:** -Create PHP configuration file or something else - ```php -// config.php -return [ - 'foo' => 'bar', -]; -``` - -```php -class Configuration +class DBConnection { - private $configuration = []; - - public function __construct(array $configuration) + public function __construct(array $dsn) { - $this->configuration = $configuration; + // ... } - public function get($key) - { - return isset($this->configuration[$key]) ? $this->configuration[$key] : null; - } + // ... } ``` -Load configuration from file and create instance of `Configuration` class +Create instance of `DBConnection` class and configure it with [DSN](http://php.net/manual/en/pdo.construct.php#refsect1-pdo.construct-parameters). ```php -$configuration = new Configuration($configuration); +$connection = new DBConnection($dsn); ``` -And now you must use instance of `Configuration` in your application. +And now you must use instance of `DBConnection` in your application. **[⬆ back to top](#table-of-contents)** From dd259f3135dae77fde97982e6ff11f8a5995befa Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 19:14:00 +0300 Subject: [PATCH 021/128] correct example of encapsulate conditionals --- README.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 4455b29..6b34815 100644 --- a/README.md +++ b/README.md @@ -559,22 +559,21 @@ $singleton = Configuration::getInstance(); ### Encapsulate conditionals **Bad:** + ```php -if ($fsm->state === 'fetching' && is_empty($listNode)) { +if ($article->state === 'published') { // ... } ``` **Good**: -```php -function shouldShowSpinner($fsm, $listNode) { - return $fsm->state === 'fetching' && is_empty($listNode); -} -if (shouldShowSpinner($fsmInstance, $listNodeInstance)) { - // ... +```php +if ($article->isPublished()) { + // ... } ``` + **[⬆ back to top](#table-of-contents)** ### Avoid negative conditionals From 03a82e35c2a9bb0fb3389b82f22c5689517ef5a7 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 19:18:03 +0300 Subject: [PATCH 022/128] correct examples of don't add unneeded context --- README.md | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 4455b29..c0525be 100644 --- a/README.md +++ b/README.md @@ -151,34 +151,36 @@ foreach ($locations as $location) { ### Don't add unneeded context + If your class/object name tells you something, don't repeat that in your variable name. **Bad:** + ```php -$car = [ - 'carMake' => 'Honda', - 'carModel' => 'Accord', - 'carColor' => 'Blue', -]; +class Car +{ + public $carMake; + public $carModel; + public $carColor; -function paintCar(&$car) { - $car['carColor'] = 'Red'; + //... } ``` **Good**: + ```php -$car = [ - 'make' => 'Honda', - 'model' => 'Accord', - 'color' => 'Blue', -]; +class Car +{ + public $make; + public $model; + public $color; -function paintCar(&$car) { - $car['color'] = 'Red'; + //... } ``` + **[⬆ back to top](#table-of-contents)** ### Use default arguments instead of short circuiting or conditionals From 825836b20197441467ec8300a409515844fe0273 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 19:25:01 +0300 Subject: [PATCH 023/128] optimize example of avoid type-checking 1 --- README.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 4455b29..3830da5 100644 --- a/README.md +++ b/README.md @@ -659,28 +659,34 @@ class Cessna extends Airplane { **[⬆ 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. **Bad:** + ```php -function travelToTexas($vehicle) { +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')); + $vehicle->peddleTo(new Location('texas')); + } elseif ($vehicle instanceof Car) { + $vehicle->driveTo(new Location('texas')); } } ``` **Good**: + ```php -function travelToTexas($vehicle) { - $vehicle->move($this->currentLocation, new Location('texas')); +function travelToTexas(Traveler $vehicle) +{ + $vehicle->travelTo(new Location('texas')); } ``` + **[⬆ back to top](#table-of-contents)** ### Avoid type-checking (part 2) From 783fe13451dcf77513dd4d45549251e791797761 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 19:40:07 +0300 Subject: [PATCH 024/128] Avoid type-checking (part 2) require PHP 7+ --- README.md | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 4455b29..38290b0 100644 --- a/README.md +++ b/README.md @@ -684,32 +684,39 @@ function travelToTexas($vehicle) { **[⬆ 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. +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. **Bad:** + ```php -function combine($val1, $val2) { - if (is_numeric($val1) && is_numeric($val2)) { - return $val1 + $val2; +function combine($val1, $val2) +{ + if (!is_numeric($val1) || !is_numeric($val2)) { + throw new \Exception('Must be of type Number'); } - - throw new \Exception('Must be of type Number'); + + return $val1 + $val2; } ``` **Good**: + ```php -function combine(int $val1, int $val2) { +function combine(int $val1, int $val2) +{ return $val1 + $val2; } ``` + **[⬆ back to top](#table-of-contents)** ### Remove dead code From eccde56b6ba6aaefeeb639da21087b5445b7c880 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Wed, 30 Aug 2017 21:54:23 +0300 Subject: [PATCH 025/128] use compact version of code --- README.md | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 99c2010..4527b7a 100644 --- a/README.md +++ b/README.md @@ -322,7 +322,8 @@ function parseBetterJSAlternative($code) { } ``` -**Good**: +**Good:** + ```php function tokenize($code) { $regexes = [ @@ -382,9 +383,11 @@ 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) { +function showDeveloperList($developers) +{ +    foreach ($developers as $developer) { $expectedSalary = $developer->calculateExpectedSalary(); $experience = $developer->getExperience(); $githubLink = $developer->getGithubLink(); @@ -398,8 +401,9 @@ function showDeveloperList($developers) { } } -function showManagerList($managers) { - foreach($managers as $manager) { +function showManagerList($managers) +{ +    foreach ($managers as $manager) { $expectedSalary = $manager->calculateExpectedSalary(); $experience = $manager->getExperience(); $githubLink = $manager->getGithubLink(); @@ -414,10 +418,34 @@ function showManagerList($managers) { } ``` -**Good**: +**Good:** + +```php +function showList($employees) +{ +    foreach ($employees as $employe) { +        $expectedSalary = $employe->calculateExpectedSalary(); +        $experience = $employe->getExperience(); +        $githubLink = $employe->getGithubLink(); + $data = [ + $expectedSalary, + $experience, + $githubLink + ]; + + render($data); + } +} +``` + +**Very good:** + +It is better to use a compact version of the code. + ```php -function showList($employees) { - foreach($employees as $employe) { +function showList($employees) +{ +    foreach ($employees as $employe) { render([ $employe->calculateExpectedSalary(), $employe->getExperience(), @@ -426,6 +454,7 @@ function showList($employees) { } } ``` + **[⬆ back to top](#table-of-contents)** ### Don't use flags as function parameters From 465ca94b43af0d8b7e10bb26606d45aeb8f8f057 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 13:54:33 +0300 Subject: [PATCH 026/128] add DRY principle --- README.md | 198 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 101 insertions(+), 97 deletions(-) diff --git a/README.md b/README.md index c8e650c..6372942 100644 --- a/README.md +++ b/README.md @@ -11,6 +11,7 @@ 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) ## Introduction @@ -375,103 +376,6 @@ function parseBetterJSAlternative($code) { ``` **[⬆ 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. - -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. - -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); - } -} -``` - -**Good:** - -```php -function showList($employees) -{ -    foreach ($employees as $employe) { -        $expectedSalary = $employe->calculateExpectedSalary(); -        $experience = $employe->getExperience(); -        $githubLink = $employe->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} -``` - -**Very good:** - -It is better to use a compact version of the code. - -```php -function showList($employees) -{ -    foreach ($employees as $employe) { - render([ - $employe->calculateExpectedSalary(), - $employe->getExperience(), - $employe->getGithubLink() - ]); - } -} -``` - -**[⬆ 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 @@ -1516,3 +1420,103 @@ class Employee { } ``` **[⬆ back to top](#table-of-contents)** + +## 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. + +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. + +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. + +**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); + } +} +``` + +**Good:** + +```php +function showList($employees) +{ +    foreach ($employees as $employe) { +        $expectedSalary = $employe->calculateExpectedSalary(); +        $experience = $employe->getExperience(); +        $githubLink = $employe->getGithubLink(); + $data = [ + $expectedSalary, + $experience, + $githubLink + ]; + + render($data); + } +} +``` + +**Very good:** + +It is better to use a compact version of the code. + +```php +function showList($employees) +{ +    foreach ($employees as $employe) { + render([ + $employe->calculateExpectedSalary(), + $employe->getExperience(), + $employe->getGithubLink() + ]); + } +} +``` + +**[⬆ back to top](#table-of-contents)** From 3c9845324fad47c0a4b97377eceec31745b5e30b Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 14:10:18 +0300 Subject: [PATCH 027/128] fix errors and correct CS --- README.md | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index c8e650c..e24c845 100644 --- a/README.md +++ b/README.md @@ -307,31 +307,34 @@ 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. **Bad:** + ```php -function parseBetterJSAlternative($code) { +function parseBetterJSAlternative($code) +{ $regexes = [ // ... ]; $statements = split(' ', $code); $tokens = []; - foreach($regexes as $regex) { - foreach($statements as $statement) { + foreach ($regexes as $regex) { + foreach ($statements as $statement) { // ... } } $ast = []; - foreach($tokens as $token) { + foreach ($tokens as $token) { // lex... } - foreach($ast as $node) { + foreach ($ast as $node) { // parse... } } @@ -340,39 +343,43 @@ function parseBetterJSAlternative($code) { **Good:** ```php -function tokenize($code) { +function tokenize($code) +{ $regexes = [ // ... ]; $statements = split(' ', $code); $tokens = []; - foreach($regexes as $regex) { - foreach($statements as $statement) { + foreach ($regexes as $regex) { + foreach ($statements as $statement) { $tokens[] = /* ... */; - }); - }); + } + } return $tokens; } -function lexer($tokens) { +function lexer($tokens) +{ $ast = []; - foreach($tokens as $token) { + foreach ($tokens as $token) { $ast[] = /* ... */; - }); + } return $ast; } -function parseBetterJSAlternative($code) { +function parseBetterJSAlternative($code) +{ $tokens = tokenize($code); $ast = lexer($tokens); - foreach($ast as $node) { + foreach ($ast as $node) { // parse... - }); + } } ``` + **[⬆ back to top](#table-of-contents)** ### Remove duplicate code From a0d6ea9770419b16e71d5d09dd213e839c80c118 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 14:40:42 +0300 Subject: [PATCH 028/128] move out dependencies --- README.md | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e24c845..de82494 100644 --- a/README.md +++ b/README.md @@ -340,7 +340,9 @@ function parseBetterJSAlternative($code) } ``` -**Good:** +**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) @@ -380,6 +382,72 @@ function parseBetterJSAlternative($code) } ``` +**Good:** + +The best solution is move out the dependencies of `parseBetterJSAlternative()` function. + +```php +class Tokenizer +{ + public function tokenize($code) + { + $regexes = [ + // ... + ]; + + $statements = split(' ', $code); + $tokens = []; + foreach ($regexes as $regex) { + foreach ($statements as $statement) { + $tokens[] = /* ... */; + } + } + + return $tokens; + } +} +``` + +```php +class Lexer +{ + public function lexify($tokens) + { + $ast = []; + foreach ($tokens as $token) { + $ast[] = /* ... */; + } + + return $ast; + } +} +``` + +```php +class BetterJSAlternative +{ + private $tokenizer; + private $lexer; + + 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... + } + } +} +``` + +Now we can mock the dependencies and test only the work of method `BetterJSAlternative::parse()`. + **[⬆ back to top](#table-of-contents)** ### Remove duplicate code From c1e704ce69bf40de04ac9440fad61307f5872afc Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 16:46:29 +0300 Subject: [PATCH 029/128] correct CS --- README.md | 74 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index c8e650c..7e3b579 100644 --- a/README.md +++ b/README.md @@ -942,98 +942,128 @@ class UserSettings { **[⬆ 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. **Bad:** + ```php -abstract class Adapter { +abstract class Adapter +{ protected $name; - public function getName() { + + public function getName() + { return $this->name; } } -class AjaxAdapter extends Adapter { - public function __construct() { +class AjaxAdapter extends Adapter +{ + public function __construct() + { parent::__construct(); $this->name = 'ajaxAdapter'; } } -class NodeAdapter extends Adapter { - public function __construct() { +class NodeAdapter extends Adapter +{ + public function __construct() + { parent::__construct(); $this->name = 'nodeAdapter'; } } -class HttpRequester { +class HttpRequester +{ private $adapter; - public function __construct($adapter) { + + public function __construct($adapter) + { $this->adapter = $adapter; } - public function fetch($url) { + public function fetch($url) + { $adapterName = $this->adapter->getName(); + if ($adapterName === 'ajaxAdapter') { return $this->makeAjaxCall($url); - } else if ($adapterName === 'httpNodeAdapter') { + } elseif ($adapterName === 'httpNodeAdapter') { return $this->makeHttpCall($url); } } - protected function makeAjaxCall($url) { + protected function makeAjaxCall($url) + { // request and return promise } - protected function makeHttpCall($url) { + protected function makeHttpCall($url) + { // request and return promise } } ``` **Good:** + ```php -abstract class Adapter { +abstract class Adapter +{ abstract protected function getName(); + abstract public function request($url); } -class AjaxAdapter extends Adapter { - protected function getName() { +class AjaxAdapter extends Adapter +{ + protected function getName() + { return 'ajaxAdapter'; } - public function request($url) { + public function request($url) + { // request and return promise } } -class NodeAdapter extends Adapter { - protected function getName() { +class NodeAdapter extends Adapter +{ + protected function getName() + { return 'nodeAdapter'; } - public function request($url) { + public function request($url) + { // request and return promise } } -class HttpRequester { +class HttpRequester +{ private $adapter; - public function __construct(Adapter $adapter) { + + public function __construct(Adapter $adapter) + { $this->adapter = $adapter; } - public function fetch($url) { + public function fetch($url) + { return $this->adapter->request($url); } } ``` + **[⬆ back to top](#table-of-contents)** From 14d4efba2064182c2c394dd40ac660629f2a4e84 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 16:49:16 +0300 Subject: [PATCH 030/128] not use abstract class as interface --- README.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 7e3b579..fce3391 100644 --- a/README.md +++ b/README.md @@ -1014,14 +1014,12 @@ class HttpRequester **Good:** ```php -abstract class Adapter +interface Adapter { - abstract protected function getName(); - - abstract public function request($url); + public function request($url); } -class AjaxAdapter extends Adapter +class AjaxAdapter implements Adapter { protected function getName() { @@ -1034,7 +1032,7 @@ class AjaxAdapter extends Adapter } } -class NodeAdapter extends Adapter +class NodeAdapter implements Adapter { protected function getName() { @@ -1061,13 +1059,12 @@ class HttpRequester return $this->adapter->request($url); } } - ``` **[⬆ back to top](#table-of-contents)** - ### 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 From b17ca6eecab2cf87f6cc9854f9a2849589fefcc9 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 16:49:56 +0300 Subject: [PATCH 031/128] remove not used methods --- README.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/README.md b/README.md index fce3391..2a1955d 100644 --- a/README.md +++ b/README.md @@ -1021,11 +1021,6 @@ interface Adapter class AjaxAdapter implements Adapter { - protected function getName() - { - return 'ajaxAdapter'; - } - public function request($url) { // request and return promise @@ -1034,11 +1029,6 @@ class AjaxAdapter implements Adapter class NodeAdapter implements Adapter { - protected function getName() - { - return 'nodeAdapter'; - } - public function request($url) { // request and return promise From 4c2133dbaf22b29d9b045ed957adb6ed43a22796 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Thu, 31 Aug 2017 17:39:13 +0300 Subject: [PATCH 032/128] change example for Function names should say what they do --- README.md | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index c8e650c..6895d0f 100644 --- a/README.md +++ b/README.md @@ -284,26 +284,41 @@ function isClientActive($client) { ### Function names should say what they do **Bad:** + ```php -function addToDate($date, $month) { - // ... -} +class Email +{ + //... -$date = new \DateTime(); + public function handle() + { + mail($this->to, $this->subject, $this->body); + } +} -// It's hard to to tell from the function name what is added -addToDate($date, 1); +$message = new Email(...); +// What is this? A handle for the message? Are we writing to a file now? +$message->handle(); ``` -**Good**: +**Good:** + ```php -function addMonthToDate($month, $date) { - // ... +class Email +{ + //... + + public function send() + { + mail($this->to, $this->subject, $this->body); + } } -$date = new \DateTime(); -addMonthToDate(1, $date); +$message = new Email(...); +// Clear and obvious +$message->send(); ``` + **[⬆ back to top](#table-of-contents)** ### Functions should only be one level of abstraction From 00358108d0e87e750b9064656fcfbdc0b7f0b574 Mon Sep 17 00:00:00 2001 From: "Connor S. Parks" Date: Fri, 1 Sep 2017 10:32:00 +0100 Subject: [PATCH 033/128] Fix Invalid Code The "Avoid Side Effects" section's "Bad" code block is actually invalid code, `splitIntoFirstAndLastName()` will throw a notice of `Undefined variable`. We need a `$global` here. I'm all for competing against code that people may have but there is no point comparing code that doesn't work in the first place, it's out of the bounds of this document. --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index a51aafb..efd6ced 100644 --- a/README.md +++ b/README.md @@ -612,6 +612,8 @@ than the vast majority of other programmers. $name = 'Ryan McDermott'; function splitIntoFirstAndLastName() { + global $name; + $name = preg_split('/ /', $name); } From a77a62d7ce12409b6115f8dd2ff4ab29119111d7 Mon Sep 17 00:00:00 2001 From: Sergiy Litvinchuk Date: Fri, 1 Sep 2017 12:32:09 +0300 Subject: [PATCH 034/128] Pass $name and $email to parent constructor in EmployeeTaxData --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index a51aafb..62f24f6 100644 --- a/README.md +++ b/README.md @@ -1664,8 +1664,8 @@ class Employee { class EmployeeTaxData extends Employee { private $ssn, $salary; - public function __construct($ssn, $salary) { - parent::__construct(); + public function __construct($name, $email, $ssn, $salary) { + parent::__construct($name, $email); $this->ssn = $ssn; $this->salary = $salary; } From 08c8cc7b0791ed38fc11d8dca9d442a31cb85795 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Fri, 1 Sep 2017 18:02:30 +0300 Subject: [PATCH 035/128] missing a second "e" at the end of employee --- README.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 6372942..b7bce13 100644 --- a/README.md +++ b/README.md @@ -1487,10 +1487,10 @@ function showManagerList($managers) ```php function showList($employees) { -    foreach ($employees as $employe) { -        $expectedSalary = $employe->calculateExpectedSalary(); -        $experience = $employe->getExperience(); -        $githubLink = $employe->getGithubLink(); +    foreach ($employees as $employee) { +        $expectedSalary = $employee->calculateExpectedSalary(); +        $experience = $employee->getExperience(); +        $githubLink = $employee->getGithubLink(); $data = [ $expectedSalary, $experience, @@ -1509,11 +1509,11 @@ It is better to use a compact version of the code. ```php function showList($employees) { -    foreach ($employees as $employe) { +    foreach ($employees as $employee) { render([ - $employe->calculateExpectedSalary(), - $employe->getExperience(), - $employe->getGithubLink() + $employee->calculateExpectedSalary(), + $employee->getExperience(), + $employee->getGithubLink() ]); } } From 1a0c0c0f560e376e84bfb7843476edf0aa1d4b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Max=20Str=C3=BCbing?= Date: Sat, 2 Sep 2017 11:31:49 +0200 Subject: [PATCH 036/128] Correct spelling --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 133914a..85eb052 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ $address = 'One Infinite Loop, Cupertino 95014'; $cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; preg_match($cityZipCodeRegex, $address, $matches); -list(, $city, $zipCode) = $matchers; +list(, $city, $zipCode) = $matches; saveCityZipCode($city, $zipCode); ``` From cc9896241ae48a8ce1fb4e6f259b17a04990e5e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emin=20=C5=9Een?= Date: Sat, 2 Sep 2017 15:57:43 +0300 Subject: [PATCH 037/128] Duplicated fixes. Duplicate variables removed. Variable names updated. --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 133914a..2ee3ae1 100644 --- a/README.md +++ b/README.md @@ -528,8 +528,6 @@ var_dump($name); // ['Ryan', 'McDermott']; **Good**: ```php -$name = 'Ryan McDermott'; - function splitIntoFirstAndLastName($name) { return preg_split('/ /', $name); } @@ -970,8 +968,9 @@ your codebase. ```php class UserSettings { private $user; + public function __construct($user) { - $this->user = user; + $this->user = $user; } public function changeSettings($settings) { @@ -990,8 +989,9 @@ class UserSettings { ```php class UserAuth { private $user; + public function __construct($user) { - $this->user = user; + $this->user = $user; } public function verifyCredentials() { @@ -1002,6 +1002,7 @@ class UserAuth { class UserSettings { private $user; + public function __construct($user) { $this->user = $user; $this->auth = new UserAuth($user); From c058329bff60ada383e5445ab3f30fbcc7ff50dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emin=20=C5=9Een?= Date: Sat, 2 Sep 2017 16:08:24 +0300 Subject: [PATCH 038/128] foreach loop syntax fixed. does not use "in" syntax, it uses "as" --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2ee3ae1..ecab842 100644 --- a/README.md +++ b/README.md @@ -1186,12 +1186,12 @@ class Square extends Rectangle { } function renderLargeRectangles($rectangles) { - foreach($rectangle in $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); - }); + } } $rectangles = [new Rectangle(), new Rectangle(), new Square()]; @@ -1250,7 +1250,7 @@ class Square extends Shape { } function renderLargeRectangles($rectangles) { - foreach($rectangle in $rectangles) { + foreach($rectangles as $rectangle) { if ($rectangle instanceof Square) { $rectangle->setLength(5); } else if ($rectangle instanceof Rectangle) { @@ -1260,7 +1260,7 @@ function renderLargeRectangles($rectangles) { $area = $rectangle->getArea(); $rectangle->render($area); - }); + } } $shapes = [new Rectangle(), new Rectangle(), new Square()]; From 0c89c48adb9ff6e4071aef6fe70f0c518039770d Mon Sep 17 00:00:00 2001 From: Guilherme Portela Date: Sun, 3 Sep 2017 10:20:14 -0300 Subject: [PATCH 039/128] Fix variable type in PHPDoc --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 133914a..d4392b6 100644 --- a/README.md +++ b/README.md @@ -1431,7 +1431,7 @@ class SuperWorker implements WorkerInterface { } class Manager { - /** @var Worker $worker **/ + /** @var WorkerInterface $worker **/ private $worker; public function __construct(WorkerInterface $worker) { From 820a6a1d9ecfa9f3588f09b1289712985f01a57f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emin=20=C5=9Een?= Date: Mon, 4 Sep 2017 04:58:47 +0300 Subject: [PATCH 040/128] Fix variable spelling and syntax Functions don't start with "new". "$req" variable incorrect variable name, "$request" good. Class construct syntax. --- README.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index ecab842..8982c4b 100644 --- a/README.md +++ b/README.md @@ -833,8 +833,8 @@ function newRequestModule($url) { // ... } -$req = new newRequestModule($requestUrl); -inventoryTracker('apples', $req, 'www.inventory-awesome.io'); +$request = newRequestModule($requestUrl); +inventoryTracker('apples', $request, 'www.inventory-awesome.io'); ``` @@ -844,8 +844,8 @@ function requestModule($url) { // ... } -$req = new requestModule($requestUrl); -inventoryTracker('apples', $req, 'www.inventory-awesome.io'); +$request = requestModule($requestUrl); +inventoryTracker('apples', $request, 'www.inventory-awesome.io'); ``` **[⬆ back to top](#table-of-contents)** @@ -1215,8 +1215,8 @@ abstract class Shape { } class Rectangle extends Shape { - public function __construct { - parent::__construct(); + public function __construct() { + parent::__construct(); $this->width = 0; $this->height = 0; } @@ -1235,7 +1235,7 @@ class Rectangle extends Shape { } class Square extends Shape { - public function __construct { + public function __construct() { parent::__construct(); $this->length = 0; } From 656e7ff6e6e2606a569e07a51eb9648efffcd4c7 Mon Sep 17 00:00:00 2001 From: eddymens Date: Mon, 4 Sep 2017 08:21:53 +0000 Subject: [PATCH 041/128] fix typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index fee49f2..7f06ac1 100644 --- a/README.md +++ b/README.md @@ -541,7 +541,7 @@ var_dump($newName); // ['Ryan', 'McDermott']; **[⬆ back to top](#table-of-contents)** ### Don't write to global functions -Polluting globals is a bad practice in very languages because you could clash with another +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 From 6e01046113f5419ffb2c2ba48bb866639c44e63b Mon Sep 17 00:00:00 2001 From: Pablo Godinez Date: Mon, 4 Sep 2017 10:57:27 +0200 Subject: [PATCH 042/128] LSP good example correction See #55 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index fee49f2..4825cd7 100644 --- a/README.md +++ b/README.md @@ -1201,7 +1201,7 @@ renderLargeRectangles($rectangles); **Good:** ```php abstract class Shape { - private $width, $height; + protected $width, $height; abstract public function getArea(); From 32344ead21b0be088c1ee352c926191695f904a9 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Mon, 4 Sep 2017 13:03:17 +0300 Subject: [PATCH 043/128] correct access to $width and $height --- README.md | 115 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 4897765..9d41d65 100644 --- a/README.md +++ b/README.md @@ -1145,48 +1145,56 @@ if you model it using the "is-a" relationship via inheritance, you quickly get into trouble. **Bad:** + ```php -class Rectangle { - private $width, $height; - - public function __construct() { +class Rectangle +{ + protected $width + protected $height; + + public function __construct() + { $this->width = 0; $this->height = 0; } - - public function setColor($color) { - // ... - } - - public function render($area) { + + public function render($area) + { // ... } - - public function setWidth($width) { + + public function setWidth($width) + { $this->width = $width; } - - public function setHeight($height) { + + public function setHeight($height) + { $this->height = $height; } - - public function getArea() { + + public function getArea() + { return $this->width * $this->height; } } -class Square extends Rectangle { - public function setWidth($width) { +class Square extends Rectangle +{ + public function setWidth($width) + { $this->width = $this->height = $width; } - - public function setHeight(height) { + + public function setHeight(height) + { $this->width = $this->height = $height; } } -function renderLargeRectangles($rectangles) { - foreach($rectangles as $rectangle) { +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. @@ -1199,61 +1207,71 @@ renderLargeRectangles($rectangles); ``` **Good:** + ```php -abstract class Shape { - protected $width, $height; - +abstract class Shape +{ + protected $width; + protected $height; + abstract public function getArea(); - - public function setColor($color) { - // ... - } - - public function render($area) { + + public function render($area) + { // ... } } -class Rectangle extends Shape { - public function __construct() { +class Rectangle extends Shape +{ + public function __construct() + { parent::__construct(); $this->width = 0; $this->height = 0; } - - public function setWidth($width) { + + public function setWidth($width) + { $this->width = $width; } - - public function setHeight($height) { + + public function setHeight($height) + { $this->height = $height; } - - public function getArea() { + + public function getArea() + { return $this->width * $this->height; } } -class Square extends Shape { - public function __construct() { +class Square extends Shape +{ + public function __construct() + { parent::__construct(); $this->length = 0; } - - public function setLength($length) { + + public function setLength($length) + { $this->length = $length; } - - public function getArea() { + + public function getArea() + { return pow($this->length, 2); } } -function renderLargeRectangles($rectangles) { - foreach($rectangles as $rectangle) { +function renderLargeRectangles($rectangles) +{ + foreach ($rectangles as $rectangle) { if ($rectangle instanceof Square) { $rectangle->setLength(5); - } else if ($rectangle instanceof Rectangle) { + } elseif ($rectangle instanceof Rectangle) { $rectangle->setWidth(4); $rectangle->setHeight(5); } @@ -1266,6 +1284,7 @@ function renderLargeRectangles($rectangles) { $shapes = [new Rectangle(), new Rectangle(), new Square()]; renderLargeRectangles($shapes); ``` + **[⬆ back to top](#table-of-contents)** ### Interface Segregation Principle (ISP) From a71ad82a792399f7a6c9ed6c0808d9e07e0d8342 Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Mon, 4 Sep 2017 13:06:19 +0300 Subject: [PATCH 044/128] forgot ; --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9d41d65..b5ae5f0 100644 --- a/README.md +++ b/README.md @@ -1149,7 +1149,7 @@ get into trouble. ```php class Rectangle { - protected $width + protected $width; protected $height; public function __construct() From aa24debbac9160c31e805ad8edafb07a27e3925f Mon Sep 17 00:00:00 2001 From: Peter Gribanov Date: Tue, 5 Sep 2017 14:08:13 +0300 Subject: [PATCH 045/128] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7958a1b..0530f2f 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,7 @@ function createMicrobrewery($breweryName = 'Hipster Brew Co.') **Not bad:** -This opinion is understandable than the previous version, but it better controls the value of the variable. +This opinion is more understandable than the previous version, but it better controls the value of the variable. ```php function createMicrobrewery($name = null) From dece5d95d6230e548c44c168b447bfd1d9994240 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 5 Sep 2017 15:03:19 +0100 Subject: [PATCH 046/128] Update README.md --- README.md | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 8eff07d..bfc33c5 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# clean-code-php +# clean-code-python ## Table of Contents 1. [Introduction](#introduction) @@ -17,8 +17,8 @@ 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. +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 @@ -30,28 +30,28 @@ Inspired from [clean-code-javascript](https://github.com/ryanmcdermott/clean-cod ### Use meaningful and pronounceable variable names **Bad:** -```php -$ymdstr = $moment->format('y-m-d'); +```python +ymdstr = datetime.date.today().strftime("%y-%m-%d") ``` **Good**: -```php -$currentDate = $moment->format('y-m-d'); +```python +current_date = 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(); +```python +get_user_info() +get_client_data() +get_customer_record() ``` **Good**: -```php -getUser(); +```python +get_user() ``` **[⬆ back to top](#table-of-contents)** @@ -62,20 +62,18 @@ understanding our program, we hurt our readers. Make your names searchable. **Bad:** -```php -// What the heck is 86400 for? -addExpireAt(86400); +```python +# What the heck is 86400 for? +time.sleep(86400); ``` **Good**: -```php -// Declare them as capitalized `const` globals. -interface DateGlobal { - const SECONDS_IN_A_DAY = 86400; -} +```python +# Declare them in the global namespace for the module. +SECONDS_IN_A_DAY = 60 * 60 * 24 -addExpireAt(DateGlobal::SECONDS_IN_A_DAY); +time.sleep(SECONDS_IN_A_DAY) ``` **[⬆ back to top](#table-of-contents)** From 73521874d510cb094e2a1a8f49abd6fc8c581d6c Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 5 Sep 2017 16:18:06 +0100 Subject: [PATCH 047/128] Update README.md --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index bfc33c5..b60a1f1 100644 --- a/README.md +++ b/README.md @@ -80,9 +80,9 @@ time.sleep(SECONDS_IN_A_DAY) ### Use explanatory variables **Bad:** -```php -$address = 'One Infinite Loop, Cupertino 95014'; -$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; +```python +address = 'One Infinite Loop, Cupertino 95014' +city_zip_code_regex = r'/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/' preg_match($cityZipCodeRegex, $address, $matches); saveCityZipCode($matches[1], $matches[2]); From bb2ea04f3ff2bb214289941c50aaad1d41da7f70 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 6 Sep 2017 13:03:33 +0100 Subject: [PATCH 048/128] Update README.md --- README.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index b60a1f1..cd294ae 100644 --- a/README.md +++ b/README.md @@ -82,23 +82,24 @@ time.sleep(SECONDS_IN_A_DAY) **Bad:** ```python address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = r'/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/' -preg_match($cityZipCodeRegex, $address, $matches); +city_zip_code_regex = '^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' +matches = re.match(city_zip_code_regex, address) -saveCityZipCode($matches[1], $matches[2]); +save_city_zip_code(matches[1], matches[2]) ``` **Not bad**: It's better, but we are still heavily dependent on regex. -```php -$address = 'One Infinite Loop, Cupertino 95014'; -$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$/'; -preg_match($cityZipCodeRegex, $address, $matches); +```python +address = 'One Infinite Loop, Cupertino 95014' +city_zip_code_regex = '^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' +matches = re.match(city_zip_code_regex, address) + +city, zip_code = matches.groups() -list(, $city, $zipCode) = $matches; -saveCityZipCode($city, $zipCode); +save_city_zip_code(city, zip_code) ``` **Good**: From c09fea7febae73b3c6bddda1bb0e5f9ca4062e4d Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 6 Sep 2017 13:11:08 +0100 Subject: [PATCH 049/128] Update README.md --- README.md | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index cd294ae..fe06327 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ time.sleep(SECONDS_IN_A_DAY) **Bad:** ```python address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = '^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' +city_zip_code_regex = r'^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' matches = re.match(city_zip_code_regex, address) save_city_zip_code(matches[1], matches[2]) @@ -94,23 +94,22 @@ It's better, but we are still heavily dependent on regex. ```python address = 'One Infinite Loop, Cupertino 95014' -city_zip_code_regex = '^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$' +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) ``` **Good**: Decrease dependence on regex by naming subpatterns. -```php -$address = 'One Infinite Loop, Cupertino 95014'; -$cityZipCodeRegex = '/^[^,\\]+[,\\\s]+(?.+?)\s*(?\d{5})?$/'; -preg_match($cityZipCodeRegex, $address, $matches); +```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) -saveCityZipCode($matches['city'], $matches['zipCode']); +save_city_zip_code(matches['city'], matches['zip_code']) ``` **[⬆ back to top](#table-of-contents)** From 348f6994dfdf1c9eab4e7f9a7ac782ea94437a72 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 6 Sep 2017 13:17:13 +0100 Subject: [PATCH 050/128] Update README.md --- README.md | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index fe06327..309c1b4 100644 --- a/README.md +++ b/README.md @@ -118,33 +118,26 @@ 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']; +```python +seq = ('Austin', 'New York', 'San Francisco') -for ($i = 0; $i < count($l); $i++) { - $li = $l[$i]; - doStuff(); - doSomeOtherStuff(); - // ... - // ... - // ... - // Wait, what is `$li` for again? - dispatch($li); -} +for item in seq: + do_stuff() + do_some_other_stuff() + # ... + # Wait, what's `item` for again? + dispatch(item) ``` **Good**: -```php -$locations = ['Austin', 'New York', 'San Francisco']; +```python +locations = ('Austin', 'New York', 'San Francisco') -foreach ($locations as $location) { - doStuff(); - doSomeOtherStuff(); - // ... - // ... - // ... - dispatch($location); -}); +for location in locations: + do_stuff() + do_some_other_stuff() + # ... + dispatch(location) ``` **[⬆ back to top](#table-of-contents)** From 5846bf603ceb0b4c407b43daf9f96d557dbbbf67 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 28 Aug 2018 17:17:10 +0100 Subject: [PATCH 051/128] Support Python 3.7+ only --- README.md | 182 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 115 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index 309c1b4..5de79ba 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,8 @@ years of collective experience by the authors of *Clean Code*. Inspired from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript) +Targets Python3.7+ + ## **Variables** ### Use meaningful and pronounceable variable names @@ -36,13 +38,14 @@ ymdstr = datetime.date.today().strftime("%y-%m-%d") **Good**: ```python -current_date = datetime.date.today().strftime("%y-%m-%d") +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() @@ -50,9 +53,29 @@ get_customer_record() ``` **Good**: +If the entity is the same, you should be consistent in referring to it in your functions: ```python -get_user() +get_user_info() +get_user_data() +get_user_record() ``` + +**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 +class User: + info : str + + @property + def data(self) -> dict: + # ... + + def get_record(self) -> Union[Record, None]: + # ... +``` + **[⬆ back to top](#table-of-contents)** ### Use searchable names @@ -65,7 +88,6 @@ Make your names searchable. ```python # What the heck is 86400 for? time.sleep(86400); - ``` **Good**: @@ -77,7 +99,6 @@ time.sleep(SECONDS_IN_A_DAY) ``` **[⬆ back to top](#table-of-contents)** - ### Use explanatory variables **Bad:** ```python @@ -149,66 +170,46 @@ variable name. **Bad:** -```php -class Car -{ - public $carMake; - public $carModel; - public $carColor; - - //... -} +```python +class Car: + car_make: str + car_model: str + car_color: str ``` **Good**: -```php -class Car -{ - public $make; - public $model; - public $color; - - //... -} +```python +class Car: + make: str + model: str + color: str ``` **[⬆ back to top](#table-of-contents)** ### Use default arguments instead of short circuiting or conditionals -**Not good:** +**Tricky** -This is not good because `$breweryName` can be `NULL`. +Why write: -```php -function createMicrobrewery($breweryName = 'Hipster Brew Co.') -{ -    // ... -} +```python +def create_micro_brewery(name): + name = "Hipster Brew Co." if name is None else name + slug = hashlib.sha1(name.encode()).hexdigest() + # etc. ``` -**Not bad:** - -This opinion is more understandable than the previous version, but it better controls the value of the variable. - -```php -function createMicrobrewery($name = null) -{ -    $breweryName = $name ?: 'Hipster Brew Co.'; - // ... -} -``` +... when you can specify a default argument instead? This also makes ist clear that +you are expecting a string as the argument. **Good**: -If you support only PHP 7+, then you can use [type hinting](http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration) and be sure that the `$breweryName` will not be `NULL`. - -```php -function createMicrobrewery(string $breweryName = 'Hipster Brew Co.') -{ -    // ... -} +```python +def create_micro_brewery(name: str="Hipster Brew Co."): + slug = hashlib.sha1(name.encode()).hexdigest() + # etc. ``` **[⬆ back to top](#table-of-contents)** @@ -224,33 +225,80 @@ arguments then your function is trying to do too much. In cases where it's not, of the time a higher-level object will suffice as an argument. **Bad:** -```php -function createMenu($title, $body, $buttonText, $cancellable) { - // ... -} +```python +def create_menu(title, body, button_text, cancellable): + # ... ``` **Good**: -```php -class MenuConfig -{ - public $title; - public $body; - public $buttonText; - public $cancellable = false; -} +```python +class Menu: + def __init__(self, config: dict): + title = config["title"] + body = config["body"] + # ... -$config = new MenuConfig(); -$config->title = 'Foo'; -$config->body = 'Bar'; -$config->buttonText = 'Baz'; -$config->cancellable = true; +menu = Menu( + { + "title": "My Menu", + "body": "Something about my menu", + "button_text": "OK", + "cancellable": False + } +) +``` + +**Also good** +```python +class MenuConfig: + title: str + body: str + button_text: str + cancellable: bool = False + + +def create_menu(config: MenuConfig): + title = config.title + body = config.body + # ... -function createMenu(MenuConfig $config) { - // ... -} +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 + +create_menu(config) ``` + +**Fancy** +```python +from typing import NamedTuple + + +class MenuConfig(NamedTuple): + title: str + body: str + button_text: str + cancellable: bool = False + + +def create_menu(config: MenuConfig): + title, body, button_text, cancellable = config + + +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)** From 85eac5bf197a795ad8e71ce25e0649d8722e73d8 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 28 Aug 2018 17:24:02 +0100 Subject: [PATCH 052/128] Add a Google-style docstring for the classes --- README.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/README.md b/README.md index 5de79ba..117662b 100644 --- a/README.md +++ b/README.md @@ -251,6 +251,14 @@ menu = Menu( **Also good** ```python 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? + """ title: str body: str button_text: str @@ -279,6 +287,14 @@ from typing import NamedTuple class MenuConfig(NamedTuple): + """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 From 71a235b83afa44d51e2351cf8ebbdafca226b672 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 28 Aug 2018 17:45:55 +0100 Subject: [PATCH 053/128] Add a generator example --- README.md | 60 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 117662b..f43d932 100644 --- a/README.md +++ b/README.md @@ -314,10 +314,8 @@ 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 @@ -326,33 +324,49 @@ cleaner. If you take nothing else away from this guide other than this, you'll b of many developers. **Bad:** -```php -function emailClients($clients) { - foreach ($clients as $client) { - $clientRecord = $db->find($client); - if ($clientRecord->isActive()) { - email($client); - } - } -} +```python + +def email_clients(clients: List[Client]): + """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'); -} +```python +def get_active_clients(clients: List[Client]) -> List[Client]: + """Filter active clients. + """ + return [client for client in clients if client.active] -function activeClients($clients) { - return array_filter($clients, 'isClientActive'); -} -function isClientActive($client) { - $clientRecord = $db->find($client); - return $clientRecord->isActive(); -} +def email_clients(clients: List[Client, ...]) -> None: + """Send an email to a given list of clients. + """ + for client in clients: + email(client) ``` + +Do you see an opportunity for using generators now? + +**Even better** +```python +def active_clients(clients: List[Client]) -> Generator[Client]: + """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 clients: + email(client) +``` + + **[⬆ back to top](#table-of-contents)** ### Function names should say what they do From ffb5fa3709e74a81c1c0022e7d2873790d18444e Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 28 Aug 2018 21:15:15 +0200 Subject: [PATCH 054/128] Add a section on naming methods --- README.md | 40 ++++++++++++++-------------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index f43d932..0b7bdc6 100644 --- a/README.md +++ b/README.md @@ -373,38 +373,26 @@ def email_client(clients: Iterator[Client]) -> None: **Bad:** -```php -class Email -{ - //... - - public function handle() - { - mail($this->to, $this->subject, $this->body); - } -} +```python +class Email: + def handle(self) -> None: + # Do something... -$message = new Email(...); -// What is this? A handle for the message? Are we writing to a file now? -$message->handle(); +message = Email() +# What is this supposed to do again? +message.handle() ``` **Good:** -```php -class Email -{ - //... - - public function send() - { - mail($this->to, $this->subject, $this->body); - } -} +```python +class Email: + def send(self) -> None: + """Send this message. + """ -$message = new Email(...); -// Clear and obvious -$message->send(); +message = Email() +message.send() ``` **[⬆ back to top](#table-of-contents)** From 24e90a955a3a06ae1d39f2590cc6fa8d0f8b7045 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Mon, 1 Oct 2018 21:07:30 +0200 Subject: [PATCH 055/128] Remove the untranslated parts for now --- README.md | 1407 ----------------------------------------------------- 1 file changed, 1407 deletions(-) diff --git a/README.md b/README.md index 0b7bdc6..76ca493 100644 --- a/README.md +++ b/README.md @@ -396,1410 +396,3 @@ message.send() ``` **[⬆ 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. - -**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... - } -} -``` - -**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... - } -} -``` - -**Good:** - -The best solution is move out the dependencies of `parseBetterJSAlternative()` function. - -```php -class Tokenizer -{ - public function tokenize($code) - { - $regexes = [ - // ... - ]; - - $statements = split(' ', $code); - $tokens = []; - foreach ($regexes as $regex) { - foreach ($statements as $statement) { - $tokens[] = /* ... */; - } - } - - return $tokens; - } -} -``` - -```php -class Lexer -{ - public function lexify($tokens) - { - $ast = []; - foreach ($tokens as $token) { - $ast[] = /* ... */; - } - - return $ast; - } -} -``` - -```php -class BetterJSAlternative -{ - private $tokenizer; - private $lexer; - - 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... - } - } -} -``` - -Now we can mock the dependencies and test only the work of method `BetterJSAlternative::parse()`. - -**[⬆ 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. - -**Bad:** -```php -function createFile($name, $temp = false) { - if ($temp) { - touch('./temp/'.$name); - } else { - touch($name); - } -} -``` - -**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. - -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'; - -function splitIntoFirstAndLastName() { - global $name; - - $name = preg_split('/ /', $name); -} - -splitIntoFirstAndLastName(); - -var_dump($name); // ['Ryan', 'McDermott']; -``` - -**Good**: -```php -function splitIntoFirstAndLastName($name) { - return preg_split('/ /', $name); -} - -$name = 'Ryan McDermott'; -$newName = splitIntoFirstAndLastName($name); - -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. - -**Bad:** - -```php -function config() -{ - return [ - 'foo' => 'bar', - ] -} -``` - -**Good:** - -Create PHP configuration file or something else - -```php -// config.php -return [ - 'foo' => 'bar', -]; -``` - -```php -class Configuration -{ - private $configuration = []; - - public function __construct(array $configuration) - { - $this->configuration = $configuration; - } - - public function get($key) - { - return isset($this->configuration[$key]) ? $this->configuration[$key] : null; - } -} -``` - -Load configuration from file and create instance of `Configuration` class - -```php -$configuration = new Configuration($configuration); -``` - -And now you must use instance of `Configuration` in your application. - -**[⬆ back to top](#table-of-contents)** - -### Don't use a Singleton pattern -Singleton is a [anti-pattern](https://en.wikipedia.org/wiki/Singleton_pattern). - -**Bad:** - -```php -class DBConnection -{ - private static $instance; - - private function __construct($dsn) - { - // ... - } - - public static function getInstance() - { - if (self::$instance === null) { - self::$instance = new self(); - } - - return self::$instance; - } - - // ... -} - -$singleton = DBConnection::getInstance(); -``` - -**Good:** - -```php -class DBConnection -{ - public function __construct(array $dsn) - { - // ... - } - - // ... -} -``` - -Create instance of `DBConnection` class and configure it with [DSN](http://php.net/manual/en/pdo.construct.php#refsect1-pdo.construct-parameters). - -```php -$connection = new DBConnection($dsn); -``` - -And now you must use instance of `DBConnection` in your application. - -**[⬆ back to top](#table-of-contents)** - -### Encapsulate conditionals - -**Bad:** - -```php -if ($article->state === 'published') { - // ... -} -``` - -**Good**: - -```php -if ($article->isPublished()) { - // ... -} -``` - -**[⬆ back to top](#table-of-contents)** - -### Avoid negative conditionals - -**Bad:** -```php -function isDOMNodeNotPresent($node) { - // ... -} - -if (!isDOMNodeNotPresent($node)) { - // ... -} -``` - -**Good**: -```php -function isDOMNodePresent($node) { - // ... -} - -if (isDOMNodePresent($node)) { - // ... -} -``` -**[⬆ 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. - -**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(); - } - } -} -``` - -**Good**: -```php -class Airplane { - // ... -} - -class Boeing777 extends Airplane { - // ... - public function getCruisingAltitude() { - return $this->getMaxAltitude() - $this->getPassengerCount(); - } -} - -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)** - -### 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. - -**Bad:** - -```php -function travelToTexas($vehicle) -{ - if ($vehicle instanceof Bicycle) { - $vehicle->peddleTo(new Location('texas')); - } elseif ($vehicle instanceof Car) { - $vehicle->driveTo(new Location('texas')); - } -} -``` - -**Good**: - -```php -function travelToTexas(Traveler $vehicle) -{ - $vehicle->travelTo(new Location('texas')); -} -``` - -**[⬆ 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 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. - -**Bad:** - -```php -function combine($val1, $val2) -{ - if (!is_numeric($val1) || !is_numeric($val2)) { - throw new \Exception('Must be of type Number'); - } - - return $val1 + $val2; -} -``` - -**Good**: - -```php -function combine(int $val1, int $val2) -{ - return $val1 + $val2; -} -``` - -**[⬆ 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. - -**Bad:** -```php -function oldRequestModule($url) { - // ... -} - -function newRequestModule($url) { - // ... -} - -$request = newRequestModule($requestUrl); -inventoryTracker('apples', $request, 'www.inventory-awesome.io'); - -``` - -**Good**: -```php -function requestModule($url) { - // ... -} - -$request = requestModule($requestUrl); -inventoryTracker('apples', $request, 'www.inventory-awesome.io'); -``` -**[⬆ back to top](#table-of-contents)** - - -## **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. - -Additionally, this is part of Open/Closed principle, from object-oriented -design principles. - -**Bad:** -```php -class BankAccount { - public $balance = 1000; -} - -$bankAccount = new BankAccount(); - -// 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; - } -} - -$bankAccount = new BankAccount(); - -// Buy shoes... -$bankAccount->withdrawBalance($shoesPrice); - -// Get balance -$balance = $bankAccount->getBalance(); - -``` -**[⬆ back to top](#table-of-contents)** - - -### Make objects have private/protected members - -**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 -``` - -**Good**: -```php -class Employee { - protected $name; - - public function __construct($name) { - $this->name = $name; - } - - public function getName() { - return $this->name; - } -} - -$employee = new Employee('John Doe'); -echo 'Employee name: '.$employee->getName(); // Employee name: John Doe -``` -**[⬆ back to top](#table-of-contents)** - - -## **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. - -**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; - - public function __construct($user) { - $this->user = $user; - } - - public function verifyCredentials() { - // ... - } -} - - -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. - -**Bad:** - -```php -abstract class Adapter -{ - protected $name; - - public function getName() - { - return $this->name; - } -} - -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 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); - } elseif ($adapterName === 'httpNodeAdapter') { - return $this->makeHttpCall($url); - } - } - - protected function makeAjaxCall($url) - { - // request and return promise - } - - protected function makeHttpCall($url) - { - // request and return promise - } -} -``` - -**Good:** - -```php -interface Adapter -{ - public function request($url); -} - -class AjaxAdapter implements Adapter -{ - public function request($url) - { - // request and return promise - } -} - -class NodeAdapter implements Adapter -{ - public function request($url) - { - // request and return promise - } -} - -class HttpRequester -{ - private $adapter; - - public function __construct(Adapter $adapter) - { - $this->adapter = $adapter; - } - - public function fetch($url) - { - return $this->adapter->request($url); - } -} -``` - -**[⬆ back to top](#table-of-contents)** - -### 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 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. - -**Bad:** - -```php -class Rectangle -{ - protected $width; - protected $height; - - public function __construct() - { - $this->width = 0; - $this->height = 0; - } - - 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; - } -} - -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 ($rectangles as $rectangle) { - $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 -{ - protected $width; - protected $height; - - abstract public function getArea(); - - public function render($area) - { - // ... - } -} - -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; - } -} - -class Square extends Shape -{ - public function __construct() - { - parent::__construct(); - $this->length = 0; - } - - public function setLength($length) - { - $this->length = $length; - } - - 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); - } -} - -$shapes = [new Rectangle(), new Rectangle(), new Square()]; -renderLargeRectangles($shapes); -``` - -**[⬆ back to top](#table-of-contents)** - -### Interface Segregation Principle (ISP) -ISP states that "Clients should not be forced to depend upon interfaces that -they do not use." - -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". - -**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 - } -} - -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } - - public function eat() { - //.... eating in launch break - } -} - -class Manager { - /** @var WorkerInterface $worker **/ - private $worker; - - public function setWorker(WorkerInterface $worker) { - $this->worker = $worker; - } - - public function manage() { - $this->worker->work(); - } -} -``` - -**Good:** -```php -interface WorkerInterface extends FeedableInterface, WorkableInterface { -} - -interface WorkableInterface { - public function work(); -} - -interface FeedableInterface { - public function eat(); -} - -class Worker implements WorkableInterface, FeedableInterface { - public function work() { - // ....working - } - - public function eat() { - //.... eating in launch break - } -} - -class Robot implements WorkableInterface { - public function work() { - // ....working - } -} - -class SuperWorker implements WorkerInterface { - public function work() { - //.... working much more - } - - public function eat() { - //.... eating in launch break - } -} - -class Manager { - /** @var $worker WorkableInterface **/ - private $worker; - - public function setWorker(WorkableInterface $w) { - $this->worker = $w; - } - - public function manage() { - $this->worker->work(); - } -} -``` -**[⬆ 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. - -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. - -**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 - } -} -``` - -**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(); - } -} - -``` -**[⬆ 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. - -**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); - } -} - -$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); - } -} - -$car = (new Car()) - ->setColor('pink') - ->setMake('Ford') - ->setModel('F-150') - ->dump(); -``` -**[⬆ 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. - -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: - -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). - -**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); - } - // ... -} -``` -**[⬆ back to top](#table-of-contents)** - -## 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. - -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. - -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. - -**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); - } -} -``` - -**Good:** - -```php -function showList($employees) -{ -    foreach ($employees as $employee) { -        $expectedSalary = $employee->calculateExpectedSalary(); -        $experience = $employee->getExperience(); -        $githubLink = $employee->getGithubLink(); - $data = [ - $expectedSalary, - $experience, - $githubLink - ]; - - render($data); - } -} -``` - -**Very good:** - -It is better to use a compact version of the code. - -```php -function showList($employees) -{ -    foreach ($employees as $employee) { - render([ - $employee->calculateExpectedSalary(), - $employee->getExperience(), - $employee->getGithubLink() - ]); - } -} -``` - -**[⬆ back to top](#table-of-contents)** From d1be3e18ef53b148331bba94ad13f56dab79b51a Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Mon, 1 Oct 2018 21:08:40 +0200 Subject: [PATCH 056/128] Update the linguist language --- .gitattributes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From b1d3ef1ff0c9ad381e34363fdba37b64a3af4f7a Mon Sep 17 00:00:00 2001 From: zacharyaanglin Date: Sun, 10 Mar 2019 14:18:13 -0400 Subject: [PATCH 057/128] Added dataclass example to MenuConfig --- README.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/README.md b/README.md index 76ca493..9ba8694 100644 --- a/README.md +++ b/README.md @@ -303,6 +303,41 @@ class MenuConfig(NamedTuple): def create_menu(config: MenuConfig): title, body, button_text, cancellable = config + # ... + + +create_menu( + MenuConfig( + title="My delicious menu", + body="A description of the various items on the menu", + button_text="Order now!" + ) +) +``` + +**Even fancier** +```python +from dataclasses import astuple, dataclass + + +@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? + """ + title: str + body: str + button_text: str + cancellable: bool = False + +def create_menu(config: MenuConfig): + title, body, button_text, cancellable = astuple(config) + # ... create_menu( From e9d78293fea352bdb3157dabe71d038b27a848fb Mon Sep 17 00:00:00 2001 From: Zachary Anglin Date: Sun, 31 Mar 2019 10:25:28 -0400 Subject: [PATCH 058/128] Add `Don't use flags as function parameters` subsection. --- README.md | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/README.md b/README.md index 9ba8694..69a20d7 100644 --- a/README.md +++ b/README.md @@ -431,3 +431,95 @@ message.send() ``` **[⬆ 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. + +**Bad:** + +```python +def parse_better_js_alternative(code: str) -> None: + regexes = [ + # ... + ] + + statements = regexes.split() + tokens = [] + for regex in regexes: + for statement in statements: + # ... + + ast = [] + for token in tokens: + # Lex. + + for node in ast: + # Parse. +``` + +**Good:** + +```python +def parse_better_js_alternative(code: str) -> None: + tokens = tokenize(code) + syntax_tree = parse(tokens) + + for node in syntax_tree: + # Parse. + + +def tokenize(code: str) -> list: + REGEXES = [ + # ... + ] + + statements = code.split() + tokens = [] + for regex in REGEXES: + for statement in statements: + # Append the statement to tokens. + + return tokens + + +def parse(tokens: list) -> list: + syntax_tree = [] + for token in tokens: + # Append the parsed token to the syntax tree. + + return syntax_tree +``` + +**[⬆ 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 your functions if they are following different code paths based on a boolean. + +**Bad:** + +```python +from pathlib import Path + +def create_file(name: str, temp: bool) -> None: + if temp: + Path('./temp/' + name).touch() + else: + Path(name).touch() +``` + +**Good:** + +```python +from pathlib import Path + +def create_file(name: str) -> None: + Path(name).touch() + +def create_temp_file(name: str) -> None: + Path('./temp/' + name).touch() +``` + +**[⬆ back to top](#table-of-contents)** From e94c4b9891f87ea5dafcf00d410bdb53379d9dcf Mon Sep 17 00:00:00 2001 From: Zachary Anglin Date: Sun, 31 Mar 2019 10:43:02 -0400 Subject: [PATCH 059/128] Added `Avoid Side Effects` sub-section. Edited wrapping lines Removed title case on Avoid Side Effects `Avoid side effects` --- README.md | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 69a20d7..28921d1 100644 --- a/README.md +++ b/README.md @@ -207,7 +207,7 @@ you are expecting a string as the argument. **Good**: ```python -def create_micro_brewery(name: str="Hipster Brew Co."): +def create_micro_brewery(name: str = "Hipster Brew Co."): slug = hashlib.sha1(name.encode()).hexdigest() # etc. ``` @@ -496,7 +496,9 @@ def parse(tokens: list) -> list: ### 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. +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:** @@ -523,3 +525,52 @@ def create_temp_file(name: str) -> None: ``` **[⬆ 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 - 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, +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:** + +```python +# Global variable referenced by following function. +# If another function used this name, now it'd be an array and could break. +name = 'Ryan McDermott' + +def split_into_first_and_last_name() -> None: + global name + name = name.split() + +split_into_first_and_last_name() + +print(name) # ['Ryan', 'McDermott'] +``` + +**Good:** +```python +def split_into_first_and_last_name(name: str) -> None: + return name.split() + +name = 'Ryan McDermott' +new_name = split_into_first_and_last_name(name) + +print(name) # 'Ryan McDermott' +print(new_name) # ['Ryan', 'McDermott'] +``` + +**[⬆ back to top](#table-of-contents)** + From 374f710491147bfd884afb2eeb2731d04f46a045 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 19 Apr 2019 09:43:35 +0200 Subject: [PATCH 060/128] Add an extra case to the 'Avoid Side Effects' section --- README.md | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 28921d1..0165301 100644 --- a/README.md +++ b/README.md @@ -529,9 +529,9 @@ def create_temp_file(name: str) -> None: ### 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. +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 @@ -541,8 +541,8 @@ several functions and classes that write to a particular file - rather, have one 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. +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:** @@ -572,5 +572,22 @@ print(name) # 'Ryan McDermott' print(new_name) # ['Ryan', 'McDermott'] ``` +**Also good** +```python +class Person: + name: str + + def __init__(self, name): + self.name = name + + @property + def name_as_first_and_last(self): + return self.name.split() + +person = Person('Ryan McDermott') +print(person.name) # 'Ryan McDermott' +print(person.name_as_first_and_last) # ['Ryan', 'McDermott'] +``` + **[⬆ back to top](#table-of-contents)** From e96896e5ea4806c7fc0472e441470b1b914f1194 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 19 Apr 2019 09:46:38 +0200 Subject: [PATCH 061/128] Mention the contributors to this project --- CONTRIBUTORS | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 CONTRIBUTORS diff --git a/CONTRIBUTORS b/CONTRIBUTORS new file mode 100644 index 0000000..6af981d --- /dev/null +++ b/CONTRIBUTORS @@ -0,0 +1,2 @@ +Rigel Di Scala +Zachary Anglin From e202f6f21d2a7c7e148cc2a9f9f14d052758975e Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 19 Apr 2019 09:48:34 +0200 Subject: [PATCH 062/128] Add missing type hints --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0165301..f3da6d8 100644 --- a/README.md +++ b/README.md @@ -577,11 +577,11 @@ print(new_name) # ['Ryan', 'McDermott'] class Person: name: str - def __init__(self, name): + def __init__(self, name: str): self.name = name @property - def name_as_first_and_last(self): + def name_as_first_and_last(self) -> list: return self.name.split() person = Person('Ryan McDermott') From 69682e3adee4768f4d7cab12418c0e2ec3de136d Mon Sep 17 00:00:00 2001 From: Mandar Vaze Date: Thu, 4 Jul 2019 20:06:34 +0530 Subject: [PATCH 063/128] Added placeholders for remaining sections Partially addresses #3 --- README.md | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f3da6d8..3c769b7 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ 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) + 6. [Don't repeat yourself (DRY)](#dont-repeat-yourself-dry) ## Introduction @@ -591,3 +591,27 @@ print(person.name_as_first_and_last) # ['Ryan', 'McDermott'] **[⬆ back to top](#table-of-contents)** +## **Objects and Data Structures** + +*Coming soon* + +**[⬆ back to top](#table-of-contents)** + +## **Classes** + +### **Single Responsibility Principle (SRP)** +### **Open/Closed Principle (OCP)** +### **Liskov Substitution Principle (LSP)** +### **Interface Segregation Principle (ISP)** +### **Dependency Inversion Principle (DIP)** + +*Coming soon* + +**[⬆ back to top](#table-of-contents)** + +## **Don't repeat yourself (DRY)** + +*Coming soon* + +**[⬆ back to top](#table-of-contents)** + From 629a21e50455e683f838b87619bfcfa739087b2f Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 5 Jul 2019 19:13:20 +0200 Subject: [PATCH 064/128] Move the constant out of the function and make it immutable --- README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 3c769b7..85b56fc 100644 --- a/README.md +++ b/README.md @@ -462,6 +462,12 @@ def parse_better_js_alternative(code: str) -> None: **Good:** ```python + +REGEXES = ( + # ... +) + + def parse_better_js_alternative(code: str) -> None: tokens = tokenize(code) syntax_tree = parse(tokens) @@ -471,10 +477,6 @@ def parse_better_js_alternative(code: str) -> None: def tokenize(code: str) -> list: - REGEXES = [ - # ... - ] - statements = code.split() tokens = [] for regex in REGEXES: From e007beb9f7b7a2868f36109124274a0c0fa35d22 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 5 Jul 2019 19:21:56 +0200 Subject: [PATCH 065/128] Exploit the opportunity for using a dataclass --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 85b56fc..ca47ad6 100644 --- a/README.md +++ b/README.md @@ -576,12 +576,12 @@ print(new_name) # ['Ryan', 'McDermott'] **Also good** ```python +from dataclasses import dataclass + +@dataclass class Person: name: str - - def __init__(self, name: str): - self.name = name - +######### @property def name_as_first_and_last(self) -> list: return self.name.split() From 9d712abf2083655bd84c3347144f7b9a56d29c9d Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 5 Jul 2019 19:22:42 +0200 Subject: [PATCH 066/128] Remove refactoring debris --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index ca47ad6..c18c554 100644 --- a/README.md +++ b/README.md @@ -581,7 +581,7 @@ from dataclasses import dataclass @dataclass class Person: name: str -######### + @property def name_as_first_and_last(self) -> list: return self.name.split() From 4112718ec7881c45195283f9e9b4254be0a0e067 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 5 Jul 2019 19:30:19 +0200 Subject: [PATCH 067/128] Add a more elaborate description to the side effects section --- README.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c18c554..df2047e 100644 --- a/README.md +++ b/README.md @@ -549,17 +549,24 @@ If you can do this, you will be happier than the vast majority of other programm **Bad:** ```python -# Global variable referenced by following function. -# If another function used this name, now it'd be an array and could break. +# This is a module-level name. +# It's good practice to define these as immutable values, such as a string. +# However... name = 'Ryan McDermott' 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 name name = name.split() split_into_first_and_last_name() print(name) # ['Ryan', 'McDermott'] + +# OK. It worked the first time, but what will happen if we call the +# function again? ``` **Good:** @@ -586,6 +593,7 @@ class Person: 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'] From be6b75d62f4eee09b66441f853af747f696aef4a Mon Sep 17 00:00:00 2001 From: AirbusDriver Date: Thu, 7 Nov 2019 20:58:31 -0500 Subject: [PATCH 068/128] Update `split_into_first_and_last_name` Fix the return signature to reflect the List[str] return value, instead of `None` --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index df2047e..289bdcb 100644 --- a/README.md +++ b/README.md @@ -571,7 +571,7 @@ print(name) # ['Ryan', 'McDermott'] **Good:** ```python -def split_into_first_and_last_name(name: str) -> None: +def split_into_first_and_last_name(name: str) -> list: return name.split() name = 'Ryan McDermott' From d7bdbc7d53c6f9eda29c644d2178a8fc102ed1b5 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 8 Nov 2019 09:18:39 +0100 Subject: [PATCH 069/128] Update CONTRIBUTORS --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 6af981d..ce5bb4a 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -1,2 +1,3 @@ Rigel Di Scala Zachary Anglin +AirbusDriver From 76e2f72d888ec3e3bd82b84ece0dc455ef48ea35 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 7 Feb 2020 22:19:42 +0100 Subject: [PATCH 070/128] Fix a typo: should use an instance and not the class itself The documentation contained a typo, that @mpkocher found, that omitted the round brackets and therefore suggested using a class to store configuration values, which is clearly an antipattern. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 289bdcb..dcf3f83 100644 --- a/README.md +++ b/README.md @@ -271,7 +271,7 @@ def create_menu(config: MenuConfig): # ... -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!" From d4b708638f502d959fdc54700a9867745302b7e7 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 7 Feb 2020 22:22:53 +0100 Subject: [PATCH 071/128] Add mpkocher as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index ce5bb4a..5257c62 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -1,3 +1,4 @@ Rigel Di Scala Zachary Anglin AirbusDriver +Micheal From 10066a457f656e11d783f38d3551065404421709 Mon Sep 17 00:00:00 2001 From: Erik OShaughnessy Date: Tue, 4 Feb 2020 14:14:49 -0600 Subject: [PATCH 072/128] Correct typo: ist -> it --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index dcf3f83..e7c20ff 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,7 @@ def create_micro_brewery(name): # etc. ``` -... when you can specify a default argument instead? This also makes ist clear that +... when you can specify a default argument instead? This also makes it clear that you are expecting a string as the argument. **Good**: From ca7a4e29b2302a5ea4db0906248fe31d665b20f3 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 7 Feb 2020 22:30:17 +0100 Subject: [PATCH 073/128] Add Erik as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 5257c62..9de3603 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -2,3 +2,4 @@ Rigel Di Scala Zachary Anglin AirbusDriver Micheal +Erik OShaughnessy \ No newline at end of file From ea1ec01c2ace08b088509bae1fb8d95652e2e717 Mon Sep 17 00:00:00 2001 From: Rigel Date: Fri, 7 Feb 2020 22:36:11 +0100 Subject: [PATCH 074/128] Add Mukhammad as a contributor --- CONTRIBUTORS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 9de3603..39e7806 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -2,4 +2,5 @@ Rigel Di Scala Zachary Anglin AirbusDriver Micheal -Erik OShaughnessy \ No newline at end of file +Erik OShaughnessy +Mukhammad Karimov \ No newline at end of file From c329c2b92cbf3dfaee0dd5db2b5c46dee34d1c11 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:17:23 +0200 Subject: [PATCH 075/128] Implement automated testing of code snippets This introduces an automated testing framework that validates the code snippets in the README. The snippets themselves have been tweaked so that they pass the tests successfully. --- .gitignore | 129 +++++++++++++++++++++++++++++++++ Makefile | 24 +++++++ README.md | 181 ++++++++++++++++++++++++++++------------------- conftest.py | 61 ++++++++++++++++ requirements.txt | 2 + 5 files changed, 326 insertions(+), 71 deletions(-) create mode 100644 .gitignore create mode 100644 Makefile create mode 100644 conftest.py create mode 100644 requirements.txt 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/Makefile b/Makefile new file mode 100644 index 0000000..7947ecf --- /dev/null +++ b/Makefile @@ -0,0 +1,24 @@ +.PHONY: deps clean tests + +ENV=.env +PYTHON=python3.7 +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 pytest + +tests: ${ENV} ${SITE_PACKAGES}/pytest.py + @${IN_ENV} pytest + +clean: + @rm -rf ${ENV} .env dist .pytest_cache diff --git a/README.md b/README.md index e7c20ff..2449120 100644 --- a/README.md +++ b/README.md @@ -11,13 +11,13 @@ 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) + 6. [Don"t repeat yourself (DRY)](#dont-repeat-yourself-dry) ## Introduction -Software engineering principles, from Robert C. Martin's book +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 +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 @@ -47,17 +47,17 @@ current_date: str = datetime.date.today().strftime("%y-%m-%d") **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: ```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** @@ -65,21 +65,26 @@ Python is (also) an object oriented programming language. If it makes sense, pac of the entity in your code, as instance attributes, property methods, or methods: ```python +class Record: + pass + + class User: info : str @property def data(self) -> dict: - # ... + return {} - def get_record(self) -> Union[Record, None]: - # ... + def get_record(self) -> typing.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 +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. @@ -87,14 +92,13 @@ Make your names searchable. **Bad:** ```python # What the heck is 86400 for? -time.sleep(86400); +time.sleep(86400) ``` **Good**: ```python # 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)** @@ -102,35 +106,45 @@ time.sleep(SECONDS_IN_A_DAY) ### Use explanatory variables **Bad:** ```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) -save_city_zip_code(matches[1], matches[2]) +print(f"{matches[1]}: {matches[2]}") ``` **Not bad**: -It's better, but we are still heavily dependent on regex. +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) + +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})?$' +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) -save_city_zip_code(matches['city'], matches['zip_code']) +print(f"{matches['city']}, {matches['zip_code']}") ``` **[⬆ back to top](#table-of-contents)** @@ -140,32 +154,32 @@ 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)** -### Don't add unneeded context +### Don"t add unneeded context -If your class/object name tells you something, don't repeat that in your +If your class/object name tells you something, don"t repeat that in your variable name. **Bad:** @@ -221,13 +235,13 @@ 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 +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. **Bad:** ```python def create_menu(title, body, button_text, cancellable): - # ... + pass ``` **Good**: @@ -355,13 +369,20 @@ create_menu( 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 +cleaner. If you take nothing else away from this guide other than this, you"ll be ahead of many developers. **Bad:** ```python +class Client: + active: bool + + +def email(client: Client) -> None: + pass + -def email_clients(clients: List[Client]): +def email_clients(clients: typing.List[Client]) -> None: """Filter active clients and send them an email. """ for client in clients: @@ -371,13 +392,20 @@ def email_clients(clients: List[Client]): **Good**: ```python -def get_active_clients(clients: List[Client]) -> List[Client]: +class Client: + active: bool + + +def email(client: Client) -> None: + pass + +def get_active_clients(clients: typing.List[Client]) -> typing.List[Client]: """Filter active clients. """ return [client for client in clients if client.active] -def email_clients(clients: List[Client, ...]) -> None: +def email_clients(clients: typing.List[Client]) -> None: """Send an email to a given list of clients. """ for client in clients: @@ -388,16 +416,26 @@ Do you see an opportunity for using generators now? **Even better** ```python -def active_clients(clients: List[Client]) -> Generator[Client]: +class Client: + active: bool + + +def email(client: Client): + pass + + +def active_clients( + clients: typing.List[Client] + ) -> typing.Generator[Client, None, None]: """Only active clients. """ return (client for client in clients if client.active) -def email_client(clients: Iterator[Client]) -> None: +def email_client(clients: typing.Iterator[Client]) -> None: """Send an email to a given list of clients. """ - for client in clients: + for client in active_clients(clients): email(client) ``` @@ -411,7 +449,7 @@ 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? @@ -423,8 +461,7 @@ message.handle() ```python class Email: def send(self) -> None: - """Send this message. - """ + """Send this message""" message = Email() message.send() @@ -445,24 +482,23 @@ def parse_better_js_alternative(code: str) -> None: # ... ] - statements = regexes.split() + statements = code.split('\n') tokens = [] for regex in regexes: for statement in statements: - # ... + pass ast = [] for token in tokens: - # Lex. + pass for node in ast: - # Parse. + pass ``` **Good:** ```python - REGEXES = ( # ... ) @@ -473,7 +509,7 @@ def parse_better_js_alternative(code: str) -> None: syntax_tree = parse(tokens) for node in syntax_tree: - # Parse. + pass def tokenize(code: str) -> list: @@ -481,7 +517,7 @@ def tokenize(code: str) -> list: tokens = [] for regex in REGEXES: for statement in statements: - # Append the statement to tokens. + pass return tokens @@ -489,14 +525,14 @@ def tokenize(code: str) -> list: def parse(tokens: list) -> list: syntax_tree = [] for token in tokens: - # Append the parsed token to the syntax tree. + pass return syntax_tree ``` **[⬆ back to top](#table-of-contents)** -### Don't use flags as function parameters +### 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 @@ -505,11 +541,13 @@ paths based on a boolean. **Bad:** ```python +from tempfile import gettempdir from pathlib import Path + def create_file(name: str, temp: bool) -> None: if temp: - Path('./temp/' + name).touch() + (Path(gettempdir()) / name).touch() else: Path(name).touch() ``` @@ -517,13 +555,14 @@ def create_file(name: str, temp: bool) -> None: **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('./temp/' + name).touch() + (Path(gettempdir()) / name).touch() ``` **[⬆ back to top](#table-of-contents)** @@ -537,7 +576,7 @@ 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 +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. @@ -550,9 +589,9 @@ If you can do this, you will be happier than the vast majority of other programm ```python # This is a module-level name. -# It's good practice to define these as immutable values, such as a string. +# It"s good practice to define these as immutable values, such as a string. # However... -name = 'Ryan McDermott' +name = "Ryan McDermott" def split_into_first_and_last_name() -> None: # The use of the global keyword here is changing the meaning of the @@ -563,7 +602,7 @@ def split_into_first_and_last_name() -> None: split_into_first_and_last_name() -print(name) # ['Ryan', 'McDermott'] +print(name) # ["Ryan", "McDermott"] # OK. It worked the first time, but what will happen if we call the # function again? @@ -574,11 +613,11 @@ print(name) # ['Ryan', 'McDermott'] def split_into_first_and_last_name(name: str) -> list: return name.split() -name = 'Ryan McDermott' +name = "Ryan McDermott" new_name = split_into_first_and_last_name(name) -print(name) # 'Ryan McDermott' -print(new_name) # ['Ryan', 'McDermott'] +print(name) # "Ryan McDermott" +print(new_name) # ["Ryan", "McDermott"] ``` **Also good** @@ -594,9 +633,9 @@ class Person: 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'] +person = Person("Ryan McDermott") +print(person.name) # "Ryan McDermott" +print(person.name_as_first_and_last) # ["Ryan", "McDermott"] ``` **[⬆ back to top](#table-of-contents)** @@ -619,7 +658,7 @@ print(person.name_as_first_and_last) # ['Ryan', 'McDermott'] **[⬆ back to top](#table-of-contents)** -## **Don't repeat yourself (DRY)** +## **Don"t repeat yourself (DRY)** *Coming soon* diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..14d6c69 --- /dev/null +++ b/conftest.py @@ -0,0 +1,61 @@ +# content of conftest.py +import datetime +import re +import typing + +import pytest + +code_rxp = re.compile('```python(.*?)```', re.DOTALL | re.MULTILINE) + + +class FakeTimeModule: + def sleep(self, seconds): + pass + + +def fake_print(*args, **kwargs): + pass + + +def pytest_collect_file(parent, path): + if path.basename == "README.md": + return ReadmeFile.from_parent(parent, fspath=path) + + +class ReadmeFile(pytest.File): + def collect(self): + raw = self.fspath.open().read() + for idx, code in enumerate(code_rxp.findall(raw), 1): + yield ReadmeItem.from_parent( + self, name=str(idx), spec=code.strip() + ) + + +class ReadmeItem(pytest.Item): + def __init__(self, name, parent, spec): + super().__init__(name, parent) + self.spec = spec + + def runtest(self): + builtins = { + 'typing': typing, + 'time': FakeTimeModule(), + 'datetime': datetime, + 'print': fake_print + } + byte_code = compile(self.spec, '', 'exec') + exec(byte_code, builtins) + + 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): + return self.fspath, 0, "usecase: {}".format(self.name) + + +class ReadmeException(Exception): + """ custom exception for error reporting. """ diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..4708a22 --- /dev/null +++ b/requirements.txt @@ -0,0 +1,2 @@ +pytest +RestrictedPython From 0f30134e8ed4e2110690f4f006e2944fd6c9e29e Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:22:34 +0200 Subject: [PATCH 076/128] Integrate with Travis --- .travis.yml | 9 +++++++++ Makefile | 4 +++- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..f52814d --- /dev/null +++ b/.travis.yml @@ -0,0 +1,9 @@ +language: python +python: + - "3.7" +# command to install dependencies +install: + - make deps +# command to run tests +script: + - make tests diff --git a/Makefile b/Makefile index 7947ecf..a880744 100644 --- a/Makefile +++ b/Makefile @@ -17,8 +17,10 @@ ${ENV}: ${SITE_PACKAGES}/pytest.py: @${IN_ENV} pip install pytest +deps: ${SITE_PACKAGES}/pytest.py + tests: ${ENV} ${SITE_PACKAGES}/pytest.py @${IN_ENV} pytest clean: - @rm -rf ${ENV} .env dist .pytest_cache + @rm -rf ${ENV} .env .pytest_cache From c8a957383e0b0fd08dfeff7e62903f38116af916 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:30:42 +0200 Subject: [PATCH 077/128] Fix Travis dependency and clean up test module --- .travis.yml | 2 +- conftest.py | 4 ---- requirements.txt | 1 - 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index f52814d..e074e4b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ language: python python: - - "3.7" + - "3.7.5" # command to install dependencies install: - make deps diff --git a/conftest.py b/conftest.py index 14d6c69..dbad9d0 100644 --- a/conftest.py +++ b/conftest.py @@ -55,7 +55,3 @@ def repr_failure(self, excinfo, **kwargs): def reportinfo(self): return self.fspath, 0, "usecase: {}".format(self.name) - - -class ReadmeException(Exception): - """ custom exception for error reporting. """ diff --git a/requirements.txt b/requirements.txt index 4708a22..e079f8a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,2 +1 @@ pytest -RestrictedPython From edb5166700899e46a777d47371a67e9b0d397e01 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:33:22 +0200 Subject: [PATCH 078/128] Fix Travis build glitch affecting pytest --- Makefile | 2 +- requirements.txt | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 100644 requirements.txt diff --git a/Makefile b/Makefile index a880744..99eaa4d 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ ${ENV}: @${IN_ENV} pip install -U pip ${SITE_PACKAGES}/pytest.py: - @${IN_ENV} pip install pytest + @${IN_ENV} pip install -U pytest deps: ${SITE_PACKAGES}/pytest.py diff --git a/requirements.txt b/requirements.txt deleted file mode 100644 index e079f8a..0000000 --- a/requirements.txt +++ /dev/null @@ -1 +0,0 @@ -pytest From 5a6b37f92d1c37ee292d6356e5fef3118b73121e Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:36:18 +0200 Subject: [PATCH 079/128] Add Python version and build status badges --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 2449120..4103b22 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,7 @@ # clean-code-python +[![Build Status](https://travis-ci.com/zedr/clean-code-python.svg?branch=master)](https://travis-ci.com/zedr/clean-code-python) + ## Table of Contents 1. [Introduction](#introduction) 2. [Variables](#variables) From f825319ae9e359bd467427f9e3b3ff9e69167d1f Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:36:23 +0200 Subject: [PATCH 080/128] Add Python version and build status badges --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 4103b22..03b9fa8 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,7 @@ # 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.7+-blue.svg)](https://www.python.org/download/releases/3.7.5/) ## Table of Contents 1. [Introduction](#introduction) From 073a08d0b7c4625a7e06180ccb0420ef6acb4e31 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:43:00 +0200 Subject: [PATCH 081/128] #6 - Point out that passing untyped dicts is very Java-like --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 03b9fa8..6c997e7 100644 --- a/README.md +++ b/README.md @@ -247,7 +247,7 @@ def create_menu(title, body, button_text, cancellable): pass ``` -**Good**: +**Java-esque**: ```python class Menu: def __init__(self, config: dict): From d08459e5189944f24eb9d5e26da48ddc07f08631 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 00:57:53 +0200 Subject: [PATCH 082/128] Migrate to Python 3.8 and suggest using TypeDict (#6) --- .travis.yml | 2 +- Makefile | 2 +- README.md | 32 +++++++++++++++++++++++++++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index e074e4b..4e198a2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ language: python python: - - "3.7.5" + - "3.8.3" # command to install dependencies install: - make deps diff --git a/Makefile b/Makefile index 99eaa4d..3f71a6b 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ .PHONY: deps clean tests ENV=.env -PYTHON=python3.7 +PYTHON=python3.8 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; diff --git a/README.md b/README.md index 6c997e7..44bdcd5 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # 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.7+-blue.svg)](https://www.python.org/download/releases/3.7.5/) +[![](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) @@ -366,6 +366,36 @@ create_menu( ) ``` +**Even fancier, Python3.8+ only** +```python +class MenuConfig(typing.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 = False + + +def create_menu(config: MenuConfig): + title = config["title"] + # ... + + +create_menu( + { + 'title' : "My delicious menu", + 'body' : "A description of the various items on the menu", + 'button_text' : "Order now!" + } +) +``` **[⬆ back to top](#table-of-contents)** ### Functions should do one thing From 576fa275bd44e49da6af0219735c6f0bdf307821 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 01:01:25 +0200 Subject: [PATCH 083/128] Add sitnarf as a contributor --- CONTRIBUTORS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 39e7806..4c0a249 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -3,4 +3,5 @@ Zachary Anglin AirbusDriver Micheal Erik OShaughnessy -Mukhammad Karimov \ No newline at end of file +Mukhammad Karimov +sitnarf From 9796f5cbf71aaae04478adeee74ab753c4145748 Mon Sep 17 00:00:00 2001 From: Rigel Date: Sun, 24 May 2020 22:26:49 +0200 Subject: [PATCH 084/128] Refactor README and introduce MyPy static type analysis --- Makefile | 2 +- README.md | 178 +++++++++++++++++++++++++++++++---------------- conftest.py | 56 +++++++++++---- requirements.txt | 2 + 4 files changed, 163 insertions(+), 75 deletions(-) create mode 100644 requirements.txt diff --git a/Makefile b/Makefile index 3f71a6b..0b85c59 100644 --- a/Makefile +++ b/Makefile @@ -15,7 +15,7 @@ ${ENV}: @${IN_ENV} pip install -U pip ${SITE_PACKAGES}/pytest.py: - @${IN_ENV} pip install -U pytest + @${IN_ENV} pip install -r requirements.txt deps: ${SITE_PACKAGES}/pytest.py diff --git a/README.md b/README.md index 44bdcd5..c6224f4 100644 --- a/README.md +++ b/README.md @@ -36,11 +36,17 @@ Targets Python3.7+ **Bad:** ```python +import datetime + + ymdstr = datetime.date.today().strftime("%y-%m-%d") ``` **Good**: ```python +import datetime + + current_date: str = datetime.date.today().strftime("%y-%m-%d") ``` **[⬆ back to top](#table-of-contents)** @@ -68,6 +74,9 @@ Python is (also) an object oriented programming language. If it makes sense, pac of the entity in your code, as instance attributes, property methods, or methods: ```python +from typing import Union, Dict, Text + + class Record: pass @@ -76,10 +85,10 @@ class User: info : str @property - def data(self) -> dict: + def data(self) -> Dict[Text, Text]: return {} - def get_record(self) -> typing.Union[Record, None]: + def get_record(self) -> Union[Record, None]: return Record() ``` @@ -94,12 +103,18 @@ Make your names searchable. **Bad:** ```python -# What the heck is 86400 for? +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) @@ -114,9 +129,10 @@ 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) -print(f"{matches[1]}: {matches[2]}") +matches = re.match(city_zip_code_regex, address) +if matches: + print(f"{matches[1]}: {matches[2]}") ``` **Not bad**: @@ -131,9 +147,9 @@ 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() - -print(f"{city}: {zip_code}") +if matches: + city, zip_code = matches.groups() + print(f"{city}: {zip_code}") ``` **Good**: @@ -145,9 +161,10 @@ 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) -print(f"{matches['city']}, {matches['zip_code']}") +matches = re.match(city_zip_code_regex, address) +if matches: + print(f"{matches['city']}, {matches['zip_code']}") ``` **[⬆ back to top](#table-of-contents)** @@ -212,6 +229,9 @@ 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() @@ -224,7 +244,11 @@ you are expecting a string as the argument. **Good**: ```python -def create_micro_brewery(name: str = "Hipster Brew Co."): +from typing import Text +import hashlib + + +def create_micro_brewery(name: Text = "Hipster Brew Co."): slug = hashlib.sha1(name.encode()).hexdigest() # etc. ``` @@ -267,6 +291,9 @@ menu = Menu( **Also good** ```python +from typing import Text + + class MenuConfig: """A configuration for the Menu. @@ -276,13 +303,13 @@ class MenuConfig: button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: str - body: str - button_text: str + title: Text + body: Text + button_text: Text cancellable: bool = False -def create_menu(config: MenuConfig): +def create_menu(config: MenuConfig) -> None: title = config.title body = config.body # ... @@ -334,6 +361,7 @@ create_menu( **Even fancier** ```python +from typing import Text from dataclasses import astuple, dataclass @@ -347,9 +375,9 @@ class MenuConfig: button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: str - body: str - button_text: str + title: Text + body: Text + button_text: Text cancellable: bool = False def create_menu(config: MenuConfig): @@ -368,7 +396,10 @@ create_menu( **Even fancier, Python3.8+ only** ```python -class MenuConfig(typing.TypedDict): +from typing import TypedDict, Text + + +class MenuConfig(TypedDict): """A configuration for the Menu. Attributes: @@ -377,10 +408,10 @@ class MenuConfig(typing.TypedDict): button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: str - body: str - button_text: str - cancellable: bool = False + title: Text + body: Text + button_text: Text + cancellable: bool def create_menu(config: MenuConfig): @@ -389,11 +420,13 @@ def create_menu(config: MenuConfig): create_menu( - { - 'title' : "My delicious menu", - 'body' : "A description of the various items on the menu", - 'button_text' : "Order now!" - } + # 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)** @@ -407,6 +440,9 @@ of many developers. **Bad:** ```python +from typing import List + + class Client: active: bool @@ -415,7 +451,7 @@ def email(client: Client) -> None: pass -def email_clients(clients: typing.List[Client]) -> None: +def email_clients(clients: List[Client]) -> None: """Filter active clients and send them an email. """ for client in clients: @@ -425,6 +461,9 @@ def email_clients(clients: typing.List[Client]) -> None: **Good**: ```python +from typing import List + + class Client: active: bool @@ -432,13 +471,14 @@ class Client: def email(client: Client) -> None: pass -def get_active_clients(clients: typing.List[Client]) -> typing.List[Client]: + +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: typing.List[Client]) -> None: +def email_clients(clients: List[Client]) -> None: """Send an email to a given list of clients. """ for client in clients: @@ -449,6 +489,9 @@ Do you see an opportunity for using generators now? **Even better** ```python +from typing import Generator, Iterator + + class Client: active: bool @@ -457,15 +500,12 @@ def email(client: Client): pass -def active_clients( - clients: typing.List[Client] - ) -> typing.Generator[Client, None, None]: - """Only active clients. - """ +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: typing.Iterator[Client]) -> None: +def email_client(clients: Iterator[Client]) -> None: """Send an email to a given list of clients. """ for client in active_clients(clients): @@ -510,6 +550,8 @@ much. Splitting up functions leads to reusability and easier testing. **Bad:** ```python +# type: ignore + def parse_better_js_alternative(code: str) -> None: regexes = [ # ... @@ -532,22 +574,25 @@ def parse_better_js_alternative(code: str) -> None: **Good:** ```python -REGEXES = ( +from typing import Tuple, List, Text, Dict + + +REGEXES: Tuple = ( # ... ) -def parse_better_js_alternative(code: str) -> None: - tokens = tokenize(code) - syntax_tree = parse(tokens) +def parse_better_js_alternative(code: Text) -> None: + tokens: List = tokenize(code) + syntax_tree: List = parse(tokens) for node in syntax_tree: pass -def tokenize(code: str) -> list: +def tokenize(code: Text) -> List: statements = code.split() - tokens = [] + tokens: List[Dict] = [] for regex in REGEXES: for statement in statements: pass @@ -555,8 +600,8 @@ def tokenize(code: str) -> list: return tokens -def parse(tokens: list) -> list: - syntax_tree = [] +def parse(tokens: List) -> List: + syntax_tree: List[Dict] = [] for token in tokens: pass @@ -574,11 +619,12 @@ paths based on a boolean. **Bad:** ```python +from typing import Text from tempfile import gettempdir from pathlib import Path -def create_file(name: str, temp: bool) -> None: +def create_file(name: Text, temp: bool) -> None: if temp: (Path(gettempdir()) / name).touch() else: @@ -588,13 +634,16 @@ def create_file(name: str, temp: bool) -> None: **Good:** ```python +from typing import Text from tempfile import gettempdir from pathlib import Path -def create_file(name: str) -> None: + +def create_file(name: Text) -> None: Path(name).touch() -def create_temp_file(name: str) -> None: + +def create_temp_file(name: Text) -> None: (Path(gettempdir()) / name).touch() ``` @@ -621,21 +670,25 @@ If you can do this, you will be happier than the vast majority of other programm **Bad:** ```python +# type: ignore + # This is a module-level name. # It"s good practice to define these as immutable values, such as a string. # However... -name = "Ryan McDermott" +fullname = "Ryan McDermott" 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 name - name = name.split() + global fullname + fullname = fullname.split() split_into_first_and_last_name() -print(name) # ["Ryan", "McDermott"] +# 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? @@ -643,32 +696,37 @@ print(name) # ["Ryan", "McDermott"] **Good:** ```python -def split_into_first_and_last_name(name: str) -> list: +from typing import List, AnyStr + + +def split_into_first_and_last_name(name: AnyStr) -> List[AnyStr]: return name.split() -name = "Ryan McDermott" -new_name = split_into_first_and_last_name(name) +fullname = "Ryan McDermott" +name, surname = split_into_first_and_last_name(fullname) -print(name) # "Ryan McDermott" -print(new_name) # ["Ryan", "McDermott"] +print(name, surname) # => Ryan McDermott ``` **Also good** ```python +from typing import Text from dataclasses import dataclass + @dataclass class Person: - name: str + name: Text @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"] +print(person.name) # => "Ryan McDermott" +print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] ``` **[⬆ back to top](#table-of-contents)** diff --git a/conftest.py b/conftest.py index dbad9d0..2b5e63f 100644 --- a/conftest.py +++ b/conftest.py @@ -1,50 +1,77 @@ -# content of conftest.py -import datetime +import importlib import re +import time import typing import pytest +from mypy import api code_rxp = re.compile('```python(.*?)```', re.DOTALL | re.MULTILINE) -class FakeTimeModule: - def sleep(self, seconds): - pass +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, fspath=path) class ReadmeFile(pytest.File): + """A Markdown formatted readme file containing code snippets""" + def collect(self): - raw = self.fspath.open().read() - for idx, code in enumerate(code_rxp.findall(raw), 1): + """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): - builtins = { - 'typing': typing, - 'time': FakeTimeModule(), - 'datetime': datetime, - 'print': fake_print - } + """Run the test""" + builtins = dict(self.builtins) byte_code = compile(self.spec, '', 'exec') - exec(byte_code, builtins) + _with_patched_sleep(exec, byte_code, builtins) + msg, _, error = api.run(['--no-color-output', '-c', self.spec]) + if error: + # 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. """ @@ -54,4 +81,5 @@ def repr_failure(self, excinfo, **kwargs): ) 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 From f5034cb4a6083e10a7dad5e99fde513a71e50fa9 Mon Sep 17 00:00:00 2001 From: Miguel Gonzalez Date: Wed, 19 Feb 2020 18:47:00 +0100 Subject: [PATCH 085/128] Fix instance variable initialization --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index c6224f4..9ae7989 100644 --- a/README.md +++ b/README.md @@ -275,8 +275,8 @@ def create_menu(title, body, button_text, cancellable): ```python class Menu: def __init__(self, config: dict): - title = config["title"] - body = config["body"] + self.title = config["title"] + self.body = config["body"] # ... menu = Menu( From 9311f654f7365b863284ddf6f52cd30b0ab8eb77 Mon Sep 17 00:00:00 2001 From: Rigel Date: Mon, 25 May 2020 19:25:34 +0200 Subject: [PATCH 086/128] Add migonzalvar as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 4c0a249..2105033 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -5,3 +5,4 @@ Micheal Erik OShaughnessy Mukhammad Karimov sitnarf +Miguel Gonzalez From ffb171a00b55b31f4a722d39fafc53fc5909afed Mon Sep 17 00:00:00 2001 From: Anvar Date: Tue, 9 Jun 2020 06:21:02 +0600 Subject: [PATCH 087/128] Use the missing function --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9ae7989..53e196d 100644 --- a/README.md +++ b/README.md @@ -481,7 +481,7 @@ def get_active_clients(clients: List[Client]) -> List[Client]: def email_clients(clients: List[Client]) -> None: """Send an email to a given list of clients. """ - for client in clients: + for client in get_active_clients(clients): email(client) ``` From 91910678ef4c51d75f7d9c78e52c5a51ee242a64 Mon Sep 17 00:00:00 2001 From: Rigel Date: Tue, 9 Jun 2020 08:33:45 +0200 Subject: [PATCH 088/128] Add Anvar as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 2105033..e322fe4 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -6,3 +6,4 @@ Erik OShaughnessy Mukhammad Karimov sitnarf Miguel Gonzalez +Anvar From bba92dab168eb2521006ff26a9b3d24beb817e37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Pavl=C3=A1sek?= Date: Mon, 22 Mar 2021 14:35:07 +0100 Subject: [PATCH 089/128] replace " by ' in words like don"t, remove trailing spaces --- README.md | 63 +++++++++++++++++++++++++++---------------------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 53e196d..e4d2801 100644 --- a/README.md +++ b/README.md @@ -14,17 +14,17 @@ 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) + 6. [Don't repeat yourself (DRY)](#dont-repeat-yourself-dry) ## Introduction -Software engineering principles, from Robert C. Martin"s book +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 +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 +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) @@ -90,14 +90,13 @@ class User: 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 +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. @@ -137,7 +136,7 @@ if matches: **Not bad**: -It"s better, but we are still heavily dependent on regex. +It's better, but we are still heavily dependent on regex. ```python import re @@ -197,9 +196,9 @@ for location in locations: **[⬆ back to top](#table-of-contents)** -### Don"t add unneeded context +### Don't add unneeded context -If your class/object name tells you something, don"t repeat that in your +If your class/object name tells you something, don't repeat that in your variable name. **Bad:** @@ -256,13 +255,13 @@ def create_micro_brewery(name: Text = "Hipster Brew Co."): **[⬆ 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 +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 +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. **Bad:** @@ -422,7 +421,7 @@ def create_menu(config: MenuConfig): create_menu( # You need to supply all the parameters MenuConfig( - title="My delicious menu", + title="My delicious menu", body="A description of the various items on the menu", button_text="Order now!", cancellable=True @@ -432,10 +431,10 @@ 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 +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:** @@ -544,7 +543,7 @@ 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 +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:** @@ -610,10 +609,10 @@ def parse(tokens: List) -> List: **[⬆ back to top](#table-of-contents)** -### Don"t use flags as function parameters +### 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 +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:** @@ -651,14 +650,14 @@ def create_temp_file(name: Text) -> None: ### 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 +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 +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. @@ -673,7 +672,7 @@ If you can do this, you will be happier than the vast majority of other programm # type: ignore # This is a module-level name. -# It"s good practice to define these as immutable values, such as a string. +# It's good practice to define these as immutable values, such as a string. # However... fullname = "Ryan McDermott" @@ -686,7 +685,7 @@ def split_into_first_and_last_name() -> None: split_into_first_and_last_name() -# MyPy will spot the problem, complaining about 'Incompatible types in +# MyPy will spot the problem, complaining about 'Incompatible types in # assignment: (expression has type "List[str]", variable has type "str")' print(fullname) # ["Ryan", "McDermott"] @@ -720,7 +719,7 @@ class Person: @property def name_as_first_and_last(self) -> list: - return self.name.split() + return self.name.split() # The reason why we create instances of classes is to manage state! @@ -749,7 +748,7 @@ print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] **[⬆ back to top](#table-of-contents)** -## **Don"t repeat yourself (DRY)** +## **Don't repeat yourself (DRY)** *Coming soon* From 3aa1a8b9d82a097fef88d0268fd357a10103b54f Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Mon, 22 Mar 2021 21:12:03 +0100 Subject: [PATCH 090/128] Add mpavlase as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index e322fe4..a234aab 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -7,3 +7,4 @@ Mukhammad Karimov sitnarf Miguel Gonzalez Anvar +Martin Pavlásek From 1b4abfc002ac7e70c83cc780663c14e25b03b123 Mon Sep 17 00:00:00 2001 From: Shahrukh Khan Date: Mon, 17 May 2021 19:56:48 +0200 Subject: [PATCH 091/128] add DRY example --- README.md | 138 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e4d2801..9175bbf 100644 --- a/README.md +++ b/README.md @@ -750,7 +750,143 @@ print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] ## **Don't repeat yourself (DRY)** -*Coming soon* +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. + +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! + +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:** + +```python +from typing import List, Text, Dict +from dataclasses import dataclass + +@dataclass +class Developer: + def __init__(self, experience: float, github_link: Text) -> None: + self._experience = experience + self._github_link = github_link + + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> Text: + return self._github_link + +@dataclass +class Manager: + def __init__(self, experience: float, github_link: Text) -> None: + self._experience = experience + self._github_link = github_link + + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> Text: + 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:** + +```python +from typing import List, Text, Dict +from dataclasses import dataclass + +@dataclass +class Employee: + def __init__(self, experience: float, github_link: Text) -> None: + self._experience = experience + self._github_link = github_link + + @property + def experience(self) -> float: + return self._experience + + @property + def github_link(self) -> Text: + 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)** From 74460c2d7d40f079667bd113f42dbcb0d4c67f5a Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sun, 30 May 2021 20:53:41 +0200 Subject: [PATCH 092/128] Update CONTRIBUTORS Add shahrukhx01 as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index a234aab..a92b1a1 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -8,3 +8,4 @@ sitnarf Miguel Gonzalez Anvar Martin Pavlásek +Shahrukh Khan From 47214e2fececbff9cddd1c8f8b19fa768c14319b Mon Sep 17 00:00:00 2001 From: Aaron Law Date: Sun, 25 Jul 2021 19:13:01 +0800 Subject: [PATCH 093/128] Add a blank link before code block --- README.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/README.md b/README.md index 9175bbf..c9939db 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,7 @@ Targets Python3.7+ ### Use meaningful and pronounceable variable names **Bad:** + ```python import datetime @@ -43,6 +44,7 @@ ymdstr = datetime.date.today().strftime("%y-%m-%d") ``` **Good**: + ```python import datetime @@ -55,6 +57,7 @@ current_date: str = datetime.date.today().strftime("%y-%m-%d") **Bad:** Here we use three different names for the same underlying entity: + ```python def get_user_info(): pass def get_client_data(): pass @@ -63,6 +66,7 @@ def get_customer_record(): pass **Good**: 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 @@ -101,6 +105,7 @@ understanding our program, we hurt our readers. Make your names searchable. **Bad:** + ```python import time @@ -110,6 +115,7 @@ time.sleep(86400) ``` **Good**: + ```python import time @@ -122,6 +128,7 @@ time.sleep(SECONDS_IN_A_DAY) ### Use explanatory variables **Bad:** + ```python import re @@ -154,6 +161,7 @@ if matches: **Good**: Decrease dependence on regex by naming subpatterns. + ```python import re @@ -172,6 +180,7 @@ 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") @@ -184,6 +193,7 @@ for item in seq: ``` **Good**: + ```python locations = ("Austin", "New York", "San Francisco") @@ -265,12 +275,14 @@ arguments then your function is trying to do too much. In cases where it's not, of the time a higher-level object will suffice as an argument. **Bad:** + ```python def create_menu(title, body, button_text, cancellable): pass ``` **Java-esque**: + ```python class Menu: def __init__(self, config: dict): @@ -289,6 +301,7 @@ menu = Menu( ``` **Also good** + ```python from typing import Text @@ -325,6 +338,7 @@ create_menu(config) ``` **Fancy** + ```python from typing import NamedTuple @@ -359,6 +373,7 @@ create_menu( ``` **Even fancier** + ```python from typing import Text from dataclasses import astuple, dataclass @@ -394,6 +409,7 @@ create_menu( ``` **Even fancier, Python3.8+ only** + ```python from typing import TypedDict, Text @@ -438,6 +454,7 @@ cleaner. If you take nothing else away from this guide other than this, you'll b of many developers. **Bad:** + ```python from typing import List @@ -459,6 +476,7 @@ def email_clients(clients: List[Client]) -> None: ``` **Good**: + ```python from typing import List @@ -487,6 +505,7 @@ def email_clients(clients: List[Client]) -> None: Do you see an opportunity for using generators now? **Even better** + ```python from typing import Generator, Iterator @@ -694,6 +713,7 @@ print(fullname) # ["Ryan", "McDermott"] ``` **Good:** + ```python from typing import List, AnyStr @@ -708,6 +728,7 @@ print(name, surname) # => Ryan McDermott ``` **Also good** + ```python from typing import Text from dataclasses import dataclass From dc8fe21655d13fe0e0b2b2836fc00e881b4d6d6d Mon Sep 17 00:00:00 2001 From: Fredson Chaves Date: Tue, 17 Aug 2021 09:13:46 -0300 Subject: [PATCH 094/128] feat: link to translation for brazilian portuguese --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index 9175bbf..fd49d22 100644 --- a/README.md +++ b/README.md @@ -890,3 +890,8 @@ company_managers_list = get_employee_list(employees=company_managers) **[⬆ back to top](#table-of-contents)** +## **Translation** + +This is also available in other languages: + +- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) \ No newline at end of file From 3f5f2a94c2e0f63d59d50cdfe6a19de75cf9d7a5 Mon Sep 17 00:00:00 2001 From: Fredson Chaves <43495376+fredsonchaves07@users.noreply.github.com> Date: Tue, 17 Aug 2021 09:15:41 -0300 Subject: [PATCH 095/128] feat: link translation to table of contents --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index fd49d22..61ef3f0 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ 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. [Translation](#translation) ## Introduction @@ -894,4 +895,6 @@ company_managers_list = get_employee_list(employees=company_managers) This is also available in other languages: -- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) \ No newline at end of file +- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) + +**[⬆ back to top](#table-of-contents)** From 02e52ddcdf329f49af9837f58eeb91f9addf5447 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 18 Aug 2021 21:14:49 +0200 Subject: [PATCH 096/128] Add AaronLaw as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index a92b1a1..4a2e21d 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -9,3 +9,4 @@ Miguel Gonzalez Anvar Martin Pavlásek Shahrukh Khan +Aaron Law From 853f42e0896aa77165e2f3007ece2b5bde75cbee Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 18 Aug 2021 21:15:25 +0200 Subject: [PATCH 097/128] Use the latest Python3 available on the system --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0b85c59..eb8f7eb 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ .PHONY: deps clean tests ENV=.env -PYTHON=python3.8 +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; From c51cdcf4be3de610fd8f5ff001f6094cdefffc74 Mon Sep 17 00:00:00 2001 From: Fredson Chaves Date: Tue, 17 Aug 2021 09:13:46 -0300 Subject: [PATCH 098/128] feat: link to translation for brazilian portuguese --- README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/README.md b/README.md index c9939db..e0a29d5 100644 --- a/README.md +++ b/README.md @@ -911,3 +911,8 @@ company_managers_list = get_employee_list(employees=company_managers) **[⬆ back to top](#table-of-contents)** +## **Translation** + +This is also available in other languages: + +- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) \ No newline at end of file From 925db83b45d2799daf5a920e25111d57c5c63e69 Mon Sep 17 00:00:00 2001 From: Fredson Chaves <43495376+fredsonchaves07@users.noreply.github.com> Date: Tue, 17 Aug 2021 09:15:41 -0300 Subject: [PATCH 099/128] feat: link translation to table of contents --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e0a29d5..82119d4 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,7 @@ 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. [Translation](#translation) ## Introduction @@ -915,4 +916,6 @@ company_managers_list = get_employee_list(employees=company_managers) This is also available in other languages: -- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) \ No newline at end of file +- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) + +**[⬆ back to top](#table-of-contents)** From cf4e0ca2c8f78e262b120e898835d52d64ebb4f7 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 18 Aug 2021 21:19:31 +0200 Subject: [PATCH 100/128] Minor stylistic edits regarding the Translations --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 82119d4..467b695 100644 --- a/README.md +++ b/README.md @@ -912,9 +912,9 @@ company_managers_list = get_employee_list(employees=company_managers) **[⬆ back to top](#table-of-contents)** -## **Translation** +## **Translations** -This is also available in other languages: +This document is also available in other languages: - ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) From e16c09e66e8d0b4efada44c6746f0a3dbca7436a Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Wed, 18 Aug 2021 21:32:34 +0200 Subject: [PATCH 101/128] Add Fredson Chaves as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 4a2e21d..ef3a134 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -10,3 +10,4 @@ Anvar Martin Pavlásek Shahrukh Khan Aaron Law +Fredson Chaves From a4bb69f1e05c08c8c68ecb4b938b05fde01298a8 Mon Sep 17 00:00:00 2001 From: Martin Thoma Date: Sat, 12 Feb 2022 09:02:34 +0100 Subject: [PATCH 102/128] Replace typing.Text by str typing.Text was introduced as a compatibility-layer for Python 2: https://docs.python.org/3/library/typing.html#typing.Text Now that Python 2 has long reached its End-of-Life, we can remove unnecessary imports --- README.md | 64 ++++++++++++++++++++++++------------------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 467b695..e38ab24 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,7 @@ Python is (also) an object oriented programming language. If it makes sense, pac of the entity in your code, as instance attributes, property methods, or methods: ```python -from typing import Union, Dict, Text +from typing import Union, Dict class Record: @@ -90,7 +90,7 @@ class User: info : str @property - def data(self) -> Dict[Text, Text]: + def data(self) -> Dict[str, str]: return {} def get_record(self) -> Union[Record, None]: @@ -254,11 +254,10 @@ you are expecting a string as the argument. **Good**: ```python -from typing import Text import hashlib -def create_micro_brewery(name: Text = "Hipster Brew Co."): +def create_micro_brewery(name: str = "Hipster Brew Co."): slug = hashlib.sha1(name.encode()).hexdigest() # etc. ``` @@ -304,9 +303,6 @@ menu = Menu( **Also good** ```python -from typing import Text - - class MenuConfig: """A configuration for the Menu. @@ -316,9 +312,9 @@ class MenuConfig: button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: Text - body: Text - button_text: Text + title: str + body: str + button_text: str cancellable: bool = False @@ -376,7 +372,6 @@ create_menu( **Even fancier** ```python -from typing import Text from dataclasses import astuple, dataclass @@ -390,9 +385,9 @@ class MenuConfig: button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: Text - body: Text - button_text: Text + title: str + body: str + button_text: str cancellable: bool = False def create_menu(config: MenuConfig): @@ -412,7 +407,7 @@ create_menu( **Even fancier, Python3.8+ only** ```python -from typing import TypedDict, Text +from typing import TypedDict class MenuConfig(TypedDict): @@ -424,9 +419,9 @@ class MenuConfig(TypedDict): button_text: The text for the button label. cancellable: Can it be cancelled? """ - title: Text - body: Text - button_text: Text + title: str + body: str + button_text: str cancellable: bool @@ -593,7 +588,7 @@ def parse_better_js_alternative(code: str) -> None: **Good:** ```python -from typing import Tuple, List, Text, Dict +from typing import Tuple, List, Dict REGEXES: Tuple = ( @@ -601,7 +596,7 @@ REGEXES: Tuple = ( ) -def parse_better_js_alternative(code: Text) -> None: +def parse_better_js_alternative(code: str) -> None: tokens: List = tokenize(code) syntax_tree: List = parse(tokens) @@ -609,7 +604,7 @@ def parse_better_js_alternative(code: Text) -> None: pass -def tokenize(code: Text) -> List: +def tokenize(code: str) -> List: statements = code.split() tokens: List[Dict] = [] for regex in REGEXES: @@ -638,12 +633,11 @@ paths based on a boolean. **Bad:** ```python -from typing import Text from tempfile import gettempdir from pathlib import Path -def create_file(name: Text, temp: bool) -> None: +def create_file(name: str, temp: bool) -> None: if temp: (Path(gettempdir()) / name).touch() else: @@ -653,16 +647,15 @@ def create_file(name: Text, temp: bool) -> None: **Good:** ```python -from typing import Text from tempfile import gettempdir from pathlib import Path -def create_file(name: Text) -> None: +def create_file(name: str) -> None: Path(name).touch() -def create_temp_file(name: Text) -> None: +def create_temp_file(name: str) -> None: (Path(gettempdir()) / name).touch() ``` @@ -731,13 +724,12 @@ print(name, surname) # => Ryan McDermott **Also good** ```python -from typing import Text from dataclasses import dataclass @dataclass class Person: - name: Text + name: str @property def name_as_first_and_last(self) -> list: @@ -797,12 +789,12 @@ updating multiple places any time you want to change one thing. **Bad:** ```python -from typing import List, Text, Dict +from typing import List, Dict from dataclasses import dataclass @dataclass class Developer: - def __init__(self, experience: float, github_link: Text) -> None: + def __init__(self, experience: float, github_link: str) -> None: self._experience = experience self._github_link = github_link @@ -811,12 +803,12 @@ class Developer: return self._experience @property - def github_link(self) -> Text: + def github_link(self) -> str: return self._github_link @dataclass class Manager: - def __init__(self, experience: float, github_link: Text) -> None: + def __init__(self, experience: float, github_link: str) -> None: self._experience = experience self._github_link = github_link @@ -825,7 +817,7 @@ class Manager: return self._experience @property - def github_link(self) -> Text: + def github_link(self) -> str: return self._github_link @@ -865,12 +857,12 @@ company_managers_list = get_manager_list(managers=company_managers) **Good:** ```python -from typing import List, Text, Dict +from typing import List, Dict from dataclasses import dataclass @dataclass class Employee: - def __init__(self, experience: float, github_link: Text) -> None: + def __init__(self, experience: float, github_link: str) -> None: self._experience = experience self._github_link = github_link @@ -879,7 +871,7 @@ class Employee: return self._experience @property - def github_link(self) -> Text: + def github_link(self) -> str: return self._github_link From 95d0b1e9869f62108b7b82d62f84bb9b93936ebb Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sun, 20 Feb 2022 09:04:36 +0100 Subject: [PATCH 103/128] Add github actions workflow --- .github/workflows/python-app.yml | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 .github/workflows/python-app.yml 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 From 4e442c04e5013533ac3d28f78c88668b3db8a06a Mon Sep 17 00:00:00 2001 From: zedr Date: Sun, 20 Feb 2022 09:17:23 +0100 Subject: [PATCH 104/128] Add MartinThoma as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index ef3a134..f8ff1a7 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -11,3 +11,4 @@ Martin Pavlásek Shahrukh Khan Aaron Law Fredson Chaves +MartinThoma From 98501387a2ba3b870d065f8d92b3c1c9721c680c Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 25 Jun 2022 17:40:58 +0200 Subject: [PATCH 105/128] Fix deprecation warning --- conftest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/conftest.py b/conftest.py index 2b5e63f..15f8bff 100644 --- a/conftest.py +++ b/conftest.py @@ -2,6 +2,7 @@ import re import time import typing +from pathlib import Path import pytest from mypy import api @@ -21,7 +22,7 @@ def fake_print(*args, **kwargs): def pytest_collect_file(parent, path): """Collect all file suitable for use in tests""" if path.basename == "README.md": - return ReadmeFile.from_parent(parent, fspath=path) + return ReadmeFile.from_parent(parent, path=Path(path)) class ReadmeFile(pytest.File): From d1c091a9f4f3a6f62dfe845133971657135e2d46 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 17:11:02 +0200 Subject: [PATCH 106/128] Add Single Responsibility Principle --- README.md | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/README.md b/README.md index e38ab24..31a1ef2 100644 --- a/README.md +++ b/README.md @@ -753,6 +753,78 @@ print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] ## **Classes** ### **Single Responsibility Principle (SRP)** + +Robert C. Martin writes: + +> A class should have only one reason to change. + +"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 + """ + + def get_version(self) -> str: + """Get the package version""" + return metadata.version("pip") + + def render(self) -> None: + print(f'') + + +VersionCommentElement().render() +``` +This class has two responsibilities: + + - Retrieve the version number of the Python package + - Render itself as an HTML element + +Any change to one or the other carries the risk of impacting the other. + +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() +``` + +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)** ### **Liskov Substitution Principle (LSP)** ### **Interface Segregation Principle (ISP)** From 278bfe82a4756531a3ca58ff60977e2e1972f539 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 18:34:52 +0200 Subject: [PATCH 107/128] Add Open/Closed Principle --- README.md | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/README.md b/README.md index 31a1ef2..8a15982 100644 --- a/README.md +++ b/README.md @@ -826,6 +826,125 @@ 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() + ) + +``` + +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. + +Let's redesign our classes to fix this problem and let the `View` class be +extended (not modified) cleanly: + +**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("index.html") as fd: + return fd.read() + + +``` + +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. + ### **Liskov Substitution Principle (LSP)** ### **Interface Segregation Principle (ISP)** ### **Dependency Inversion Principle (DIP)** From 8861c48df0e0b45e7f7b117b99633b4a28a8274f Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 18:39:28 +0200 Subject: [PATCH 108/128] Add a comment about types in variable names --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 8a15982..9021998 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,8 @@ 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 From bd7420dd319586f845cfa03b9315489a4a9a44d8 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 20:36:57 +0200 Subject: [PATCH 109/128] Explain Mixins --- README.md | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 9021998..828025a 100644 --- a/README.md +++ b/README.md @@ -936,7 +936,7 @@ class TemplateView(View): def render_body(self) -> str: """Render the message body as HTML""" - with open("index.html") as fd: + with open(self.template_file) as fd: return fd.read() @@ -947,6 +947,100 @@ 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. +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 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" + +``` + +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. + +The popular Django project makes heavy use of Mixins to compose its class-based +views. + +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. + ### **Liskov Substitution Principle (LSP)** ### **Interface Segregation Principle (ISP)** ### **Dependency Inversion Principle (DIP)** From 587a646ede1059afaf0c40c0990f05dcc685324c Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 21:34:29 +0200 Subject: [PATCH 110/128] Add Liskov substitution principle --- README.md | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 828025a..1197e5c 100644 --- a/README.md +++ b/README.md @@ -830,7 +830,8 @@ 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. +> 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) @@ -1042,6 +1043,93 @@ 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. ### **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) + +``` + +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`. + +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 +``` + ### **Interface Segregation Principle (ISP)** ### **Dependency Inversion Principle (DIP)** From 64ee3201b2a78dc470e7c31030aabce7013d9f6e Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 13 Aug 2022 23:46:06 +0200 Subject: [PATCH 111/128] Add Interface Segregation --- README.md | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/README.md b/README.md index 1197e5c..b24face 100644 --- a/README.md +++ b/README.md @@ -1131,6 +1131,175 @@ error: Signature of "get" incompatible with supertype "View" ``` ### **Interface Segregation Principle (ISP)** + +> “Keep interfaces small +> so that users don’t end up depending on things they don’t need.”, +> Uncle Bob. + +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". + +Python does not have interfaces. We have Abstract Base Classes instead, which +are a little different, but can serve the same purpose. + +**Good** +```python + +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()) +``` + +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. + +**Error** +```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 + +``` + +But we can't! If we don't implement the `.save()` method, an exception will be +raised: + +``` +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): + """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 + +``` + ### **Dependency Inversion Principle (DIP)** *Coming soon* From 08fe98cc30b2d49c5a19ffc7dd4264dbcaaecb7c Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sun, 14 Aug 2022 00:06:11 +0200 Subject: [PATCH 112/128] Add Dependency Inversion --- README.md | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b24face..671cc3c 100644 --- a/README.md +++ b/README.md @@ -1302,7 +1302,96 @@ def view(request): ### **Dependency Inversion Principle (DIP)** -*Coming soon* +> “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 + +``` + +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)** From 0d04d247d2a5f49b3b0d54e7dbf298aed2255d70 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sun, 14 Aug 2022 00:07:29 +0200 Subject: [PATCH 113/128] Remove Objects and Data Structures --- README.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/README.md b/README.md index 671cc3c..d15d733 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,6 @@ 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) @@ -746,12 +745,6 @@ print(person.name_as_first_and_last) # => ["Ryan", "McDermott"] **[⬆ back to top](#table-of-contents)** -## **Objects and Data Structures** - -*Coming soon* - -**[⬆ back to top](#table-of-contents)** - ## **Classes** ### **Single Responsibility Principle (SRP)** From ff1d2b1bcbadffdda4f388e9b3f9148db94dbfd6 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 26 Aug 2022 18:21:42 +0200 Subject: [PATCH 114/128] Add link to Persian translation --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d15d733..bf750ea 100644 --- a/README.md +++ b/README.md @@ -1534,6 +1534,7 @@ company_managers_list = get_employee_list(employees=company_managers) This document is also available in other languages: -- ![br](https://raw.githubusercontent.com/gosquared/flags/master/flags/flags/shiny/24/Brazil.png) **Brazilian Portuguese**: [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) +- :pt: :br: **Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) +- :iran: **Persian:** [https://github.com/SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) **[⬆ back to top](#table-of-contents)** From 64dc2bec580b510c35fa07390fddf9e5c0bfe784 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Fri, 26 Aug 2022 18:27:12 +0200 Subject: [PATCH 115/128] Add Ali Bagheri as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index f8ff1a7..da6b341 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -12,3 +12,4 @@ Shahrukh Khan Aaron Law Fredson Chaves MartinThoma +Ali Bagheri From 2f7b4841cba156b99b21611ea0f468842a3a1fe4 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 4 Oct 2022 19:54:56 +0200 Subject: [PATCH 116/128] Add link to Chinese translation by yinruiqing --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index bf750ea..224581e 100644 --- a/README.md +++ b/README.md @@ -1534,7 +1534,8 @@ company_managers_list = get_employee_list(employees=company_managers) This document is also available in other languages: +- :zh: **Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/clean-code-python) - :pt: :br: **Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) -- :iran: **Persian:** [https://github.com/SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) +- :iran: **Persian:** [SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) **[⬆ back to top](#table-of-contents)** From 7803a88d39513ec10b7ce892544dcab1aa8cba9e Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 4 Oct 2022 19:55:42 +0200 Subject: [PATCH 117/128] Fix anchor to Translations section --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 224581e..b2d8862 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ 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. [Translation](#translation) + 7. [Translations](#translation) ## Introduction From 5f61533e2b73373c731e5702a5f921fc838e9ec3 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 4 Oct 2022 19:56:28 +0200 Subject: [PATCH 118/128] Add yinruiqing as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index da6b341..2ca0c2b 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -13,3 +13,4 @@ Aaron Law Fredson Chaves MartinThoma Ali Bagheri +yinruiqing From e70e3cb05ea398a8ba09ec64390b1c46f843f275 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Tue, 4 Oct 2022 20:01:09 +0200 Subject: [PATCH 119/128] Fix certain flags --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index b2d8862..e664879 100644 --- a/README.md +++ b/README.md @@ -1534,8 +1534,8 @@ company_managers_list = get_employee_list(employees=company_managers) This document is also available in other languages: -- :zh: **Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/clean-code-python) -- :pt: :br: **Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) -- :iran: **Persian:** [SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) +- **Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/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)** From fe2ff7938ee437ae24a038c7cf92063c8b457cd9 Mon Sep 17 00:00:00 2001 From: zedr Date: Sat, 1 Apr 2023 09:04:58 +0200 Subject: [PATCH 120/128] Ignore missing return errors --- README.md | 2 +- conftest.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e664879..35ed942 100644 --- a/README.md +++ b/README.md @@ -1281,7 +1281,7 @@ class PDFDocument(Loadable): ... # Code goes here - omitted for brevity @classmethod - def load(cls, name: str): + def load(cls, name: str) -> None: """Load the file from the local filesystem""" ... # Code goes here - omitted for brevity diff --git a/conftest.py b/conftest.py index 15f8bff..cdf71bb 100644 --- a/conftest.py +++ b/conftest.py @@ -67,6 +67,9 @@ def runtest(self): _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: From 14d22bbf982ad927ba29fa83cbea377e2c56f017 Mon Sep 17 00:00:00 2001 From: zedr Date: Sat, 1 Apr 2023 09:22:56 +0200 Subject: [PATCH 121/128] Rephrase the section about function arguments --- README.md | 837 +++++++++++++++++++++++++++++------------------------- 1 file changed, 451 insertions(+), 386 deletions(-) diff --git a/README.md b/README.md index 35ed942..fd00134 100644 --- a/README.md +++ b/README.md @@ -4,34 +4,39 @@ [![](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) - 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](#translation) + +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](#translation) ## 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) +Inspired +from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript) Targets Python3.7+ ## **Variables** + ### Use meaningful and pronounceable variable names **Bad:** @@ -39,20 +44,20 @@ Targets Python3.7+ ```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. +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 @@ -62,22 +67,32 @@ 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**: -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 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 @@ -88,7 +103,7 @@ class Record: class User: - info : str + info: str @property def data(self) -> Dict[str, str]: @@ -101,17 +116,17 @@ class User: **[⬆ 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 import time - # What is the number 86400 for again? time.sleep(86400) ``` @@ -121,20 +136,20 @@ time.sleep(86400) ```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 import re - address = "One Infinite Loop, Cupertino 95014" city_zip_code_regex = r"^[^,\\]+[,\\\s]+(.+?)\s*(\d{5})?$" @@ -150,7 +165,6 @@ 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) @@ -167,7 +181,6 @@ 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})?$" @@ -175,9 +188,11 @@ 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. @@ -187,8 +202,8 @@ Explicit is better than implicit. seq = ("Austin", "New York", "San Francisco") for item in seq: - #do_stuff() - #do_some_other_stuff() + # do_stuff() + # do_some_other_stuff() # Wait, what's `item` again? print(item) @@ -200,13 +215,13 @@ for item in seq: locations = ("Austin", "New York", "San Francisco") for location in locations: - #do_stuff() - #do_some_other_stuff() + # do_stuff() + # do_some_other_stuff() # ... print(location) ``` -**[⬆ back to top](#table-of-contents)** +**[⬆ back to top](#table-of-contents)** ### Don't add unneeded context @@ -249,8 +264,8 @@ def create_micro_brewery(name): # etc. ``` -... when you can specify a default argument instead? This also makes it 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**: @@ -264,16 +279,21 @@ def create_micro_brewery(name: str = "Hipster Brew Co."): ``` **[⬆ 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. +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. **Bad:** @@ -291,6 +311,7 @@ class Menu: self.body = config["body"] # ... + menu = Menu( { "title": "My Menu", @@ -391,6 +412,7 @@ class MenuConfig: button_text: str cancellable: bool = False + def create_menu(config: MenuConfig): title, body, button_text, cancellable = astuple(config) # ... @@ -441,14 +463,16 @@ 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. + +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:** @@ -527,7 +551,6 @@ def email_client(clients: Iterator[Client]) -> None: email(client) ``` - **[⬆ back to top](#table-of-contents)** ### Function names should say what they do @@ -539,6 +562,7 @@ class Email: def handle(self) -> None: pass + message = Email() # What is this supposed to do again? message.handle() @@ -551,6 +575,7 @@ class Email: def send(self) -> None: """Send this message""" + message = Email() message.send() ``` @@ -559,8 +584,8 @@ 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:** @@ -591,9 +616,8 @@ def parse_better_js_alternative(code: str) -> None: ```python from typing import Tuple, List, Dict - REGEXES: Tuple = ( - # ... + # ... ) @@ -664,21 +688,23 @@ def create_temp_file(name: str) -> None: ### 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. +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 +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. +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:** @@ -690,6 +716,7 @@ If you can do this, you will be happier than the vast majority of other programm # However... fullname = "Ryan McDermott" + 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 @@ -697,6 +724,7 @@ def split_into_first_and_last_name() -> None: global fullname fullname = fullname.split() + split_into_first_and_last_name() # MyPy will spot the problem, complaining about 'Incompatible types in @@ -716,6 +744,7 @@ from typing import List, AnyStr def split_into_first_and_last_name(name: AnyStr) -> List[AnyStr]: return name.split() + fullname = "Ryan McDermott" name, surname = split_into_first_and_last_name(fullname) @@ -756,56 +785,59 @@ Robert C. Martin writes: "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: +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 - """ + """An element that renders an HTML comment with the program's version number + """ + + def get_version(self) -> str: + """Get the package version""" + return metadata.version("pip") - def get_version(self) -> str: - """Get the package version""" - return metadata.version("pip") - - def render(self) -> None: - print(f'') + def render(self) -> None: + print(f'') VersionCommentElement().render() ``` + This class has two responsibilities: - - Retrieve the version number of the Python package - - Render itself as an HTML element +- Retrieve the version number of the Python package +- Render itself as an HTML element Any change to one or the other carries the risk of impacting the other. -We can rewrite the class and decouple these responsibilities: +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) +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 - """ + """An element that renders an HTML comment with the program's version number + """ + + def __init__(self, version: str): + self.version = version - def __init__(self, version: str): - self.version = version - - def render(self) -> None: - print(f'') + def render(self) -> None: + print(f'') VersionCommentElement(get_version("pip")).render() @@ -813,9 +845,9 @@ VersionCommentElement(get_version("pip")).render() 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 +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. @@ -827,9 +859,10 @@ As an added bonus, the `get_version()` is now reusable elsewhere. > 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. +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 @@ -841,48 +874,49 @@ 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""" + """An HTTP response""" - status: int - content_type: str - body: str + status: int + content_type: str + body: str class View: - """A simple view that returns plain text responses""" + """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" - ) + 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.""" + """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() - ) + 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() + ) ``` 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()` +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. @@ -891,47 +925,48 @@ Let's redesign our classes to fix this problem and let the `View` class be extended (not modified) cleanly: **Good** + ```python from dataclasses import dataclass @dataclass class Response: - """An HTTP response""" + """An HTTP response""" - status: int - content_type: str - body: str + status: int + content_type: str + body: str class View: - """A simple view that returns plain text responses""" + """A simple view that returns plain text responses""" - content_type = "text/plain" + content_type = "text/plain" - def render_body(self) -> str: - """Render the message body of the response""" - return "Welcome to my web site" + 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() - ) + 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.""" + """A view that returns HTML responses based on a template file.""" - content_type = "text/html" - template_file = "index.html" + 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() + def render_body(self) -> str: + """Render the message body as HTML""" + with open(self.template_file) as fd: + return fd.read() ``` @@ -942,17 +977,22 @@ that **invites subtypes to override it**. It is designed to be extended by its subtypes. 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/). +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): ...` + +- 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 @@ -960,74 +1000,74 @@ from typing import Protocol @dataclass class Response: - """An HTTP response""" + """An HTTP response""" - status: int - content_type: str - body: str - headers: dict = field(default_factory=dict) + status: int + content_type: str + body: str + headers: dict = field(default_factory=dict) class View: - """A simple view that returns plain text responses""" + """A simple view that returns plain text responses""" - content_type = "text/plain" + content_type = "text/plain" - def render_body(self) -> str: - """Render the message body of the response""" - return "Welcome to my web site" + 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() - ) + 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 TemplateRenderMixin: - """A mixin class for views that render HTML documents using a template file - - Not to be used by itself! - """ - template_file: str = "" + """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.") + 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() + 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 + """A mixin class for views that injects a Content-Length header in the + response + + Not to be used by itself! + """ - 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 + 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.""" + """A view that returns HTML responses based on a template file.""" - content_type = "text/html" - template_file = "index.html" + content_type = "text/html" + template_file = "index.html" ``` 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. +related functionality into a highly reusable class with a single +responsibility, allowing clean decoupling. Class extension is achieved by " +mixing-in" the additional classes. The popular Django project makes heavy use of Mixins to compose its class-based views. @@ -1047,73 +1087,75 @@ computer scientist Jeannette Wing on the seminal paper "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. +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""" + """An HTTP response""" - status: int - content_type: str - body: str + status: int + content_type: str + body: str class View: - """A simple view that returns plain text responses""" + """A simple view that returns plain text responses""" - content_type = "text/plain" + content_type = "text/plain" - def render_body(self) -> str: - """Render the message body of the response""" - return "Welcome to my web site" + 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() - ) + 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.""" + """A view that returns HTML responses based on a template file.""" - content_type = "text/html" + 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 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) + """Render a View""" + return view.get(request) ``` -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` +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`. -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: +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" @@ -1131,15 +1173,16 @@ error: Signature of "get" incompatible with supertype "View" 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". +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". Python does not have interfaces. We have Abstract Base Classes instead, which are a little different, but can serve the same purpose. **Good** + ```python from abc import ABCMeta, abstractmethod @@ -1147,26 +1190,26 @@ 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.""" + """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""" + @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""" + """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}!") + @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 a user with a given name using the provided actor""" + actor.greet(user_name) welcome_user("Barbara", FriendlyActor()) @@ -1179,26 +1222,27 @@ documents, so we go ahead and design a comprehensive abstract base class for our document. **Error** + ```python import abc class Persistable(metaclass=abc.ABCMeta): - """Serialize a file to data and back""" + """Serialize a file to data and back""" - @property - @abc.abstractmethod - def data(self) -> bytes: - """The raw data of the file""" + @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""" + @classmethod + @abc.abstractmethod + def load(cls, name: str): + """Load the file from disk""" - @abc.abstractmethod - def save(self) -> None: - """Save the file to disk""" + @abc.abstractmethod + def save(self) -> None: + """Save the file to disk""" # We just want to serve the documents, so our concrete PDF document @@ -1206,23 +1250,23 @@ class Persistable(metaclass=abc.ABCMeta): # a public attribute named `data`. class PDFDocument(Persistable): - """A PDF document""" + """A PDF document""" - @property - def data(self) -> bytes: - """The raw bytes of the PDF document""" - ... # Code goes here - omitted for brevity + @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 + @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 + """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 ``` @@ -1234,8 +1278,8 @@ 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. +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 @@ -1244,52 +1288,58 @@ 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. +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): - ... + """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): - ... + """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: - ... + """Can save data to storage""" + + @abc.abstractmethod + def save(self) -> None: + ... class PDFDocument(Loadable): - """A PDF document""" + """A PDF document""" - @property - def data(self) -> bytes: - """The raw bytes of the PDF document""" - ... # Code goes here - omitted for brevity + @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 + @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 + """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 ``` @@ -1303,84 +1353,92 @@ 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 + """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 + 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 ``` -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. +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` +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 + """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"'}, - ) + """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. +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) @@ -1390,26 +1448,27 @@ by this author. ## **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! - -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 +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! + +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:** @@ -1418,53 +1477,57 @@ updating multiple places any time you want to change one thing. 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 - }) + '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 - }) + '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'), @@ -1486,31 +1549,32 @@ company_managers_list = get_manager_list(managers=company_managers) 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 - }) + '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'), @@ -1526,16 +1590,17 @@ company_managers = [ 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) -- 🇵🇹 🇧🇷 **Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) -- 🇮🇷 **Persian:** [SepehrRasouli/clean-code-python](https://github.com/SepehrRasouli/clean-code-python) +- ** + Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/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)** From 155cb0dfbbd7bb738b7e1f964f570301fdf2dd1f Mon Sep 17 00:00:00 2001 From: zedr Date: Sat, 1 Apr 2023 09:25:02 +0200 Subject: [PATCH 122/128] Address functions doing one thing first --- README.md | 174 +++++++++++++++++++++++++++--------------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/README.md b/README.md index fd00134..3ab41d1 100644 --- a/README.md +++ b/README.md @@ -282,6 +282,93 @@ def create_micro_brewery(name: str = "Hipster Brew Co."): ## **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) A large amount of parameters is usually the sign that a function is @@ -466,93 +553,6 @@ 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. - -**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 names should say what they do **Bad:** From 59359a8d326d62a8f6cb7e35e32e28960a9530e6 Mon Sep 17 00:00:00 2001 From: zedr Date: Sat, 1 Apr 2023 09:26:07 +0200 Subject: [PATCH 123/128] Give due credit correctly --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3ab41d1..4aa8d41 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ 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 +Adapted from [clean-code-javascript](https://github.com/ryanmcdermott/clean-code-javascript) Targets Python3.7+ From 3835cef302e1a2ba14968c0e45eac227c6651770 Mon Sep 17 00:00:00 2001 From: zedr Date: Sat, 1 Apr 2023 09:41:41 +0200 Subject: [PATCH 124/128] Add a clarification on moving parameters into objects --- README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 4aa8d41..a1f26c8 100644 --- a/README.md +++ b/README.md @@ -380,7 +380,11 @@ 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. +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:** From ea33f1dc57f4012b949deecfeba1735cf202eac4 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 1 Apr 2023 09:45:18 +0200 Subject: [PATCH 125/128] Add Chinese flag --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a1f26c8..c353080 100644 --- a/README.md +++ b/README.md @@ -1600,7 +1600,7 @@ company_managers_list = get_employee_list(employees=company_managers) This document is also available in other languages: -- ** +- 🇨🇳 ** Chinese** [yinruiqing/clean-code-python](https://github.com/yinruiqing/clean-code-python) - 🇵🇹 🇧🇷 ** Portugese** [fredsonchaves07/clean-code-python](https://github.com/fredsonchaves07/clean-code-python) From 99d935792183231e9af835e636817a337df62ef8 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 1 Apr 2023 09:46:58 +0200 Subject: [PATCH 126/128] Fix broken anchor --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c353080..da1f8a8 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ 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](#translation) +7. [Translations](#translations) ## Introduction From a971906c651a35e7149411744c7200cdc51ee790 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 10 Jun 2023 16:48:51 +0200 Subject: [PATCH 127/128] Add Korean translation link --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index da1f8a8..cae487b 100644 --- a/README.md +++ b/README.md @@ -1602,6 +1602,7 @@ 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) - 🇮🇷 ** From dfa0f7c24a4f75ddd5763291b498dd2a9cb73db2 Mon Sep 17 00:00:00 2001 From: Rigel Di Scala Date: Sat, 10 Jun 2023 16:49:35 +0200 Subject: [PATCH 128/128] Add wooy0ng as a contributor --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 2ca0c2b..96657b2 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -14,3 +14,4 @@ Fredson Chaves MartinThoma Ali Bagheri yinruiqing +YongWoo Lee