このコードを単純化しないでください

 (github.com/kubernetes)
2 ポイント 投稿者 GN⁺ 2024-08-07 | 1件のコメント | WhatsAppで共有
  • Kubernetes の pv_controller.go は PV/PVC バインディング を同期するコントローラーで、ファイル冒頭から「単純化せず、space shuttle style を維持せよ」と明記している
  • このスタイルは、すべての if に対応する else を置き、一見自明な条件でもコメントとして残し、検討済みの分岐と意図 をコード内に示すための方式である
  • 設計の中心は、pvc.Spec.VolumeName と pv.Spec.ClaimRef でつながる 双方向ポインタ であり、トランザクションのない環境で競合・削除・ユーザー修正・同時バインディングを回復可能な形で扱う
  • コントローラーは PV/PVC の変更監視、内部キャッシュ、単一ワーカーキュー、イベント記録、動的プロビジョニング、CSI マイグレーションインターフェースを組み合わせて バインディング状態遷移 を管理する
  • 冗長な分岐とコメントは、動作に関する業務知識と障害回復の文脈を保存するための仕組みであり、今後の変更でも同じスタイルに従う必要がある

pv_controller.go の役割と記述原則

  • pv_controller.go は Kubernetes persistentvolume パッケージの PersistentVolumeController 実装ファイルである
  • このコントローラーは PersistentVolumeClaim と PersistentVolume の状態を整合させる
    • PersistentVolume の変更を監視するキャッシュコントローラー
    • PersistentVolumeClaim の変更を監視するキャッシュコントローラー
    • 2 つのオブジェクトの変更イベントに基づいて PV/PVC の状態を同期
  • ファイル冒頭のコメントは、このコードを 単純化するな と繰り返し警告している
    • スタイル名は space shuttle style
    • すべての if 文に対応する else を置く方式である
    • 単純なエラーチェックを除き、あらゆる分岐を明示することが目的である
    • 自明に見える動作もコメントで残し、保守担当者がバインディングの複雑さを追跡できるようにしている

space shuttle style を維持する理由

  • このコントローラーは、もともと 3 つのコントローラーに分かれていた処理を 1 つに統合した結果生まれたものである
  • PV サブシステムを単純化する過程で、すべての条件をコード上で明示的に扱う方式が必要になった
  • その結果、コードは冗長でコメントや分岐が多く見えることがある
  • この冗長さは バインディング動作に関する業務知識と文脈 をコード内に残すための仕組みである
  • このファイルを変更する際は space shuttle style を維持し、必要に応じて同様の方法で分岐とコメントを追加しなければならない

中核設計: PV と PVC の双方向ポインタ

  • 設計の中心には、PV と PVC の間の 双方向ポインタ がある
    • PVC 側ポインタ: pvc.Spec.VolumeName
    • PV 側ポインタ: pv.Spec.ClaimRef
  • この双方向性はトランザクションのないシステムでは扱いが難しいが、障害時でも正常動作を保証するために必要である
  • rogue HA controller instance が競合状態を作ると、区別できない複数のバインディングが生じ、データ損失の可能性 が生まれる
  • コントローラーは基本的に active-passive 高可用性 モードで動作するよう設計されている
    • オブジェクト遷移は active-active HA でも動作できるように設計されている
    • ただし 2 つの active コントローラーが頻繁に衝突すると性能が低下する可能性がある

バインディング方式と回復条件

  • コントローラーは双方向の pre-bound オブジェクト をサポートする
    • 特定の PV を要求する PVC
    • 特定の PVC のために予約された PV
  • バインディングは 2 段階で進行する
    • まず PV.Spec.ClaimRef を変更する
    • 次に PVC.Spec.VolumeName を変更する
  • この過程のどの時点でも、PV や PVC はユーザーまたは別のコントローラーによって変更・削除される可能性がある
  • 2 つ以上のコントローラーが、異なるボリュームとクレームを同時にバインドしようとする場合もある
  • コントローラーはこのような衝突状況から 回復 できなければならない

