【GAS】期限切れ通知のスクリプトをガード節を使ってリファクタリングする。
誰だよこのコード書いたの!過去の自分だよ!あ~~~~~!
そんなコードを泣きながらリファクタリングしました。
なんでこんな書き方してたんだ自分。あほか。
それだけ成長したってことだな!
beforeとafterのコードを見比べ、その要点をメモする。
今回は問い合わせフォームの回答シートを見て、問い合わせに対応していなかったらslackに通知する、という内容のコード。これとほぼ同じ内容で、入金予定日に入金が無いものについても通知するといったものも作った。
配列指定なので、列を動かさないことを前提にしている。
連想配列にするとか、クラス化とか、反復メソッドとか、その辺はもっと規模の大きいコードを書くときに検討することにする。そのあたりのことをすると、初学者殺しにもなるので、現状、導入はしないほうがいいかな、とも思っている。自分がまだそのへんの理解をきちんとしていないのも大きい。
もっとイケテル書き方があるぜ!と思われそうだが、ひとまずこれで動いているので、堪忍やで。イケテル書き方、ぜひ教えてください。
例によって @etau0422 に大感謝です。
before
/**
* 最終更新 XXXX
* 更新者 XXXX
* トリガー インストラーブルトリガで週に一度 月曜朝8~9時 設定済
*
* <プロジェクト概要>
* 週に一度、問い合わせ対応していなかったらslackに通知する。=作業用シートの「対応状況」列が空白だったらslackに通知する。
*/
function noticeUnrespond() {
//シートを取得
const sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('作業用');
//最終行を指定
const lastRow = sheet.getLastRow();
//for文で上から下に走査。開始行2行目(1行目はヘッダーなので飛ばす)
for (let i = 1; i <= lastRow; i++) {
//i行目の縦12列目を取得
const targetRange = sheet.getRange(i, 12);
// console.log(targetRange.getA1Notation());
//空白かどうかをisBlankで判断。空白ならif節を実行。そうでないならelseを実行。
if (targetRange.isBlank()) {
// Logger.log("このセルは空白です");
//通知用のシート情報取得
const values = sheet.getDataRange().getValues();
// console.log('values',values);
// console.log('i',i);
//問い合わせ日時
const timestamp = Utilities.formatDate(values[i - 1][0], "JST", "yyyy年MM月dd日 HH:MM:ss");//問い合わせ日時
console.log(timestamp);
//問い合わせ者名前
const name = values[i - 1][2];
console.log(name);
//別の関数呼び出し。Slack通知。
sendSlack(timestamp, name);
} else {
// console.log("このセルは空白ではありませんので、特になにもしません");
}//else End
}//for End
}//function End
うううう。
values[i - 1][2];
のように、i - 1 というのが、なんともなあ。
else節も特に使ってないのに、そのままにしていた。
after
/**
* 最終更新 XXXX
* 更新者 XXXX
* トリガー インストラーブルトリガで週に一度 月曜朝8~9時 設定済
*
* <プロジェクト概要>
* 週に一度、問い合わせ対応していなかったらslackに通知する。=作業用シートの「対応状況」列が空白だったらslackに通知する。
*/
function noticeUnrespond() {
//シートを取得
const sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('作業用');
//最終行を指定
const lastRow = sheet.getLastRow();
//シートのすべての値。ここで二次元配列で一気に取得し、後のコードで配列で取り出す。
const values = sheet.getDataRange().getValues();
//for文で上から下に走査。i = 0 なら1行目から下に走査していく。
for (let i = 0; i <= lastRow - 1; i++) {
//L列 対応状況
const correspondenceStatus = values[i][11];
//ガード節。対応状況が空白でないなら、以下の処理に進まず、forの次のループに入る。
if (correspondenceStatus !== '') continue;
//問い合わせ日時
const timestamp = Utilities.formatDate(values[i][0], "JST", "yyyy年MM月dd日 HH:MM:ss");//問い合わせ日時
console.log({timestamp});
//問い合わせ者名前
const name = values[i][2];
console.log({name});
//別の関数呼び出し。Slack通知。
sendSlack(timestamp, name);
}//for End
}//function End
for (let i = 0; i <= lastRow - 1; i++) {
配列と実際の行数のズレということで、lastRow - 1 はもうしょうがないかなー。
function sendSlack
ついでに載せておく。function noticeUnrespond() の中で sendSlack(timestamp, name); を呼び出している。timestampとnameを渡し、sendSlack の中で通知文に利用している。
function sendSlack(timestamp, name) {
const postUr = 'https://hooks.slack.com/******';
// 投稿メッセージ
const message = `
<!channel>
【未対応の問い合わせがあります】
${timestamp}の${name}様からの問い合わせに対応していないようです。
シートを確認してください。
https://docs.google.com/spreadsheets/d/*****
対応済なら、L列「対応状況」のフラグを埋めておいてくださいね!
ここが空白だと、週に1度、通知しますよ!
`;
const jsonData =
{
"text": message
};
const payload = JSON.stringify(jsonData);
const options =
{
"method": "post",
"contentType": "application/json",
"payload": payload
};
UrlFetchApp.fetch(postUrl, options);
}
リファクタリングの要点
ガード節を用いて、ネストも浅くなって、読みやすくなった~!
getDataRange().getValues(); をfor文の外に置く。
これにより、ここでガッと配列でシート全体の値を取っておくことで多少は短縮になっているはず。今後、ここから値を取りだすものが増えても対応しやすい。
ガード節 if (correspondenceStatus !== '') continue;
これの意味は、correspondenceStatus が 空白でないなら(!で否定している)continueする、ということ。continueは、以下の処理に進まずに次のforのループに入る。余計な処理をスキップして次に進める。
isBlank()
isBlank() これは範囲に対して有効なので、values[i][11] のように値を取っているときには使えない。なので 比較演算子を用いて !== '' のように書いた。
今後
リファクタリングの本も読まなきゃかしら.…リーダブルコードもまだ読み終えてないけど…Effective JavaScript も気になってるんだよなあ。うぐぐぐ。
今日はそんな感じです!