バックエンドの設計で直したほうが良いコード9選
バックエンド兼インフラエンジニアのrevenue-hackです！
今回は今までバックエンドエンジニア10年くらいやってきて、「これはまずいなー」と思ったコードについて紹介していきます。
最初に大事なことを言っておくと、あくまであまりやらないほうが良いというだけであって、デメリットを理解した上でやる分には問題ないです！
結論、アーキテクチャはそのプロダクトによって様々なのでレイヤードアーキテクチャのチームもあれば、クリーンアーキテクチャで実装しているチームもあり、大事なのはちゃんと意思統一ができていて、コードでもその意思統一がしやすい状態ということです。
↓mentaで設計やバックエンド、インフラ、RDBに関して教えていますー↓
想定読者
- 初心者〜エンジニア歴3年未満の人向け
- バックエンド以外のフロントエンドとかでも読んでも為になるかも
- 使っている言語は特に問わない
バックエンドの良くない設計: 困ったらときの関数置き場になっているutil
まずは色々な実装で使われそうな関数群をまとめたutilですね。
utilなどをちゃんと管理して運用できている会社を見たことがないですｗ
何故だめかというと、色々な実装で使われそうなものをまとめているだけあって、ほぼ無秩序状態で、とりあえず困ったらutilに入れておくというケースが多いからです!
結果的にfatしてきて古参の人がいなくなり、そもそも存在すら把握できない関数だらけになってしまうという落ちです。。。
例えばよくあるのは
- パスワードのハッシュ化するための関数
- 文字列の長さチェック、空文字チェックなどのバリデーション系
とかでしょうか。
無秩序状態の何が行けないかというと
- とりあえずどこに書くかわからなくなったらutilに入れて、どんどんfatしてくる
- 実装者の感覚で入れられるので正解がないため、レビュー時は無視されがち
- 1箇所でしか使ってない関数だらけになる
- リファクタで全く使わないのに(いつか使うかもしれないから？)放置される
- utilを管理していたエンジニアもいなくなり、fatしてきて誰も存在をしらないため、実はutilに同様に処理があるのに使われない
ということが起こります。
**対策としてはそもそもutilを作るのをやめましょう！＊＊
そうすることでとりあえず困ったらutilへという思考が脳内から消え去り、ちゃんと設計と向き合えるようになります！
どうしても作りたい場合は、局所的な層でのみ使用できるutilとかにとどめておくとまだ管理は楽かと思います！
バックエンドの良くない設計: とりあえず状態を管理するクラスにgetter, setter導入
状態管理しているクラスで全フィールドにgetter, setterを入れたり、プロパティでgetterのみのreadonlyにしなかったりしてるあれです！
例えばJavaとかでたまに見かけるとりあえずgetter, setterのlombok導入とかがまさにそうです！
import lombok.Getter;
import lombok.Setter;
// 全フィールドにgetter, setterが付与される
@Getter
@Setter
public class User {
private long id;
private String name;
}
何故だめかというと、せっかくクラス作ってカプセル化して、状態を制御しようとしてるのに、setterがあると、いつでもどこからでも変更できるし、変更して良いよという意図を伝える実装になってしまうからです！
対策としては、ちゃんとカプセル化しようぜって話になります！
例えば上記のUserの例でいうと
import lombok.Getter;
import lombok.Setter;
public class User {
@Getter
private long id;
@Getter
@Setter
private String name;
// コンストラクタ
}
のように、idはUserオブジェクトが生成されたら変更されることはないから、getterのみ付与して、ユーザ更新ユースケースなどで変更されるnameにのみsetterを付与するという感じに実装すれば良いです！
他のエンジニアにもidは更新されるものではないという意図も伝わりますし、プログラム的にsetterがないので、idにおいてはイミュータブルになります。
バックエンドの良くない設計: ドメインモデル貧血症
1個前の内容にも少し関連しています。
ちなみに「ドメインモデル貧血症」はマーチン・ファウラーが命名した名前です。
ドメインが層から別の層へただTransterするための箱(DTO)になってしまっているという状態です。
これは先程説明したgetter, setterの話にもかなり親和のある話になります。
例えばID必須、ユーザ名必須,100文字以内のドメインルールをもつUserドメインがあったとして
Goで書くとドメインモデル貧血症のドメインはこんな感じ↓
type User struct {
ID string
Name string
}
ドメインルールが全く何も書かれていないのがわかります。
これだとID、名前必須、100文字以内というルールはユースケース側などでチェックする必要が出てきます！
これがドメインにドメインルールが書かれておらず、知識が漏れ出している(貧血になっている状態)ということからドメイン貧血症と言われています!
なのでちゃんとドメインルールをドメインに書いてカプセル化しよう！！
というのが対策になります！
貧血症でない例を書くとこんな感じ↓
type User struct {
id string
name string
}
func NewUser(id, name string) (*User, error) {
// idは空ではないか？idとして適切な文字列か？バリデーション
// nameは空ではないか？100文字以内か？バリデーション
return &User {id:id, name:name}, nil
}
func (u *User) ID() string {
return u.id
}
func (u *User) Name() string {
return u.name
}
こんな感じです(ValueObjectとかは今回は無視で)
こうすることでちゃんとドメインにドメインルールがカプセル化できている状態となり、どこユースケースからUserドメインを使っても、ちゃんとドメインルールを担保したUserができるというわけです！
バックエンドの良くない設計: 責務を複数持ちすぎクラス
責務を複数持ちすぎて、色々と役割を担いすぎているクラスがそれに当たります。
よくあるのがフルスタックのMVCフレームワークのコントローラーにデータ取得処理も実装しているようなケースですね。
RailsやLaravelとかによく見ます。
例えば
<?php
class AController
{
public api()
{
$a = AModel::where('active', 1)->get();
return $a;
}
}
みたいな感じでModelでの取得実装を直接書いてあるケースです。
Controllerとしてはリクエストとレスポンスの処理を基本的に行うのが責務なので、良くないコードです。
それ以外の責務を任せてしまうとテストも書きづらくなります。
後は神クラスと呼ばれるものとかもまさにこれの良くない例です！
対策は単純で「ちゃんと責務分離しましょう」ということになります！
バックエンドの責務分離に関してはこのMENTAのプランで色々と教えています
秘伝のタレ化して触れない神クラス
1個前の内容に近いですが、超複雑な処理や、抽象度の高いコードがまさに神クラスです。
僕が以前出くわしたのは、configに設定値を入れると勝手にテーブルのCRUDのシステム(テーブルへのデータ制御が画面から出来るようになるシステム)ができるという代物でした。
実装としてはcrud.phpとconfigure.phpという2つのファイルが有り、configure.phpに様々な設定値を書きます。
例えば
- 使うテーブル名
- CRUDのどの部分のページを作るのか
- テーブルのカラムごとのデータ型を設定
などを設定します。
作ったページにアクセスするとcrud.phpがconfigure.phpを読み込みテーブルのCRUDのページが表示されます。
様々な設定ができるような汎用的なシステムになっているため、crud.phpはかなり実装が複雑で、抽象度が高い実装になっているため、かなりデバッグに時間がかかります。
そしてもうそれを作った人はいないという状況。。。
もはや触ることが出来ない秘伝のタレ状態でした。。。
(もちろんテストもありません。というかテストの書きようがなさそうな感じでした)
このようなシステムの実装は設定値のみで簡単にできますが、イレギュラーなことをしようとすると、もうどうしようもなくなります。
対策としてはこのような神クラスは作るのやめて、ちゃんと責務を考えながら設計していくと良いです！
commonと言う名のすぐにcommonじゃなくなるファイル(ディレクトリ)
よくcommon.cssやcommon_controller、commonディレクトリというのをみたりします。
(もうSPAが主流になってあまり目にする機会も減りましたが)
名前の通り共通で使うデザインや、共通で使う処理などをおいておく場所として作られている訳ですが、これもほぼすべての会社でうまくいっているのを見たことがないです。
理由としては
- 運用と共に、commonではなくなってくる
- util同様に共通で使いそうだったら、ここに実装され、fatしていく
- fatしてきて誰も管理できなくなる
などになります。
対策はutil同様になります。
エラーの握りつぶし
これはやめましょう！
エラーの握りつぶすと運用でエラーの発見が遅れたり、デバッグでもわからなくなってしまうということが起きてしまいます。
エラーの握りつぶしはやめましょう！
とりあえず何でも共通化する設計
盲目的に同じ処理は共通化するというような設計にしていると、
変な依存が生まれてメンテナンス性が悪くなったり、設計を崩さないといけなくなったりと言うことが起こります。
偶然同じようなコードになったからと言って共通化するのではなく、共通化するときは、同様の責務であるべきかどうかを考えて共通化しましょう。
バックエンドの設計に関してはこのMENTAのプランで色々と教えています
継承ミス(名前とやってることがあってなかったり、ポリモーフィズムでない)
Railsとかで見るこんな感じとかですね。
class BaseController
before_action :authenticate
// 認証ありのエンドポイント
class UserController < BaseController
// 認証不要のエンドポイントはBaseControllerが使えない
class ArticleController < NotAuthorizedBaseController
ArticleControllerは認証の必要のないAPIのためBaseControllerが使えないです。
なので別のNotAuthorizedBaseControllerを作って継承する感じ。。
いかにもBaseControllerというと全Controllerの「基盤」的なControllerだと思いきや、
名前からは考えつかない認証までも含めたControllerになっているため、クラス名を変えたり、そもそもBaseControllerに認証を置かないようにする必要があります。
後はポリモーフィズムでない継承も見かけます（上の例もポリモーフィズムでないと言われればそうですが）。
※以下はイメージを伝えるためのコードなのでシンタックスもあっていないと思う
class AnimalController {
public swim() {
return "泳ぐ"
}
public eat() {
return "食べる"
}
public fly() {
return "飛ぶ"
}
}
class CrowController extends AnimalController {
public swim() {
return "泳げない"
}
}
class WhaleController extends AnimalController {
public fly() {
return "飛べない"
}
}
「カラスは泳げません、クジラは飛べません」
これではうまくis-aの関係が構築できていないため、継承が正しく使えていないです。
対策としては継承よりコンポジションが良いです！
is-a(継承)の関係ではなくhas-a(合成)の関係にするということです。
意味の無さすぎるコメント
// ユーザ情報を取得します
userRepository.GetById(id)
見ればわかることをコメントに書く必要はないですね。。。
このあたりはリーダブルコードを見ると良いですね！
端的にコメントでは
- コードではわからない背景の情報を書くこと
- パフォーマンスで考慮すべきこと
などを書くと良いです！
詳しくはこちらのリーダブルコードのまとめ記事を見ると良いです！
バックエンドの設計は客観的に見て後々困らないかを考えよう
客観的に見て後々困らないかを考えて設計していきましょう。
個人的には一番コードを考える時に意識するのは「自分がいなくても意図がわかるコードになっているか」ということです！
また最初の再掲ですが、今回紹介したコードはものによっては、ちゃんとデメリットを理解した上で使うと良いです！
↓mentaで設計やバックエンド、インフラ、RDBに関して教えています、興味があれば気軽にDMください！
Discussion