バックアップ：PHPerのための「PHPのリーダブルなコード」を語り合うPHP TechCafe
=========================================
# PHPer's NEWS
# 特集：PHPのリーダブルなコード
* PHPのBADコードをいくつか用意
* どこが悪いかを議論しながら（時間が許す限り）綺麗に直していく
* サンプルコードは全部で初級から上級まで8つ
### 参考
[jupeter/clean-code-php](https://github.com/jupeter/clean-code-php#introduction)
------------------
### 初級
#### 真偽値は直接それを返す
##### ダメなコード
```php=
if (foo === bar) {
return true;
} else {
return false;
}
```
##### ダメな理由
* 結果をtrueまたはfalseで返したいとき、if文を使用するのは冗長です。
##### 解消例
```php=
return foo === bar;
```
#### if文の条件はシンプルに（廣部）
##### ダメなコード
```php=
if($isOK && !hasPermission && $hoge !== "sample")
// なんらかの処理
}
```
##### ダメなコード
if文の条件が複雑になると、単純に読みにくくなるだけでなく、不具合も起こりやすくなります
##### 解消例
早期リターンを使う
```php=
if (!$isOK){
return "NG";
}
if (hasPermission){
return "OK";
}
if ($hoge !== "sample"){
//なんらかの処理
}
```
関数に閉じ込める
```php=
if (条件がわかりやすく言語化された関数名()){
// なんらかの処理
}
```
#### 曖昧な比較演算子
##### ダメなコード
```php=
if ($input == 0) {
echo('0です！');
}
```
##### ダメな理由
PHPにおいて曖昧な比較演算子を使った比較は、想定外のバグの原因になるため注意が必要。
（特にユーザーの入力値やnullやfalseなどが入る可能性がある場合）
基本的には厳密な比較を使用した方がいい。
###### 曖昧な比較の場合
```php=
var_dump('001' == 1); // true
var_dump(' 1' == 1); // true
var_dump(null == 0); // true
var_dump(null == []); // true
var_dump(null == ''); // true
```
※PHPのバージョンが8以前の場合は、文字列と数値を比較する際に文字列が数値にキャストされるためさらに危険
```php=
var_dump(0 == "a"); // true
```
##### 解消例
```php=
if ($input === 0) {
echo('0です！');
}
```
#### ネストは浅くする
参考
https://github.com/jupeter/clean-code-php#avoid-nesting-too-deeply-and-return-early-part-1
### ダメなコード
```php=
/**
* お店が営業日かをチェックする
*
* @param $day 曜日の文字列
* @return bool
*/
function isShopOpen($day): bool
{
if ($day) {
if (is_string($day)) {
$day = strtolower($day);
if ($day === 'friday') {
return true;
} elseif ($day === 'saturday') {
return true;
} elseif ($day === 'sunday') {
return true;
}
return false;
}
return false;
}
return false;
}
```
### ダメな理由
1. どういうチェックが必要なのかがわかりにくい
* 分岐をすべて読まないとチェックしたい内容がわからない
* day が以下の場合に true になる
* friday
* saturday
* sunday
* if-else で条件分岐を作る際は、単純化できないか考えるチャンス
* 分岐を進むにつれて、該当処理が実行される条件の数が多くなる
* これまでの分岐条件を記憶しなければならない
2. 引数に型が指定されていないためメソッド内で値のチェックをしなければならない
* ロジックを読み始めてから、Stringの値が前提になっている処理であることを理解する
* 引数の名前から DateTime クラスのインスタンスを受け取るのかと思った
* 実際は String を受け取ることを前提とした処理になっていることを後から理解
3. 前提部分のチェック処理はネストを深くせずガード節で対応する
* 処理の最初で return する
### 解消例
```php=
/**
* お店が営業日かをチェックする
*
* @param string $day 曜日の文字列
* @return bool
*/
function isShopOpen(string $day): bool
{
// 値がセットされているか（これがガード節）
if (empty($day)) {
return false;
}
// 定休日一覧
$openingDays = ['friday', 'saturday', 'sunday'];
// 定休日に該当する場合は true を返す
return in_array(strtolower($day), $openingDays, true);
}
```
### フラグ引数で処理を切り替える
#### 良くないコード
```php=
// 問題のあるコード例
// 別のビジネス概念を無理に1つの関数で処理するためにフラグ引数を渡している。
// 今後更に別のビジネス概念が増えるとより複雑化すると想定される
// 実装者は共通化する意図を持っていたと思われるが、結局共通化できていない
// メルマガ購読顧客リストまたはYoutubeのチャンネル登録会員リストを更新する
function updateMailMagazineListOrYoutubeList($accountId, $MailMagazine, $Youtube, $isMailMagazine, $isYoutube) {
if ($isMailMagazine) {
# code...
} else if($isYoutube) {
# code...
}
if ($isMailMagazine) {
$sql = "update mailMagazineList set ...";
} else if($isYoutube) {
$sql = "update youtubeLst set ...";
};
}
```
#### 修正後のコード
```php=
// 異なるビジネス概念を扱うなら関数・クラスを別にする
function updateMailMagazineList($accountId, $MailMagazine) {
# code ...
$sql = "update mailMagazineList set ...";
# fugafuga...
}
function updateYoutubeList($accountId, $Youtube) {
# code ...
$sql = "update mailMagazineList set ...";
// hogehoge...
}
```
## メソッド内で複数の目的を達成しようとしない
参考
https://github.com/jupeter/clean-code-php#functions-should-only-be-one-level-of-abstraction
### ダメなコード
```php=
/**
* PHPによる形態素解析処理
*
* @param string $code 文章
*/
function parseBetterPHPAlternative(string $code): void
{
$regexes = [
// ...
];
$statements = explode(' ', $code);
$tokens = [];
foreach ($regexes as $regex) {
foreach ($statements as $statement) {
// ...
}
}
$ast = [];
foreach ($tokens as $token) {
// lex...
}
foreach ($ast as $node) {
// parse...
}
}
```
### ダメな理由
1. 1つの関数で複数の処理を行っている
* 修正時の影響が大きくなる
* 修正箇所の後続処理への影響を考えて修正しなければならない
2. 不具合が発生した際の問題個所の特定が困難
* 複数の処理が組み込まれていると処理の前後を理解する必要がある
* 処理が分割していれば問題箇所も特定しやすい
4. 内部処理の再利用ができない
* 処理の中に組み込まれてしまうと特定処理だけを他の処理でも利用したくても利用できない
5. テストが書きにくい
* 複数の処理がまとまっていると細かいテストコードが書けない
* 処理が分割されていれば細かな条件のテストコードが書ける
### 解消例
```php=
/**
* トークナイザ
*/
class Tokenizer
{
/**
* 文章を単語に分解する
*
* @param string $code 文章
* @return array $tokens 単語のリスト
*/
public function tokenize(string $code): array
{
$regexes = [
// ...
];
$statements = explode(' ', $code);
$tokens = [];
foreach ($regexes as $regex) {
foreach ($statements as $statement) {
$tokens[] = /* ... */;
}
}
return $tokens;
}
}
/**
* 字句解析器
*/
class Lexer
{
/**
* 単語の解析処理
*
* @param string $tokens 単語のリスト
* @return array $ast 解析結果のリスト
*/
public function lexify(array $tokens): array
{
$ast = [];
foreach ($tokens as $token) {
$ast[] = /* ... */;
}
return $ast;
}
}
/**
* PHPによる形態素解析処理
*
* @param string $code 文章
*/
class BetterPHPAlternative
{
/** @var Tokenizer */
private $tokenizer;
/** @var Lexer */
private $lexer;
public function __construct(Tokenizer $tokenizer, Lexer $lexer)
{
$this->tokenizer = $tokenizer;
$this->lexer = $lexer;
}
/**
* 形態素解析
*
* @param string $code 文章
*/
public function parse(string $code): void
{
$tokens = $this->tokenizer->tokenize($code);
$ast = $this->lexer->lexify($tokens);
foreach ($ast as $node) {
// parse...
}
}
}
```
## フラグでなんでも解決しようとする
### ダメなコード
#### 修正前のコード
```php=
public function getRecipeListString():string {
$recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの
return implode(",", recipeList);
}
```
#### 修正後のコード
```php=
public function getRecipeListString(bool $isSpace):string {
$recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの
if ($isSpace) {
return implode(" ", recipeList);
} else {
return implode(",", recipeList);
}
}
```
### ダメな理由
* if文をどんどん増やすことになる
* 違う要件が来た時にまたif文が増える
* 保守性も可読性も低い
### 解消例
処理を分けるパターン
```php=
public function getRecipeListStringWithComma():string {
$recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの
return implode(",", recipeList);
}
public function getRecipeListStringWithSpace():string {
$recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの
return implode(" ", recipeList);
}
```
デリミタを引数にするパターン
```php=
public function getRecipeListString(string $delimiter):string {
$recipeList = getRecipeList(); // なにかレシピのリストを配列で取得するもの
return implode($delimiter, recipeList);
}
```
## 依存性逆転の原則(DIP)に反している
参考
https://github.com/jupeter/clean-code-php#dependency-inversion-principle-dip
### ダメなコード
```php=
<?php
class FavoriteRecipe {
public function getRecipe(string $name):void {
$limit = 10;
$recipeRepository = new RecipeRepository();
$recipes = $recipeRepository.findByName($name, $limit);
foreach ($recipes as $recipe) {
var_export($recipe);
}
}
}
```
```php=
<?php
class RecipeRepository {
// CookPadからレシピを取得する
function findByName(string $name, int $limit):Recipe {
$cookpad = new Cookpad(new \PHPHtmlParser\Dom);
$result = $cookpad->search($name, 1, $limit, false);
return $result;
}
}
```
### ダメな理由
FavoriteRecipeクラスがRecipeRepositoryクラスの実装に依存している
domain層
FavoriteRecipe
↓
infra層
RecipeRepository
この場合RecipeRepositoryクラスのfindByNameメソッドの変更がFavoriteRecipeクラスにも波及する。
(例)findByNameメソッドの引数を増やした場合
⇒FavoriteRecipeクラスのgetRecipeメソッドの`$recipeRepository.findByName($name, $limit);`も修正する必要が出てくる
RecipeRepositoryクラスのfindByNameメソッドを修正
```php=
<?php
class RecipeRepository {
// 引数を修正
function findByName(string $name,int $pageNum, int $limit, bool $isRandom):Recipe[] {
$cookpad = new Cookpad(new \PHPHtmlParser\Dom);
$result = $cookpad->search($name, $pageNum, $limit,$isRandom);
return $result;
}
}
```
FavoriteRecipeクラスのgetRecipeメソッドを修正
```php=
<?php
class FavoriteRecipe {
public function getRecipe(string $name):void {
$pageNum = 10;
$limit = 10;
$isRandom = false;
$recipeRepository = new RecipeRepository();
// reposiotoryの修正を受けて引数を修正
$recipes = $recipeRepository.findByName($name, $pageNum, $limit, $isRandom);
foreach ($recipes as $recipe) {
var_export($recipe);
}
}
}
```
`$recipeRepository.findByName`を利用している場所が他にもあれば修正が波及する。
実装の詳細に変更は付き物。実装の詳細に依存することで変更に脆くなる。
Clean Codeの例でいうと、継承元の変更が派生クラスに影響する。
→これって継承の問題では？と思ったので自分でコード書きました。
### 解消例
抽象に依存せよ
インターフェースを作成
```php=
<?php
interface RecipeRepository {
public function findByName(string $name, $pageNum = 10, $limit = 10, $isRandom = false):Recipe[];
}
```
Reoisitoryクラスはインターフェースを実装するよう修正
```php=
<?php
class RecipeRepositoryImpl implements RecipeRepository {
public function findByName(string $name, $pageNum = 10, $limit = 10, $isRandom = false): Recipe[] {
$cookpad = new Cookpad(new \PHPHtmlParser\Dom);
$result = $cookpad->search($name, $pageNum, $limit, $isRandom);
return $result;
}
}
```
FavoriteRecipeクラスはインターフェースを利用する。
```php=
<?php
class FavoriteRecipe {
private RecipeRepository $recipeRepository;
public function __construct(RecipeRepository $recipeRepository) {
$this->recipeRepository = $recipeRepository;
}
public function getRecipe(string $name):void {
$pageNum = 10;
$limit = 10;
$isRandom = false;
$recipes = $this->recipeRepository.findByName($name, $pageNum, $limit, $isRandom);
foreach ($recipes as $recipe) {
var_export($recipe);
}
}
}
```
domain層
FavoriteRecipe
↓
infra層
RecipeRepository
↑
RecipeRepositoryImpl
RecipeRepositoryインターフェースをDomain層に移動させると
domain層
FavoriteRecipe
↓
RecipeRepository
↑
infra層
RecipeRepositoryImpl
上位モジュールが下位モジュールに依存しなくなる。
誰がインスタンスを渡すのか,DIって何？LaravelのDIの実装,Clean Architectureの話もできる（やりすぎ）
Sign in with Wallet
Connect another wallet