2023/09/13(水)else句を書く必要性について
更新日：
投稿日：
投稿日：
今の職場でコードを書いていてifに対して原則elseを書かないという雰囲気があるのですが、個人的にはelseを書きたいのでその話です。
世間的にelseを書かないというコーディングルールは一定の支持があるようで、まぁ一種の宗教だとは思っていますが、個人的にはどうなんかなと思っています。まぁ割と最近の流行りな気もしていますが…。（昔はなかったと思う…というとアレですが）
elseがあってほしい理由
else ifやelseブロックがあると処理の関連性が見やすくなると考えています。
処理の関連性がわかりやすくなる
例えば以下のコードの場合、ifが連続しているよりelseがある方が処理の関連性が明示的に見えると思います。elseで繋げないことは基本的に処理として関連性がないはずです。
// ifのみ
export const hoge = (mode: 1|2|3) => {
if (mode === 1) {
// なんかの処理
return 'hoge';
}
if (mode === 2) {
// なんかの処理
return 'piyo';
}
if (mode === 3) {
// なんかの処理
return 'fuga';
}
}
// elseあり
export const piyo = (mode: 1|2|3) => {
if (mode === 1) {
// なんかの処理
return 'hoge';
} else if (mode === 2) {
// なんかの処理
return 'piyo';
} else if (mode === 3) {
// なんかの処理
return 'fuga';
}
}
予期せぬ不具合が入り込みづらくなる
ifだけで構成されたコードでは以下のようにifブロックの外に処理を入れ込むことができますが、このときA1, B1, C1のifの外にある処理は後続処理に影響します。しかしこの書き方だと処理の影響範囲が見積もりづらく、不必要なバグを生む元になるため、個人的には避けたいと考えています。（他の処理の作用に引っ張られているので、副作用のある実装ということになると思っています）
またA1, B1, C1の行数が伸びるとコードの見通しが悪くなり、if同士の関係性が分かりづらくなります。
elseを使うとこういった副作用的な振る舞いをする実装が作りづらくなり、影響範囲もブロックインデントで分かれるので、そういった心配がなくなります。
// ifのみ
export const hoge = (mode: 1|2|3) => {
// なんかの処理 A1
if (mode === 1) {
// なんかの処理 A2
return 'hoge';
}
// なんかの処理 B1
if (mode === 2) {
// なんかの処理 B2
return 'piyo';
}
// なんかの処理 C1
if (mode === 3) {
// なんかの処理 C2
return 'fuga';
}
}
elseが必要なのに書き忘れることが減る
elseを書かないことに捕らわれていると以下のようなコードが生まれることがあります。
さてこのコードにおいて!validHoge(hoge) || !hasPiyoのケースはどのように処理されるのでしょうか？答えは処理されませんが、もしこのコードに対してUnit testが実装されておらず、特段のドキュメントもなければ、それが正しいのかどうかをコードから読み取ることが出来ません。
const isValidUsername = (username: string, hasInputed: boolean) => {
if (validUsername(username) && hasInputed) {
return requestSearchUsername(username);
}
}
せめてこう書いてあれば判断も付くでしょう。
const isValidUsername = (username: string, hasInputed: boolean) => {
if (validUsername(username) && hasInputed) {
return requestSearchUsername(username);
} else {
// 何もしない
}
}
因みにこの処理はユーザー登録フォームでユーザー名の書式が正しければ既に存在するユーザー名かどうかをAPIに問い合わせ、既に存在すればエラーメッセージを、存在しなければtrueを返すという内容ですが、そもそも書式が不正である場合は何もしないため、ユーザーはエラーであることを知ることが出来ません。
もしelseブロックを書いていれば考慮漏れに気づけた可能性もあったのではないかと思います。
elseを使うケースを考えなくて良くなる
ifに対して原則elseを書かないというルールがあってもelseを書かないと成り立たないケースは存在します。そう言った場合に原則ifしか書いてはならないというルールがあるとelseを書いてよいかどうか考える必要が出てきますが、元からelseも書いて良いルールであればそこは考える必要がありません。個人的に判断の余地が生じるコーディングルールはチーム開発ではない方が良いと考えています。
またこのルールの結果、本来必要だったelseを書かなかったことにより不具合が発生する可能性もあります。（実装者が軽率にelseを使わなかったことによる判断ミス）
そもそも何故elseを書いてはならないのでしょうか？コードのネストが深くなるからでしょうか？少なくともelse if相当のifには、その作用はありません。あるとしたら純粋なelseブロックはインデントが減るでしょう。しかしこのインデントはあったほうが処理の関連性が掴みやすくなると思います。
elseのないifは悪か？
ここまで散々elseを書くべきと言ってきましたが、ではelseがないifは悪かと言うと、そうは思いません。例えば、以下のような早期リターンと呼ばれる記法であればそれは良いと考えています。では何を早期リターンとするかですが、個人的には例外的に処理を継続しない場合を一つの基準にしています。私もここの感覚は上手く言語化できていないのですが、恐らく何もしないときだけelseを書かないのは問題ないと考えています。
これは本質的ではない処理のせいで、いたずらにネストが増えるのを防ぐためです。関連性がある場合にelseを使うのがコードのメリハリとして目視で読んだときの認知性の向上に繋がると考えています。
export const putLog = (message: string) => {
if (message === '') return;
console.log(message);
}
この点については個人的に共感できる意見があったので以下の記事を紹介させて頂きます。
else句を使わないのが良いコードなの？いや、そんなはずは・・・ · DQNEO日記
記事中にある以下の部分、特殊ケースに対してガード節を使うというところで、例外ケースでのみ早期リターンをするという部分がありますが、やはりこれが一番無難だなと思いました。
条件記述の単純化 「ガード節による入れ子条件記述の置き換え」
特殊ケースに対してガード説を使う
ただ例外ケースとは何か？それは数値化でき、チーム開発で標準的に取り扱えるものなのか？と言われると、正直私もそこまでは言語化できていないので難しい部分ではあります。そもそも人間が書くコードが完全に均一になることはないと考えているので、そこまで強い思想を持って考えてはいないです。（もしChatGPTに書かせてもバラツキは出るでしょう）
私がこの記事で言いたいのは単にelseを書いてもいいのではないかということと、その場合でも早期リターンは許容しても問題ないだろうというところなので、一旦早期リターンの基準についてはここでは考えないこととします。
そうなると結局elseなしでifだけあればいいじゃないかという話に戻ってくるとは思うのですが、正直そこは常識で考えてほしいというところです。まぁ常識も人の数だけあるので難しいですし、そんな物があればこんな記事も生まれていないわけで、ソフトウェア開発は難しいですね。
2023/08/03(木)JavaScriptのundefined判定の処理速度比較
更新日：
投稿日：
投稿日：
undefinedの判定方法が複数あるということでundefined判定の処理速度比較をしてみたのでその結果。
端的に言うと、hoge === undefinedとtypeof hoge === 'undefined'の二方式がある。後者は原則考慮不要だが、言語仕様上存在しているので比較したが、現実的に見た場合、どちらで記述した場合でも処理速度に有意な差はないように感じた。
確認環境
| Env | Ver |
|---|---|
| Node.js | 20.1.0 |
| TypeScript | 4.9.5 |
| @swc/core | 1.3.8 |
比較結果
hoge === undefinedの方が早く見えるが実行するタイミングで変わるので誤差の範疇だと思う。
| 方式 | ms |
|---|---|
hoge === undefined |
4,514 |
typeof hoge === 'undefined' |
4,515 |
確認コード
const tyof = (param?: string) => {
return typeof param === 'undefined';
};
const undef = (param?: string) => {
return param === undefined;
};
const tyStart = +new Date();
for (let i = 0; i < 10000000000; i++) {
tyof();
}
console.log('typeof', +new Date() - tyStart);
const unStart = +new Date();
for (let i = 0; i < 10000000000; i++) {
undef();
}
console.log('undefined', +new Date() - unStart);
TSから生成されたJS
"use strict";
Object.defineProperty(exports, "__esModule", {
value: true
});
const tyof = (param)=>{
return typeof param === 'undefined';
};
const undef = (param)=>{
return param === undefined;
};
const tyStart = +new Date();
for(let i = 0; i < 10000000000; i++){
tyof();
}
console.log('typeof', +new Date() - tyStart);
const unStart = +new Date();
for(let i = 0; i < 10000000000; i++){
undef();
}
console.log('undefined', +new Date() - unStart);
あとがき
MDNを読む限りtypeof hoge === 'undefined'は該当変数が存在しない場合に有用なようであるが、TypeScriptで書いている場合、通常このようなコードが生まれることがなく、仮に起きるとした場合、次のようなコードになるため現実的に考慮する必要はない。なおMDNにも「こんなことはしないこと」と書いてあるので、一般的なコードでないことは客観的にも伺えるだろう。
(() => {
const undefined = 123;
const hoge = undefined;
if (typeof hoge === 'undefined') {
console.log('hoge is undefined');
} else {
console.log('hoge is not undefined');
}
})();
上記コードの実行結果としてはhoge is not undefinedが出力される。
このコードの主な問題点
const undefined = 123;というコードは予約語を変数名にしているため、混乱を招くコードであり、書かないことが好ましい- MDNには予約語ではないとあるが、一般的には予約語の一つとして解釈して支障ないと考える
- このコードはESLintのeslint:recommendedで検知されるため、通常であれば書かれることはない
なお、このコードは例示のために即時実行関数形式で記述しているが、必要がない限りこの形式での実装は避けたほうが問題が少なくなると思う。これは不必要なネストが生まれたり、スコープの混乱を生むためである。
2022/03/09(水)シンプルで事故が起き辛いGitのブランチフローを考えて実務で運用した話
更新日：
投稿日：
投稿日：
これは、かつて参画したGitHubを利用したプロジェクトでブランチフローが悪く事故が多発したので考案し、運用した内容です。
基本的には後々の運用を考えた時に情報源になり、かつGit操作に極力手間を取られることがなく、CI/CDを回しながら品質を維持できる内容で考えています。
フローの要件として考えたこと
取り回しが単純明快であること
開発以外の要素に振り回されないように単純なフローにしていて、世に言われる履歴の綺麗さとか言うのは個人的には関心が薄いので重視していません。その代わりコミットが壊れないことや、インデックスとして見やすくなるような部分を重視しています。
GitHubとの相性が良いこと
まず個々の開発タスクをIssueベースで管理し、Pull Requestベースで取り込む運用としました。
Pull Requestの取り込み方式はSquash Mergeとし、メインブランチのコミット履歴がPull Requestのマージコミットになるようにしました。
これはメインブランチのコミット履歴はPull Requestのマージコミットだけあれば後から追えるというのと、メインブランチのコミットログを単純にする意味でこの方式にしています。
CI/CDを活用しやすいこと
これは割とどこでもやっていると思いますが、ブランチ名にdevelopとかstaging, productionとか付けて管理することで、自動的に環境を識別できるようにしました。
実際に運用したフロー
フロー図
フローの運用内容
develop/mainブランチを最新ブランチとする運用develop/mainブランチ相当のものが複数ある状態というのが世の中にあると思いますが、管理が非常に大変なのでそれはしない方向にしました
- 機能ブランチを
develop/mainブランチに取り込むのはSquash Merge- 基本的に変更履歴を見る時はGitLensや
blameで変更行からPull Requestを当てて、そこを見に行くという運用にしていました
- 基本的に変更履歴を見る時はGitLensや
- 機能ブランチに
develop/mainブランチを取り込むのはmerge- 一般的には
rebaseが多いと思いますが、次の観点から採用しませんでした- どのポイントで取り込んだのかわからない
mergeと比較した場合にコンフリクト対応に手を取られる- 経験上ここで事故が頻発する
- 素直に
push出来ない
mergeであれば以下のように単純な流れに出来ますし、push前に差分確認して事故を防ぐことも容易ですgit switch develop/maingit pullgit switch -git merge -- コンフリクトがあれば解消
git push
- 一般的には
- デプロイ方式によるルートブランチ分割
- ルートブランチ名によってデプロイ先を変更できるようにしました
- GitHub Actionsでブランチ名を拾って環境変数を差し替えることでデプロイするワークフローを組んでいます
staging/やproduction/ブランチはdevelop/mainからcheckoutする運用です- これらはデプロイするためだけの使い捨てブランチなので毎回生えます
あとがき
このフローの利点としてはマージでコンフリクトが起きても基本的にCurrentとIncomingを一回比較するだけで済むのでコンフリクトの解消が簡単で、コンフリクト時はIncomingが壊れないようにCurrentを直すのが基本になり、Currentを優先する場合は適宜上書きするといった内容です。
ブランチの合流はmergeだとマージコミット分一回の解消だけでいいので事故の発生要因が低いのがrebaseと比較した時の利点です。rebaseだとコミット回数分再帰的に合流させる必要があるのでブランチの寿命が長かったりすると苦行になってきます。
このフローができた経緯としては元々はGitHub Flowをベースにしていたのですが、色々やっていくうちにこうしたらもっと良くなるのではないか？というのを試行錯誤していてこの形に落ち着いたのですが、後から調べたらGitLab Flowに近い形式に見えたので、似た内容は既に誰かが考えているものだなと感じました。
上で挙げた内容の他にもGitHubのリポジトリ設定でブランチ保護のルールを設定したり、Pull Requestのマージ設定でSquash mergingだけ許可したり、ヘッドブランチの自動削除をするなど、基本的に面倒なことを考えたり、しなくて良い様にするなど、開発に注力しやすいように環境を整えると心理的な抵抗が少なく運用できて便利だなと感じています。
2022/01/10(月)Reactでより良い開発をするために意識したいこと
更新日：
投稿日：
投稿日：
別にReactに限った話でもないのですが、実務で悩ましいコードにあたって頭を抱えてる内にハゲてきたので、ハゲがこれ以上進まない事を祈り書きました。いやいっそのこと全ハゲになりたいですが、それはさておき
とりあえず個人的には以下2点を意識すれば起きないとは思うので、Next.jsを使った簡単なサンプルを交えながら3例ほどケーススタディ形式で紹介していきます
- 密結合にしない
- シンプルに実装する
挙げている事例については例示用にフルスクラッチで書いています（主題と関係ないコードは端折ってます
ここはこうした方がより良いのでは？などのご意見があれば是非コメントいただけると嬉しいです
コンポーネントのOptional引数のオーバーライド
コンポーネント引数をコンポーネント側で書き換えるような実装はどうかなと思います
問題点
- 親の預かり知らぬところで値が設定されている
- 誰かが知らずに設定値をすり替えたりしたら不具合が起きそうです
- 型のコメントと設定値が異なる
- 型のコメントと実装が異なるので、誰かが直すかもしれません
- すると、コンポーネントを参照している全コンポーネントに影響が波及します
- そもそも型と実装は本質的に関係ないので、こういった運用はNG
- 型のコメントと実装が異なるので、誰かが直すかもしれません
一例
type AccountContainerProps = {
email: string;
// デフォルトtrue
isUpdate?: boolean;
// デフォルトfalse
gotNotify?: boolean;
};
export const AccountContainer = ({
isUpdate = true,
gotNotify = true,
...props
}: AccountContainerProps) => {
...
}
改善案
呼び元で明示的に指定するように変更しています
これによって親は子の実装を知る必要がなくなり、責務がそこで閉じるようになりました
改善点
- 親の預かり知らぬところで値が設定されなくなった
- これで子コンポーネントで値が変わることに怯える必要はなくなりました
- 型の初期値コメントを削除できた
- 実装と一致する保証がないコメントは削除するべきでしょう
type AccountContainerProps = {
email: string;
isUpdate: boolean;
gotNotify: boolean;
};
export const AccountContainer = (props: AccountContainerProps) => {
...
}
コンポーネントのOptional引数の多重オーバーライド
コンポーネントのOptional引数のオーバーライドが多重化されている上に、なんか途中で更に書き換えられているとかいう地獄
どうしてそんなことをするのか…
一例
type AccountTemplateProps = {
email: string;
// デフォルトtrue
isUpdate?: boolean;
// デフォルトfalse
gotNotify?: boolean;
from: 'register' | 'update';
};
export const AccountTemplate = ({
isUpdate = false,
gotNotify = true,
...props
}: AccountTemplateProps) => {
if (props.from === 'register') isUpdate = false;
return <AccountContainer {...props} />;
};
type AccountContainerProps = {
email: string;
// デフォルトtrue
isUpdate?: boolean;
// デフォルトfalse
gotNotify?: boolean;
};
export const AccountContainer = ({
isUpdate = true,
gotNotify = true,
...props
}: AccountContainerProps) => {
...
}
改善案
前項のように明示的に値を渡してあげるようにしましょう
直列に分散されたコンポーネント
コンポーネントの中にコンポーネントがネストされ続けてるパターンです
一見して何をしているのか分かりづらい上、StateやらHookやら色んな処理が各コンポーネントに分散配置されていることもあります
問題点
- 親からみると子が何をしているのかが分かりづらい
- コンポーネント名が意味をなしていない（責務が別れていない）
- 子コンポーネントが状態を持っているため、親コンポーネントでハンドリングができない
- 子コンポーネントのロジック変更が参照している全コンポーネントのロジックに波及する
一例
親コンポーネント
RegisterHeaderなる物が差し込まれていることだけがわかります
このコンポーネントが何をするのかはパッと見ではよくわかりません
const AccountUpdatePage = () => {
return <Register header={<RegisterHeader />} />;
};
ラッピングしているコンポーネント
ラッパーなのでこのコンポーネントそのものは何をしているのかわかりません
type RegisterProps = {
header: JSX.Element;
};
export const Register = (props: RegisterProps) => {
return <>{props.header}</>;
};
差し込まれているコンポーネントの中身
どうやらRegisterHeaderはRegisterContentを含むようです
なんでヘッダーの中にコンテンツがあるのでしょうか…
export const RegisterHeader = () => {
return (
<>
<p>head</p>
<RegisterContent />
</>
);
};
差し込まれているコンポーネントの子
現在のパスに応じて叩くAPIを変えるような実装がされていますが、もしパスが変わったり増えたりしたらこのコンポーネントの実装を知らない限り面倒なことになります
export const RegisterContent = () => {
const rt = useRouter();
const currentPath = rt.pathname;
const url =
currentPath === 'register'
? 'https://example.com/kaiin/touroku'
: 'https://example.com/kaiin/koshin';
const [username, setUsername] = useState<string | undefined>(undefined);
const onSubmit = (ev: React.FormEvent<HTMLFormElement>) => {
ev.preventDefault();
axios.post(url, {
username,
});
};
return (
<>
<form onSubmit={onSubmit}>
<input
type="text"
value={username}
onChange={(ev) => setUsername(ev.target.value)}
/>
<button>送信</button>
</form>
<RegisterFooter />
</>
);
};
export const RegisterFooter = () => {
return <p>foot</p>;
};
改善案
改善点
- 親から子への見通しが改善された
- コンポーネント名が名前の通りの意味を持つようになった
- ヘッダーはヘッダー、フォームはフォーム、フッターはフッターの責務だけに集中できます
Registerとか言う謎コンポーネントも姿を消しました
- 状態をすべて親に集約した
- 子コンポーネントのロジック変更が起きる確率が減った
- 子コンポーネント側のロジックを減らし、親から渡されるコールバックで行うようにしたため、子コンポーネントのロジック変更が他に影響する確率が減りました
親コンポーネント
- ひとまずヘッダーと入力フォーム、フッターがあるんだなという事が解るようにはなったと思います
- 状態を親に集約したのでAPI叩く時のURIも態々パスから判断しなくて良くなったのでコードの複雑性が減っています
const usePageState = () => {
const [username, setUsername] = useState('');
return {
username,
setUsername,
};
};
const onSubmit = (username: string) => {
axios.post('https://example.com/kaiin/koshin', {
username,
});
};
const AccountUpdatePage = () => {
const ps = usePageState();
return (
<>
<RegisterHeader />
<RegisterForm
onChangeUsername={ps.setUsername}
username={ps.username}
onSubmit={() => onSubmit(ps.username)}
/>
<RegisterFooter />
</>
);
};
ヘッダー
export const RegisterHeader = () => {
return <p>head</p>;
};
入力フォーム
type RegisterFormProps = {
username: string;
onChangeUsername: (value: string) => void;
onSubmit: () => void;
};
const onChangeUsername = (
ev: React.ChangeEvent<HTMLInputElement>,
setState: (value: string) => void
) => {
const value = ev.target.value;
setState(value);
};
const onSubmit = (
ev: React.FormEvent<HTMLFormElement>,
onSubmit: () => void
) => {
ev.preventDefault();
onSubmit();
};
export const RegisterForm = (props: RegisterFormProps) => {
return (
<>
<form onSubmit={(ev) => onSubmit(ev, props.onSubmit)}>
<input
type="text"
value={props.username}
onChange={(ev) => onChangeUsername(ev, props.onChangeUsername)}
/>
<button>送信</button>
</form>
</>
);
};
フッター
export const RegisterFooter = () => {
return <p>foot</p>;
};