はじめに

Salesforceのカスタマイズ性はとても高く、フローや自動計算式を巧みに使って自動化をしている方も多いかと思います。しかし、それだけでは現在の業務フローの仕様を満たせない、もっと複雑な処理をするためにフローでは複雑すぎて開発できない、そのようなフェーズにきて初めてApexを触り始める方もまた多いと思います。

今までもフローでifや繰り返し処理はやってきたし、エクセルでの数式もいくらでも書いてきた。それをプログラムに置き換えるだけなので記述方法さえわかってしまえば自分でも複雑な開発できるようになる！と思っていると、思わぬ罠にはまり、結果ガバナ制限に引っかかりまくったり、どこで何をやっているのかわからなくなって手が出せなくなることになります。

これはApexでの開発をしていた非エンジニアの方(Iさんとします)の退職に伴い、エンジニアである私がその引き継ぎを行った時の状況の共有と、皆さんに同じことをするとどのような恐ろしいことが起きるのかということを知ってもらうために書いた記事です。

非エンジニアの方には、難しいことが多いと思います。
しかし、その難しいことをしないでApexを触ることは、将来の自分を苦しめることになり、またSalesforce自体を機能不全に陥らせ、会社に不利益を与えることになります。

どうか「非エンジニアだから分からなくても仕方がない」では終わらせずに、エンジニアの仲間入りをしてApexの開発を行ってください。そうすればあなたのスキルはみるみるあがり、キャリアの道もひらけていくでしょう。

バージョン管理をしていない

エンジニア間ではgitを使ってバージョン管理をするのが一般的です。
しかし、非エンジニアであったIさんは、gitを触ったことがなかったのでそのまま開発を続けました。
その結果、何が起きたのか？

自分が書いたコードが他の人に上書きされたり、ある機能の以前の状態が分からなくなった

特に開発初期は開発会社も入り、複数人での開発がされていたようでした。その時に、コードが上書きされて再度書き直すということがあったようです。
これも、バージョン管理をしていれば

上書きをされても、上書き前に戻すことができる
ある機能の状態は全て過去に遡ることができるので前の状態を調べることも戻すこともできる

さらに、複数人であれば同じファイルを触ってしまうこともあるでしょう。その結果、同じ箇所を編集してどちらを優先すべきか？という問題が発生します。これをコンフリクトと呼ぶのですが、gitにはそれを解消する機能があります。

一人で使っても、複数人で使っても、大きなメリットがあるのがバージョン管理です。
gitは難しそう・・・と思っていても、とりあえず触ってみて基本的なコマンドを繰り返し使っているうちに徐々に馴染んできます。
そして、何か困ったときにこのバージョン管理をしていたことに助けられるはずです。

バージョン管理は必ず行いましょう。

全てのファイルがルートディレクトリ配下にあり、何かしらのグルーピングは一切されていない

salesforceのApexではnamespaceという概念がないため、フォルダに入れても入れなくても全て同じ階層にいるものとみなされて実行されます。
そのため、sandboxなどからプログラムを全て落としてくると「全てがルートディレクトリに配置された状態」になってしまうのです。

それで何が困るのか？

開発初期は良いが、機能が増えてくるにつれて調べたい処理が書いてあるクラスを探すのに時間がかかるようになる

例えば、

商談に関する何らかの処理を行うクラスが5個
他のオブジェクトの何らかの処理を行うクラスが30個
あったとします。

商談に関するクラスのうち、1つのクラスの処理を確認しようとした時に35個のクラスが同じ場所に集結していると、名前から探すのが大変になります。
クラス名の頭にOppという名前をつけるという方法もありますが、salesforceではファイル名の上限があるためこれはお勧めしません。もし今後長いオブジェクト名ができた時に破綻してしまいます。
略称を入れるという案もありますが、この記事の後半でもお伝えしますが略称がパッと見て理解できるものでないと結局探すのが大変になります。
他にもクラス名が必ずオブジェクト名から始まるとより良いクラス名があるのにこのルールのせいで命名が制限されることもあります。

これらを解消するためにディレクトリを分けるのです。
商談（Opportunity)であればOpportunityフォルダを作りその中に商談に関するクラスをまとめておくのです。
そうすれば商談に関する処理を見たければまずはOpportunityフォルダを見る、そして5つのうちから選ぶ、となり判断する回数がグッと減ります。

そして、ディレクトリに分けた情報はsandboxへあげると失われてしまうのでgitを介して他の人と共有をします。

クラスはそれぞれの役割や機能ごとに分類する。
機能が増えれば増えるほど、分類は細かく行う。