コントローラー構造体の主要構成

  • PersistentVolumeController は、PV/PVC 同期に必要な lister、informer sync 関数、Kubernetes クライアント、イベントレコーダー、ボリュームプラグインマネージャーなどを持つ
  • 最後に認識した PV/PVC のバージョンは内部キャッシュに保存される
    • volumes persistentVolumeOrderedIndex
    • claims cache.Store
  • このキャッシュは、API サーバーに保存した最新バージョンと etcd イベントで入ってきたバージョンの両方を反映する
  • 1 回のバインディングでおおむね 4 つのイベントが発生しうる
    • volume.Spec 更新
    • volume.Status 更新
    • claim.Spec 更新
    • claim.Status 更新
  • 内部キャッシュがないと、informer が古い状態を保持しているときに、すでに完了したバインディングを再修正しようとする可能性がある
  • このとき API サーバーに再書き込みを試みると、すでに保存済みのオブジェクトと バージョン競合 が発生しうる

ワークキューと並行性の制約

  • コントローラーは claim と volume の処理用に別々の workqueue を持つ
    • claimQueue
    • volumeQueue
  • 各キューは必ず 1 つだけの worker thread を持つ必要がある
  • 特に syncClaim() は再入可能ではない
  • 2 つの syncClaim() が同時に実行されると、次の問題が起こりうる
    • 異なる 2 つの claim を同じ volume にバインドする
    • 1 つの claim を 2 つの volume にバインドする
  • コントローラーは API サーバーのバージョンエラーと独自の検査でこうした状況から回復できるが、multi-worker 方式は全体速度を低下させる可能性がある

syncClaim: PVC 同期のエントリポイント

  • syncClaim は、claim の作成・更新・定期同期時に呼ばれる主要メソッドである
  • このメソッドはイベント種別を区別しない
  • まず PVC に正しい migration annotation を設定し、必要なら API サーバーに更新する
  • その後、AnnBindCompleted annotation の有無に応じて分岐する
    • annotation がなければ syncUnboundClaim
    • annotation があれば syncBoundClaim
  • 実際の処理は可読性のため unbound claim と bound claim 用メソッドに分けられている

checkVolumeSatisfyClaim: PV 要件の検査

  • checkVolumeSatisfyClaim は、要求された PV が PVC の要件を満たすかどうかを確認する
  • 検査条件はコード内に明示的に列挙されている
    • PV に DeletionTimestamp があればエラー
    • PV 容量が PVC の要求容量より小さければエラー
    • storageClassName が異なればエラー
    • VolumeAttributesClass feature gate が有効なら VolumeAttributesClassName 一致を確認
    • feature gate が無効なのに claim または volume に VolumeAttributesClassName があればエラー
    • volumeMode に互換性がなければエラー
    • access mode に互換性がなければエラー
  • すべての条件を通過すれば nil を返す

遅延バインディング PVC のイベント処理

  • emitEventForUnboundDelayBindingClaim は、遅延バインディングモードの未バインド claim に情報イベントを生成する
  • デフォルトの reason は WaitForFirstConsumer
  • デフォルトメッセージは、最初の consumer が作成されるまでバインディングを待つという内容である
  • その PVC を参照する未スケジュールの Pod があれば、reason は WaitForPodScheduled に変わる
    • Pod が複数ある場合は、すべての Pod 名をメッセージに含める
    • volume scheduling では 1 つの Pod しか考慮されないが、どの Pod が使われるか分からないため全 Pod を含める

syncUnboundClaim: まだバインドされていない PVC の処理

  • claim.Spec.VolumeName が空なら、ユーザーが特定の PV を要求していない状態である
  • この場合、コントローラーは claim の遅延バインディングモードを確認し、findBestMatchForClaim で最適な PV を探す
  • 適切な PV がない場合は、次の順序で処理する
    • デフォルト StorageClass を割り当てられるなら PVC を更新して同期を終了する
    • 遅延バインディングで、まだ provisioning 状態でなければ待機イベントを生成する
    • claim に StorageClass があれば provisionClaim で動的プロビジョニングを試みる
    • それ以外では、利用可能な PV も StorageClass もないことを示す FailedBinding イベントを記録する
  • 適切な PV があれば bind を呼び出して PV と PVC をバインドする
    • 成功時には provision + binding 作業の metric を記録し、timestamp キャッシュを掃除する
    • 保存中にエラーが起きた場合は、後続の syncClaim がバインディングを完了する

