freeeの開発情報ポータルサイト

トップ > Advent Calendar 2023 > freeeカードチームの開発(Go)から得た学びベスト5

freeeカードチームの開発(Go)から得た学びベスト5

Advent Calendar 2023 ピックアップ

この記事はfreeeアドベントカレンダー2023の19日目の記事です。

こんにちは！freeeカードチームのmattsunです。freeeカードUnlimitedの開発運用をしています。私は1年前にfreeeに入社しfreeeカードチームに所属しています。これまでの自分のエンジニアとしてのキャリア（10年強）を通してみても、今のチームではPRレビューやリファクタなどからの学びが多いなぁと感じます。個人的に学びがあったことやチームとしての知見が深まったもののうち、ベスト5（私の主観）をまとめます。

freeeカードシステムは、フロントエンド(TS,React)・BFF(RoR)・Backend(Go)で構成されており、Goでの開発比率が多いことから、本記事はGoのコードに関する言及が多いです。freee社全体をみるとRailsで開発されたシステムも多いですが、Goで開発しているサービスもあるのです！ ※ 完全に余談ですが、先日の「Go Conference mini 2023 Winter IN KYOTO」のJobBoardに freeeの名前があって嬉しかったです。

この記事から得られること

  • Goで開発する際に気をつけるポイントが分かる
  • 一般的に、バックエンド開発する際に気をつけるポイントが分かる（第1位、第2位など）
  • freeeカードチームではこのようにチーム内で知見を深めているよって様子がわかる

ベスト5の発表！

それではさっそく、個人的に学びがあったことやチームとしての知見が深まったもののベスト5（私の主観）の発表です！

第5位: error flowの書き方

PRレビュー(引用)

このようにerror flowを変更してはどうでしょう？下記も参考になりそうです。CodeReviewCommentsはGoオフィシャルのガイドラインなので一通り読んでおくと良いです。

CodeReviewComments（日本語）> Indent Error Flow
CodeReviewComments（原文）> Indent Error Flow

// Before
func someFunc() {
    ...
    if err := u.repo.CreateXXX(ctx, xxx); err != nil {
        // 重複エラーの場合はすでにレコードが存在するためエラーを返さない
        if /* RecordNotFoundなエラーのとき */ {
            return nil
        }
        return err
    }
    return nil
}

// After
func someFunc() {
    ...
    err := u.repo.CreateXXX(ctx, xxx);
    if err == nil {
        return nil
    }
    if /* RecordNotFoundなエラーのとき */ {
        return nil // 重複エラーの場合はすでにレコードが存在するためエラーを返さない
    }
    return err
}

感想

PRを出したのは新卒メンバーだったのですが、「オフィシャルなガイドラインを読むと良いよ」って伝わったのでチームとしてとてもいいレビューだったなと思います。この方が読みやすそう（=主観）というコメントではなく、ドキュメントを提示しながら意識が合っていくのがよいなと思いランクインしました。

第4位: ロジックを追加するレイヤーを考えよう

背景・要件

外部から期間を指定してデータ取得できるapiを新規実装しました。 期間(終了日)はYYYY-MM-DDのフォーマットで文字列で指定しますが、期待することとしては終了日までを含む（例：2023-12-24とリクエストがきたら、12/24 23:59:59のデータまでを返却する）ことでした。

freeeカードシステムは、【連載 第4回】freeeカード Unlimited でのClean Architecture実践 でも紹介されているように、クリーンアーキテクチャで実装されています。

大きく分けて、adapter(外部アダプタ層)、domain、usecaseのどの層に実装するのか留意する必要があります。

PRレビュー(引用)

レビュー指摘箇所のコード抜粋

// Before
func (r *DBRepository) FindXXXs(ctx context.Context, ..., from time.Time, to time.Time) ([]*domain.XXX, error) {

    var args []any
    to = to.AddDate(0, 0, 1) // 終了日までを含むという要件を、adapter層で実装していた
    ...
}

レビューコメントは次のとおりでした。