テストはデプロイ基準のコードカバレッジ75%を到達するために書かれているが、ただコードを通るだけで何の検証もされていない

これは、すでに開発をしている非エンジニアの人にとってはかなり耳がいたい話題だと思います。
すでに退職されたIさんは、業務フローをsalesforceで開発するにあたり、スピードを重視し、開発を頑張り、sandbox上で手で動作確認をし、リリースの際にコードカバレッジが75%以上必要なので最低限コードをなぞるように処理を走らせ、コードカバレッジを見たしたのでリリースしていました。

これを繰り返すと、何が起きるのか？

機能が増えるにつれて、手でのデバッグが追いつかなくなる
機能改修で以前の機能にバグが出てその修正に追われる

次のリンクは、Salesforce本体の主席エンジニアの方のブログです。Apexに関わる様々なトピックを発信しています。

この中で彼は

Salesforce implicitly (and explicitly in its Trailhead articles) encourages Test Driven Development (or TDD).

「暗黙的にsalesforceではテスト駆動開発を推奨している」と言っています。
つまり、Iさんはsalesforceでは非推奨な体制での開発を行っていたのです。

これに関して、私は既存機能についてはリファクタリングとテストケースの充実を図りつつ、新規機能については必ず十分なテストケースを書いています。
もはやコードカバレッジなど気にしていません。なぜなら、ちゃんとしたテストを書いていれば75%は簡単に超えるからです。

テストコードを書く技術は、開発スキルとはまた違ったものになるので習得するのに時間がかかります。
しかし、テストコードを充実させようとすると自然とクラスが洗練されたものになっていきます。
その結果、後で見直した時に読みやすく、デバッグの手間を減らし、機能を改修しても機能退行（デグレ）をしなくなり、継続的な機能開発が可能になります。

テストコードはコードカバレッジを満たすためだけに書いてはいけません。

未来のあなたを守るために、テストコードを書きましょう。

マジックナンバーが多用されている

このようなコードを書いていませんか？

if(opportunity.SaleStyle__c == '1') {
  // 何らかの処理
}

これを見ると、おそらく商談の販売スタイルが1の時に処理を行うということがわかります。
では、1とは何ですか？

Iさんはこのようなコードを書いていました。
私はこれを見つける度に、

salesforceのオブジェクトマネージャーを開き
商談オブジェクト検索し
商談オブジェクトの項目から、SaleStyle__cを開き
その中の選択肢で 1 に該当するものが何かを確認する

ということをしていました。これを非効率と呼ばずになんと呼べば良いでしょうか？
この1のような、それ自体が何の意味かわからないものを「マジックナンバー」と呼びます。

これを解消するためには次のようなクラスを作成して参照するのが良いでしょう。

OpportunityTypes

// 販売スタイル  ← 項目の参照名を入れるとわかりやすい
// SaleStyle__c  ← 項目のAPI参照名を入れておくと使う時に調べなくて済む
// https://forcecom ~~~~  ← 項目のURLを入れておくといざという時にすぐアクセスできて便利
public with sharing class OppSaleStyle {
  // 直販
  public static final String DIRECT = '1';
  // 代理店経由
  public static final String VIA_AGENCY = '2';
}

if(opportunity.SaleStyle__c == OppSaleStyle.DIRECT) {
  // 何らかの処理
}

1は実は直販を表していたのです。これでif文を見た時も悩まなくてすみます。
また、ケースとしては少ないかもしれませんが1の意味合いが変わった時に、クラスを別に作って参照していればDIRECT = '1'を変えるだけで済みますが、もしマジックナンバーのままだと、コードの名から直販の意味で1を使っているところを全て探し出して直す必要があります。あまりにも現実的ではありませんね。

マジックナンバーは使ってはいけません！

必ず何らかの意味を持たせられるようにクラスを作り、それを参照する

変数名が略称になっているが非常にわかりづらい

if(sls != null) {
  // なんらかの処理
}

さて、このslsとはなんでしょうか？
実は「売上(sales)」の略称として使われていました。あなたはわかりましたか？

変数名を省略する動機として、

キーボードのタイプ数を少なくする
変数名の打ち間違い（タイポ）を減らす

というのがありますが、現代のIDE、ことApexに至ってはVSCodeが推奨されていますが変数名の候補ぐらい出してくれるので上記の動機で変数名を意味の通じにくい略称にするのは適切でないです。
これのようなスッと理解しづらい変数名を使用していると何が起きるのでしょうか？

プログラムを読む「認知負荷」が高まり、本来一番理解するべきコアロジックに集中することができない