特定の PV を要求する PVC の処理

  • claim.Spec.VolumeName が空でなければ、ユーザーが特定の PV を要求している状態である
  • 要求された PV がキャッシュにない場合、PVC 状態を Pending に更新して後で再試行する
  • 要求された PV が存在し、volume.Spec.ClaimRef がない場合、その PV はまだ claim されていない状態である
    • checkVolumeSatisfyClaim で要件を検査する
    • 要件を満たさなければ VolumeMismatch イベントを記録し、PVC を Pending のままにする
    • 要件を満たせば bind を呼び出す
  • 要求された PV がすでにこの PVC に claim されている場合は、bind を呼び出してバインディングを完了する
  • 要求された PV が別の claim に紐付いている場合は次のように処理する
    • claim に controller によるバインディング annotation がなければ FailedBinding イベントを記録し、Pending にしておく
    • controller がバインドしたように見えるのに別 claim に紐付いているなら、「should never happen」状態としてエラーを返す

syncBoundClaim: すでにバインド済みの PVC の処理

  • syncBoundClaim は AnnBindCompleted annotation を持つ PVC を処理する
  • すでにバインド済みの claim なのに claim.Spec.VolumeName が空なら、claim 状態を ClaimLost に変更する
    • イベントメッセージは、bound claim が PV 参照を失い、volume のデータが失われたという内容である
  • claim が指す PV が存在しない場合も同様に ClaimLost に変更する
    • イベントメッセージは、bound claim が PersistentVolume を失い、データが失われたという内容である
  • PV は存在するが volume.Spec.ClaimRef がない場合、volume が unbound 状態になったと見なし、再度 bind を呼び出す
  • PV の ClaimRef.UID が claim の UID と同じなら、正常なバインディング状態とみなして bind を呼び出す
    • ほとんどの場合、これは何もしない呼び出しである
  • PV が別の claimant を指している場合、claim phase を終端状態の Lost に設定する

syncVolume: PV 同期のエントリポイント

  • syncVolume は volume の作成・更新・定期同期時に呼ばれる主要メソッドである
  • イベント種別は区別しない
  • まず PV に正しい migration annotation と finalizer を設定し、必要なら API サーバーに更新する
  • volume.Spec.ClaimRef がなければ未使用の volume とみなし、phase を Available に設定する
  • ClaimRef はあるが UID が空なら、特定の PVC に予約された PV とみなし、phase を Available に設定する
    • その PVC はまだこの PV にバインドされておらず、PVC sync が処理する

claim を見つけられない PV の処理

  • PV が claim にバインドされている場合、コントローラーは ClaimRef の namespace/name で PVC を探す
  • キャッシュで PVC が見つからない場合、特定条件下で追加確認を行う
    • informer cache で再確認
    • API サーバーで再確認
  • 外部 PV provisioner や外部 PV binder が作成した PV では、高負荷時に PVC がまだローカルキャッシュへ同期されていない可能性がある
  • PVC を誤って reclaim しないよう、二重確認 を行う
  • claim が存在しないと判断したら、volume phase を Released に変更し、reclaimVolume を実行する
    • 既存 phase が Failed なら上書きしない
    • reclaim policy が Retain なら、存在しない claim を参照する PV であることをログに残す

PV と PVC の対応がずれた場合

  • claim は存在するが claim.Spec.VolumeName が空なら、PVC はまだ PV 名を持っていない状態である
  • volumeMode が一致しなければ、PV と PVC の両方に VolumeMismatch イベントを記録し、syncClaim をスキップする
  • mismatch でなければ、claim を claimQueue に追加して syncClaim がすぐ呼ばれるようにする
    • この方式は provisioned volume のバインディングを高速化する
  • claim の Spec.VolumeName が現在の volume 名と同じなら、正常バインディングとみなし、volume phase を Bound に更新する
  • claim が別の volume にバインドされている場合は、状況に応じて処理する
    • 動的に provision された volume で reclaim policy が Delete なら、Released としてマークして reclaimVolume を実行する
    • controller がバインドした volume なら unbindVolume で整理する
    • ユーザーが作ったポインタならそのまま残しつつ、unbindVolume を呼んで phase を更新し、ClaimRef.UID を消去する