IMO 責務としてadapter層では入力通りに返すようにして、addDateのロジックは入れないようにしましょう。これは、ドメイン知識だと思うのでdomain層にいれるのが適切だと思います。

ここのメソッドの引数はtime.Timeであり、あとから別の箇所からこの関数を呼ぶ実装が増えたときに、誤った動作に繋がると思います。

感想

レイヤーごとどこに実装するかの指針が決まっていても、チームで具体的な実装箇所の認識がずれることがあります。こういう認識がズレやすいポイントを、PRレビューを通してすり合わせていくのが良いコミュニケーションだと感じます。実装する層が異なることによってどんな問題がおきそうかという点がチームでそろうととても強いチームになると思いました。

第3位: panicをinterceptorでhandlingするテクニック

背景・要件

panicがおきたときに、適切にstack traceが取れておらず運用がつらい時がありました。社内ライブラリでのエラーハンドリングのために独自エラーにラップする必要があったので次の通り対応しました。

PRレビュー(というかPR本体)

// PanicUnaryServerInterceptor returns a new unary server interceptor for panic.
// The interceptor wraps a panic object as unexpected error with stack trace when panic occurs.
func PanicUnaryServerInterceptor() grpc.UnaryServerInterceptor {
    return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (res any, err error) {
        defer func() {
            // panicが発生した場合、errortype.Unexpected.Wrapでエラーをラップして、
            // 名前付き戻り値のerrに代入する。
            // そのため、このInterceptorを経由して呼び出されるgRPCのハンドラ内でpanicが発生した場合、
            // panicを送出する代わりにそのpanicの内容とスタックトレースを含むエラーを返すようになる。
            if r := recover(); r != nil {
                var ok bool
                err, ok = r.(error)
                if !ok {
                    err = errors.Newf("%v", r)
                }
                err = errortype.Unexpected.Wrap(err, "panic") // 社内FW独自のエラータイプを指定する
            }
        }()
        return handler(ctx, req)
    }
}

感想

Goのmiddlewareとして、panicのハンドリングやstacktraceをエラーに含める実装を追加しました。こういう処理は、Goだとmiddlewareとして実装することにより、ハンドリング漏れがなくなるよという学びになりました。

第2位: 書籍ASoSDにおけるDeepModuleになってない

背景・要件

"A Philosophy of Software Design"という書籍があり、設計指針について知見をもたらしてくれるとても良い書籍（“A Philosophy of Software Design” を30分でざっと理解する p21 の資料がとても参考になります）なのですが、その中で Module Should be deep と言及されています。

Module Should be deepに対する私の意訳: モジュールは「深く」すべき。つまりIFはシンプルに、内部実装が複雑で細かな挙動まで含んでいるとよい。

PRレビュー(引用)

Condition という構造体に、何でも取得条件を指定できる便利？！なメソッドが追加されていました。

// Before
// FindCards finds cards in accordance with a condition.
func (r *DBRepository) FindCards(ctx context.Context, cond port.CardsCondition) ([]*domain.Card, error) {

    query := "SELECT DISTINCT c.* FROM cards AS c ")

    if cond.CompanyID != 0 {
        query += " c.company_id = ? "
        args = append(args, cond.CompanyID)
    }

    if len(cond.OrderStatuses) > 0 {
        query += fmt.Sprintf(" c.status IN (%s) ", sql.Placeholders(len(cond.OrderStatuses)))
        args = append(args, toInterfaces(cond.OrderStatuses)...)
    }
    // ... 検索条件:Condition のfieldごとの列挙が続く ...

PRレビュー

複数箇所で呼ばれるFindCards()メソッドですが、本当に共通化すべきでしょうか？ ユースケースごとに必要となる粒度ごとに、メソッドを分割したらどうでしょう？ 次の資料の中でも Module Should be deep と言及されていて、IFをシンプルに・実装は深くすべきとありますが、それとは逆の実装方針になってしまっていると思います。 https://speakerdeck.com/iwashi86/understand-roughly-philosophy-of-software-design-in-30-minutes?slide=21

感想