エンジニアはコードを書く時間よりも読む時間のほうが圧倒的に多いそうです。
ならば、読む作業を効率化するためにすぐに理解できる変数名にするのが最適です。
したがって、今回のケースでいえば正直にsalesという変数名を使うべきでしょう。

タイポを減らしたりキーボードのタイプ数を少なくするために、意味の分かりづらい略称を使わない

略称を使う場合は、意味の通りやすい略称を使う。

社内チャットでよく使うもの(workflow → WF）
頭の文字ですぐ何かわかるもの(opportunity → opp）
一般的に使われるもの(temporary → tmp）

未来のあなたが変数名で困らないように、適切な略称を使いましょう！

N+1がそこらじゅうで発生している

非エンジニアのとっては馴染みのない言葉かと思います。
Webエンジニアにとっては非常に起きやすいので常に注意を払っている例のアイツです。
知らないと何もどのような時に起きてどのような影響があるのかが分かりやすいですよね？
では、これがsalesforceで発生するとsalesforceがどうなるのかというと

複数のオブジェクトの更新などをかけた時に、即ガバナ制限に到達する

N+1をchatGPTさんに解説してもらいました。

🍽️ 食堂の注文に例える
あなたは食堂の店員です。お客さんが「このテーブルにいる10人分の料理をください」と言いました。

❌ ダメな方法（N+1問題）
あなたは1人ずつ「何を注文しますか？」と聞いて、1つずつ料理を作ります。

1人目に聞く → 料理を作る
2人目に聞く → 料理を作る
…
10人目に聞く → 料理を作る
➡️ 10回の注文処理が発生し、厨房も大変！

✅ 良い方法（まとめて取得）
最初に「このテーブル全員分の注文をまとめてください」と聞いて、一気に厨房に伝えます。

1回の注文で10人分の料理を作る
➡️ 1回の処理で済むので効率的！

N+1が起きている時に、10人のテーブルが 10個あると、 10*10　で 100回の注文処理が必要ですが、N+1が起きていない状態では10回の注文処理で済みます。

salesforceではこの注文処理に上限が決まっている（= SOQLの発行数の100回)ので、注文処理をいかに減らすかが大事になります

次のようなコードがN+1を発生させる例です。

List<Account> accounts = [SELECT Id, Name FROM Account];
for (Account acc : accounts) {
    // Accountの数だけselectを発行してしまいN+1が発生
    List<Contact> contacts = [SELECT Id, Name FROM Contact WHERE AccountId = :acc.Id];
}

これをN+1を発生させないようにするには一旦まとめて取得して、AccountのIDをキーに保存して、取り出します。

// 1. Accountを取得
List<Account> accounts = [SELECT Id, Name FROM Account];

// 2. AccountIdごとに関連するContactをMapに格納
Map<Id, List<Contact>> accountIdToContactsMap = new Map<Id, List<Contact>>();
List<Contact> contacts = [SELECT Id, Name, AccountId FROM Contact WHERE AccountId IN :accounts];
for (Contact c : contacts) {
    if (!accountIdToContactsMap.containsKey(c.AccountId)) {
        accountIdToContactsMap.put(c.AccountId, new List<Contact>());
    }
    accountIdToContactsMap.get(c.AccountId).add(c);
}

// 3. Accountごとの関連Contactを取得
for (Account acc : accounts) {
    List<Contact> relatedContacts = accountIdToContactsMap.get(acc.Id);
    System.debug('Account: ' + acc.Name + ' has contacts: ' + relatedContacts);
}

もしAccountが200件あった場合、

N+1が発生するコード: 1 + 200 = 201 回のSOQL発行でガバナ制限に引っかかる
N+1が発生しないコード: 1 + 1 = 2 回のSOQL発行でガバナ制限に引っかからない

となります。ただし、メモリを多く使用するのでまた別のガバナ制限に引っかからないかのチェックは必要です。

N+1が発生しないように常に気を配る。特に次のような場合に起こりやすい

処理する箇所が離れている
- N+1に気を配ることができなくなるから
更新トリガーが1件ずつ動いている
- 何らかの処理でレコードを1件ずつ更新するが、実はそのレコードのトリガーでSOQL発行をしていたため結果的にN+1になっているパターン。コードのつながりがないので非常に分かりにくく、発生しやすい。

トリガーコントロールができていない

Iさんがその時は良しとしてトリガーに機能を追加した結果、その悲劇は起きました。
更新処理がうまくいかないのです。その原因が、
あるオブジェクトの更新→トリガーで別のオブジェクトの更新→トリガーでまた別のオブジェクトの更新→ ...
と続いていき最後はガバナ制限で落ちていたのです。

開発時にはそれぞれのトリガーが単独でちゃんと機能するように考えていたのでしょうが、その先のトリガーが数珠繋ぎに発動するところまでは考慮できなかったのだと思われます。
ここまで行くともはや「手遅れ」と言っても過言ではありません。一つ前のN+1も同時に発症しているので、一発ので解決は無理なので少しずつ解消していくしかありません。
トリガーの処理を見て、他のオブジェクトにどのような処理を行い、そのオブジェクトのトリガーが動いても問題ないかどうかを一つ一つ確認していくのです。

気軽にトリガー内でinsertやupdateを行わない。
行う場合は、その先で連鎖的に動くものも十分に確認してすること

UtilクラスやServiceクラスやDaoクラスに処理が集められていて非常に凝集度が低い

一見共通の処理を置いているので問題がないのでは？と思われがちですがこのようなクラスは

処理を次々に追加していくのでクラスが無駄にデカくなってしまう
同じクラスを触るケースが多くなるのでコンフリクトを起こしやすくなる
処理を変更したら、実は別のところで使っていてそっちでバグが起きた
テストケースがとにかく書きづらい。１メソッドごとに使う側の前提条件を理解する必要がある
微妙に意味合いの異なる、似たコードの処理が増える
- フラグ変数を引数に入れて処理を分岐させよう！とすると途端に複雑さが増してものすごく使いづらいく理解しづらいものへ変貌を遂げる

まぁデメリットを挙げるとキリがありませんが、要は「一昔前の使われなくなった設計」だと思ってください。
その一昔前では、データを保持して自身は何も処理しないデータクラスと処理だけをまとめたサービスクラスで役割分担をさせていたようですが結局使いづらく今のオブジェクト指向に移っていきました（という私の理解）
オブジェクト指向では、1つのクラスの中にデータと処理を全てまとめてしまい一つの部品として完成させて、その部品を組み合わせて複雑な処理を組み立てていきます。

この本が有名ですが、有名ゆえにかなり分かりやすいです。具体的なバッドケースも載っているのでApexに慣れ始めた頃に読むといかに自分が悪魔を呼び寄せるコードを書いていたのかがよく理解できると思います。（退職したIさんに送りつけたいぐらい）

Util、Service と名のつくクラスは悪魔を呼び寄せるので作らない！

オブジェクト指向でクラスを設計する
（テストも書きやすくなるよ！）

if文のネストが非常に深く、分岐が多い

こんな処理が残っていました。

if (なんかの条件) {
  // 何もしない
} else {
  // 何からの処理が続く
  ...
  ...
}

最初の「何もしない」には一体何の意味が？？？？

早期リターンをすればelse以降のネストが一つ減ります。
ネストが減るとコードを読む時に覚えておくことが一つ減るので、コアロジックに集中しやすくなります。

ネストを無意味に深くしない。認知負荷が高まりコードの素早い理解を妨げる

早期リターンやFactoryパターンなどを使い、ネストを浅くする。

interfaceやtry-catchを全く使えていない

個人的にはApex中級者からの概念だと思います。
特に、salesforceは業務フローがわかっていてエクセルとかの数式が得意な非エンジニアがなりがちだと推測しており、そのような人たちは変数や繰り返しやifはすんなり理解できて実装できますが、interfaceやtry-catchとなると難しいようです。
Iさんの残したコードを見ているとバグとして残っていたり、チャレンジしたけど出来なくてコメントアウトされたりしていて、結局正常に動いているものはありませんでした。

interfaceはfactoryパターンと組み合わせることでif文の削減に貢献したり処理をクラスに閉じ込めるオブジェクト指向にマッチしていたりします。

try-catchは意図的にエラーハンドリングをする際には必須の機能となり、何かエラーがあった時にはそれを開発者にいち早く伝えてもらえるようメッセージを出したり、対処方法を画面に表示させたりして問い合わせを減らしたりと便利な使い方はいくらでもあります。

特にinterfaceは抽象的な概念を扱うものなので理解が難しいです。でも、使いこなせればとても便利なものです。

今自分にできることで何とかするのではなく、より良い書き方、機能がないか常にアンテナを高くし、自身のスキルとしていく

終わりに

ちょっとパソコンに詳しいから、業務フローに詳しいから、エクセルが得意だから、でsalesforceの担当になりフローや数式でナントなっているうちは良いですが、Apexの世界にそのままの気持ちでくると確実にsalesforceをダメなものに変えてしまいます。
でも気持ちを切り替えてApexを使ったプログラミングに真剣に取り組めばsalesforceはさらに使いやすくなり、あなたの価値も上がります。

あなたは、salesforceをダメにする人ですか？それともより良いものにする人ですか？