状態更新とイベント発行

  • updateClaimStatus は PVC status を API サーバーに保存する
    • phase の変更
    • volume がない場合の AccessModes、Capacity、CurrentVolumeAttributesClassName の初期化
    • volume がある場合の access mode、capacity、current volume attributes class 名の更新
  • claim が Bound になる瞬間にだけ capacity を更新する条件がある
    • PVC filesystem size と PV block device size の差異は意図的な場合があるため、すでに bound の claim の capacity を上書きしない
  • VolumeAttributesClass feature gate が有効なら、pending から bound に変わる間に CurrentVolumeAttributesClassName を設定する
    • その後は resizer や admin override が扱うべきで、controller が設定し続けると race condition の可能性がある
  • updateClaimStatusWithEvent と updateVolumePhaseWithEvent は、実際に status/phase が変化した場合にのみイベントを発行する

デフォルト StorageClass の割り当て

  • assignDefaultStorageClass は、claim に storage class がないときデフォルト StorageClass を見つけて割り当てる
  • すでに storage class がある claim は無視する
  • デフォルト class がなければ更新せず false を返す
  • デフォルト class があれば claim.Spec.StorageClassName に class 名を設定し、API サーバーに更新する

ファイルの範囲と明示的な限界

  • GitHub ページに表示されたファイルメタデータによれば、pv_controller.go は 2038 行、1864 LOC、91 KB である
  • 提供された本文には、ファイル前半から bindVolumeToClaim 関数の冒頭までしか含まれておらず、残りは raw view リンクへ続いている
  • したがってこの要約は、提供されたコード本文に現れているコントローラー構造、設計コメント、主要な同期分岐、状態更新ロジックに限定される

関連記事