一見共通化されているからよさそうだと思うメソッドでも、 Module Should be deep の観点からみると明確に避けたほうが良いパターンだと分かりました。つまり、複数のユースケースから共通で参照できることよりも、わかりやすくIFがシンプル（conditionのような汎用的すぎるがゆえに複雑な引数を避けた）メソッドのほうがよいと理解できました。本メソッドに限らず、今後の設計指針にも活きそうな観点で学びが深かったです。

第1位: 凝縮度に基づくメソッド設計

背景・要件

外部連携処理を実装する要件において、連携済みかどうかの判定や、認証完了判定のために連携処理中のレコードを取得する必要がありました。

PRレビュー(引用)

// Before
// FindXXXIntegration finds an xxx integration by company id.
func (r *DBRepository) FindXXXIntegration(ctx context.Context, companyID domain.CompanyID, isAuthorized bool) (*domain.XXXIntegration, error) {
    var query
    if isAuthorized {
        query = "SELECT * FROM xxx_integrations WHERE /* 認証済レコードを取得する条件 */ "
    } else { 
        query = "SELECT * FROM xxx_integrations WHERE /* 連携処理中なレコードを取得する条件 */ "
    }

    // ... queryをもとにdbからデータ取得する ... 
}


// After
// bool引数で分岐させるのではなく、メソッドごと{Authorized, Unauthorized}の２つに分ける

// FindAuthorizedXXXIntegration finds an authorized xxx integration by company id.
func (r *DBRepository) FindAuthorizedXXXIntegration(ctx context.Context, companyID domain.CompanyID) (*domain.XXXIntegration, error) {
    const query = "SELECT * FROM xxx_integrations WHERE /* 認証済レコードを取得する条件 */ "
    // ... queryをもとにdbからデータ取得する ... 
}
// FindUnauthorizedXXXIntegration finds an unauthorized xxx integration by company id.
func (r *DBRepository) FindUnauthorizedXXXIntegration(ctx context.Context, companyID domain.CompanyID) (*domain.XXXIntegration, error) {
    //...省略...
}

IMO ここは isAuthorized bool の引数で内部で分岐させるのではなく、明示的に異なるメソッドに分けてはどうですか？ 引数の値を元に内部で処理が分岐するような関数は「論理的凝集」という凝集度が低い状態にあり良くない設計だとされているためです。 参考: 凝集度について
https://zenn.dev/portinc/articles/design-principle-cohesion

別メンバーのコメント

FYI) 「論理的凝縮度」については、これもめっちゃわかりやすかったです
https://speakerdeck.com/moriatsushi/liang-ikodotohahe-ka-enziniaxin-zu-yan-xiu-suraidogong-kai?slide=37

感想

「凝縮度」のような概念があり、それに基づき設計することの大切さをチームで持つことができました。概念だけだと「わかったような・・？」となり実践しにくいと感じることもありますが、実例ベースでこちらのほうが良さそうと判断の指針にできたので、学びでした。

まとめ

以上、freeeカードチームの開発(Go)で得た学びベスト5でした。

  • 第5位: error flowの書き方（CodeReviewCommentsを読もう）
  • 第4位: ロジックを追加するレイヤーを考えよう
  • 第3位: panicをinterceptorでhandlingするテクニック
  • 第2位: 書籍ASoSDにおけるDeepModuleになってない
  • 第1位: 凝縮度に基づくメソッド設計

僕自身はチームにjoinしてから1年経ちますが、freeeカードチームはとても優秀な新卒メンバーから中途入社で経験豊富なメンバー（それぞれの経験や強みも異なる）まで揃っており、日々学びが多くてとても刺激的なチームだと感じます。本記事では周りから受けた刺激や、学びがたくさんあったので、学びベスト5をまとめました。

2023年12月時点で、freeeカードチームではエンジニアを募集しています。本記事から少しでも興味を持った方がいたら、ぜひぜひご応募ください！申し込みいただけたら、カジュアル面談からスタートしますので、話を聞きたいといった方もお気軽にどうぞ。

👉2023年12月時点求人: クレジットカード事業開発エンジニア