1件のコメント

 
GN⁺ 2024-08-07
Hacker Newsの意見

  • このファイルのコードが本当に普通の Go コードのように感じられるのは、自分がおかしいのかどうか分からない。Go なので冗長で、深い抽象化に頼っていないぶん長く見えるが、コード自体は典型的に見える。
    抽象化は両刃の剣なので、こういうやり方も悪くないし、冒頭のコメントがなければ、この書き方について二度考えることはなかったと思う。たぶん、システムソフトウェアよりエンタープライズソフトウェアの経験が多いことによる違いかもしれない。Kubernetes に継続的に貢献している人には、これらのコメントは不要に見えるかもしれないが、企業環境で、遠い未来の読者が文脈なしに読むコードだとしたら、この複雑さならむしろもっとコメントを付けていたと思う。

    • 以前はこういうコードが普通に感じられたが、この10年ほどで、多くの人が明示性よりも短さを高く評価するようになった気がする。
      特にこういう重要なコードでは、明示性をはるかに好む。複数の条件をまとめ、業務上の文脈や意味を説明するコメントを省いたコードのせいで、現在の挙動が意図したものなのか偶然なのか判断できなかったことが、キャリアの中で何度もあった。こういうやり方は変更に強いコードではなく、変更を妨げるコードになりがちで、少なくとも作者以外の人が修正しにくくする。不必要なチェスタトンの柵を作るのは保守性に反する。

    • このコメントはおそらく、コードを単純化しようとして失敗したあと、将来のメンテナに同じ試みをする前に考え直すよう警告するために追加されたのだと思う。
      警告を追加したコミットは「Add note about space-shuttle code style」[1]で、その直前のコミットは「Revert controller/volume: simplify sync logic in syncUnboundClaim」[2]だった。

      [1] https://github.com/kubernetes/kubernetes/commit/de4d193d45f6...

      [2] https://github.com/kubernetes/kubernetes/commit/8a1baa4d64ca...

    • 自分も似たように考えていたが、大きくネストした if 文を見て考えが変わった。あの部分は間違いなく早期リターンの分岐にしていたと思う。
      「動くようにし、速くし、きれいにしろ」のうち、最初の段階だけ終えて「きれいにする」をやっていない感じだ。ややこしい状態の相互作用を解きほぐすときに、こういう醜くてコメントの多いコードを書いたことはあるが、普通はレビュー前に少し整理する。もしかすると、ファイルの一番上に「このコードを単純化しようとしないこと」という大きなバナーを貼るだけの方がいいのかもしれない。それでも、確かにものすごく悪いわけではない。

    • 奇妙ではあるかもしれないが、あなた一人ではない。自分にもこのコードは完全に普通に見える。システムの信頼性に重要だと感じるコンポーネントには、こういうコードとコメントを書いたことがある。
      「コメントのないコード」という流行には賛同したことがないし、数カ月後や数年後に戻ってきたとき、自分が書いたコメントが未来の自分にとって非常に貴重だったことがあまりにも多い。この程度の複雑さを持つコンポーネントに埋め込まれたロジックを、しっかりしたコメントなしに再び把握するのは想像しにくい。

    • 特に、すべての if に対応する else コメントがあるという説明は、安定して真だとは言えなさそうだ。対応のない if の多くは単純な if (err != nil) { チェックや別の早期リターンだが、それらを除いても対応のない if があるように見える。
      ただし、エンタープライズソフトウェアの経験上、追加コメントが必ずしも多かったわけでもない。コードベースには // end if コメントが疫病のように存在していたが、実際に説明するコメントはまれだった。

  • Space Shuttle のソフトウェア品質に関する記事: https://archive.is/HX7n4
    抜粋すると、このソフトウェアが驚くべき理由は、どれだけ多くのことをするかではなく、どれだけうまく動作するかにある。決してクラッシュせず、再起動を必要とせず、バグがなく、人間が達成した水準として完璧に近いという。最後の 3 バージョンはそれぞれ 42 万行だったが、エラーは 1 つずつしかなく、最後の 11 バージョン全体のエラーは 17 個だった。同じ複雑さの商用プログラムなら、エラーが5,000個ほどあっただろうという。

    • 「最後の 3 バージョンがそれぞれ 42 万行で、エラーが 1 つずつあった」というのが正確にどういう意味なのか気になる。3 つのバージョンそれぞれにバグが正確に 1 つあったのなら、前の 2 回の修正が機能しなかったか、新しいバグを入れたという話を変に表現しているだけではないのか？
    • NASA のやり方と SpaceX のやり方がどう違うのか比較してみると面白そうだ。SpaceX も有人ミッションを実施しているので、要件はかなり似ているように見える。
    • 5000 / 17 ≈ 295 だ。同じ複雑さの商用プログラムは人時が 295 分の 1だったと仮定しても、公平だろうか？
    • Space Shuttle の開発方法論の問題は、ものすごく高価で遅いにもかかわらず、100% バグなしではないことにある。
      高価で遅すぎるので、現代的な証明支援系（proof assistant）でソフトウェアの正しさを証明する方が、はるかに安く速く、実際にはより安全だろう。seL4 や CompCert のようなプロジェクトが、どうすべきかを示している。
    • 自分が最も好きな記事の一つだ。1996年のインターネット記事がまだアクセス可能だというのは驚きだ。

  • // KEEP THE SPACE SHUTTLE FLYING. という意図は分かるが、安全記録が良くなく、もはや運用されていないシステムをコメントで参照しているのは少し笑える。
    10年ほど後にも、人々は Space Shuttle を良いものとして記憶しているだろうか？

    • Space Shuttle の安全問題はおおむねハードウェアの問題であって、ソフトウェアの問題ではなかった。
      1986年の Challenger 事故報告書に掲載された Richard Feynman の付録「Appendix F - Personal Observations on Reliability of Shuttle」[0] では、こう述べられている。

      要約すると、コンピュータソフトウェアの検査システムと姿勢は最高品質である。Solid Rocket Booster や Space Shuttle Main Engine の安全システムに特徴的に見られる、基準を下げながら徐々に自らを欺いていく過程は見られない。

      彼は航空電子ソフトウェアの品質を、Shuttle のような大規模で複雑な政府プロジェクトであっても適切にエンジニアリングでき、それ自体が低品質や危険を予定されたものではないという例として、特に強調していた。

0: https://www.nasa.gov/history/rogersrep/v2appf.htm

  • 100回をはるかに超える成功ミッションで、人と機材を宇宙へ送り出し、再び地球へ連れ帰った。今でも好意的に見ているし、今後もそうである可能性が高い。人類の進歩と純効果という面では成功だった

  • Shuttleを終わらせたのは悪い安全記録ではなく、コストと将来的な安全性低下の予測だった
    2度のShuttle事故で、他のNASAの災害よりも多くの宇宙飛行士が亡くなったのは事実だが、実際に行われていたことの難易度を考えると、安全記録は本当に驚くべきものだった。コードはかなり良さそうに見える

  • Space Shuttleの状況は、単に安全性が悪かったと言うよりもっと複雑だ。ミッション基準で見ると、他の打ち上げ機より記録は良いほうだ。Shuttleは135回中2回が致命的ミッションで、ソ連時代のSoyuzは66回中2回、SpaceShipTwoはわずか12回の飛行中1回が致命的ミッションという、恐ろしいほど悪い記録だ
    ただしSpace Shuttleは、多くのミッションで必要とされるよりはるかに大きな乗員収容能力を持っていた。ApolloやSoyuzの3人と違って最大8人まで搭乗でき、ソ連/Roscosmos、ESA、CNSAのミッションの大半が完全無人の自律ミッションだったことを考えると、危険にさらされる乗員そのものがいなかった。もしかすると、この比喩はKubernetesによりよく当てはまるかもしれない。高度に設計され、強力で、多目的だが多くの注意を要し、おそらく必要以上に少し使われているシステムだ

  • 乗客マイル当たりという最も一般的な尺度で見ると、Space Shuttleはこれまで作られて飛行した乗り物の中でも最も安全な部類に入る
    子ども時代がまさに1980年代だった立場から率直に言うと、どうして好意的に記憶できないのか分からない。若すぎて、このプログラムとそのすべてのミッション・成果を完全に回顧的にしか見ておらず、現在の民間宇宙請負業者中心の時代の空気に染まった視点しか持っていないのだろうか？

  • Richard HippがSQLiteのコードを航空標準に合わせる話もかなり興味深い: https://corecursive.com/066-sqlite-with-richard-hipp/#testin...

    DO-178B。安全必須の航空製品のための品質標準です... テストでは、結果として得られるバイナリコードの各分岐演算が少なくとも一度は実行され、少なくとも一度は通過される必要があります... 週60時間で1年かかりました... 非常に大きな違いを生みました。その後8〜9年間、事実上バグはありませんでした

  • この部分はTypeScriptコードの網羅性チェックを思い出させる。常に使うようにしている
    https://www.typescriptlang.org/docs/handbook/2/narrowing.htm...

    • より新しいsatisfies neverがこの用途にとても良い。好みでif elseチェーンを使う場合にも便利

    • ts-patternが気に入るかもしれない

      https://github.com/gvergnaud/ts-pattern

  • 完全に些細とは言えないifごとに明示的なelseを付ける場合だけを考えると、Kubernetesの作者たちがif/elseブロックではなく構造的パターンマッチングを中心に設計していたら、このコードはどれほど単純になっていただろうかと思う
    構造的パターンマッチングをサポートする複数の主流言語には、マッチングが網羅的かどうかをコンパイル時に確認するツールがあり、それだけでもコードの情報密度を高めつつ慣用的な解法になり得る

  • 2018年の議論: https://news.ycombinator.com/item?id=18772873

  • コードをざっと眺めただけだが、正直そこまで悪くは見えない。自分なら違うやり方にした部分はあるだろうが、もっとひどいコードはたくさん見てきた
    少なくともこのコードは1つのルールに従っており、すべてが考えられて書かれていて、この混沌にもそれなりの方法があるという印象を受ける。何度も見てきたスタイルの混在、怠惰なコーディング、非論理的な構造といった典型的な寄せ集めより、こういうコードをいつでも選ぶ

  • なぜ「安全」慣行を新たに作りながら、文書化されたソフトウェア工学のベストプラクティスは無視するのだろうか
    2,000行のモジュールや200行のメソッド、3〜4段階のifのネストは有害だと見なされている。なぜではなく何をしているかだけを述べるコメントも有用ではなく、実際のコードとずれやすい。不要なnilの使用も見える。結合度や単一責任原則のようなより深い問題に入らなくても、表面的にこうした点が見える

    • こうしたものが有害だと思うなら、「John Carmack on Inlined Code」を読むことを勧める
      http://number-none.com/blow/john_carmack_on_inlined_code.htm...

      「Armadilloロケットの飛行制御コードは数千行しかなかったので、メインのtic関数をつかんで、すべてのサブルーチンをインライン化し始めた。実際の墜落を引き起こし得る隠れたバグを見つけたとは言えないが、何度も設定される変数がいくつかと、少し怪しそうな制御フローをいくつか見つけ、最終的なコードはより小さく、よりきれいになった」

      Carmackがこのアプローチに価値を見いだしたのなら、早まって無視すべきではないと思う。後続のコメントも読む価値がある

      「この記事を書いてから数年の間に、C/C++でも合理的な範囲では純粋関数型プログラミングにずっと前向きになった... 手に負えなくなってきたら、ブロックを純粋関数として切り出す方法を探せ」

    • ときには「他に方法がない(TM)」場合がある
      任意の行数制限は不要な断片化を生みやすい。include、ライセンス、接着コード、コメントまで加わると、近づきにくいスパゲッティになる。高性能コードでメソッドを200行に保とうとしてみれば、性能がイカロスの飛行のように墜落することがある

コードのコメントを読んでみると、このコードを単一モジュールに単純化し、アクセスしやすく、さらに重要なことに持続可能にするために、膨大なノウハウが注ぎ込まれていることが分かる。言語やロジックを知らない人にとっては、コードが何をしているのかの輪郭を示してくれるコメントは非常に有用。6か月後には自分のコードでさえ見慣れないものになるので、自分自身にとっても有用。

コメントはコードとコードベースの一部である。周辺のコードを直しながらコメントを一緒に更新しなければ、コードにドキュメントのバグを入れることになる。コンパイラが処理しないからといって、機能的な部分ではないわけではない。本質的にコメントは知識であり、コードの中に埋め込まれた研究ノートであり、書いたコードを保守するときには、実行されるコードより価値がある場合もある。

ベストプラクティスは法律や厳格なルールではなく、指針である。コードベースに合うときに適用すべきで、盲目的に従って問題のあるコードベースを作ってはいけない。時にはルールを曲げ、自分で作る必要があり、自分が何をしているか分かっているなら、それはまったく受け入れられる。

  • かなり長い間このような「安全な」書き方をしてきたが、早期リターンによる鉄道指向のエラー処理よりも、はるかに多くのバグを生み、修正にもはるかに時間がかかった
    すべての if ブロックに明示的な else を付けると、現在のコンテキストを覚えておかなければならない複雑さが爆発する。このルールは「すべての if 条件ブロックは早期リターンするか、対応する else ブロックを持つ」に変えるのが妥当だと思う。if (cond) { 特別処理 } パターンは、早期リターンより確実にはるかに危険で、推論しにくくする。

  • 唯一の公式なベストプラクティス集のようなものは存在しない
    関数の長さやファイル内のコード行数それ自体が、本質的に有害だったり有益だったりするわけでもない。言語ごとにコードをどう構成すべきかという考え方はあるが、そのどれも「最善の慣行」だと主張することはできない。Go は、コードをたくさんの小さなファイルに分割する方式を好む言語ではない。

  • 200行のメソッドが本質的に間違っているわけではない。内部コードが線形で、同じ抽象化レベルを保っているなら、最善の選択になり得る
    代替として5行のメソッドを40個作るのは、もっと悪い場合がある。全体を理解するにはあちこち飛び回らなければならず、呼び出し順を壊してしまうこともある。選択可能な順列は 40! 個もある。

  • このようなコードは、宣言的・ルールベース・テーブル駆動システムへ移すのに理想的な候補に見える
    その方式のほうが、場当たり的な命令型の if 句だらけのコードより理解しやすく、検証しやすい。この種の雑然としたコードは、たいてい欠けている抽象化があるという兆候である。

    • Go の思想は基本的に、C で書いただろうコードをある程度そのまま移すようにすべて書き下し、何かを抽象化しようとしないことに近い。